Fix multiple requests on same HTTP1 connection by gjcairo · Pull Request #78 · swift-server/swift-http-server

gjcairo · 2026-04-02T10:57:50Z

We're not currently handling keep alive HTTP1 connections properly: we close connections at the end of the first request, yet we don't send a connection close header. This PR fixes that by reusing the iterator until the stream is finished, so we can keep reading all requests that come through the same connection.

This also removes 6.2 support because there are some compiler issues with sendability that have not been properly addressed.

guoye-zhang · 2026-04-02T19:24:46Z

I pointed apple/swift-http-api-proposal#137 to the latest commit on this branch, but the AHC conformance test is still failing it appears

gjcairo · 2026-04-07T15:01:07Z

@guoye-zhang hm, interesting. I'll take a look at that.

0xTim · 2026-04-09T16:36:05Z

Just to but in here, this has fixed a load of test failures in Vapor that were failing with Caught error: I/O on closed channel so thank you!

gjcairo · 2026-04-13T21:11:35Z

@guoye-zhang the failing conformance test now passes for me.

guoye-zhang · 2026-04-14T04:57:05Z

I updated apple/swift-http-api-proposal#137 and it's still running into these errors with AHC conformance tests: ✘ Test testOk failed with error: I/O on closed channel

gjcairo · 2026-04-14T09:43:12Z

testOk and testBasicRedirect are flaky, but they only fail like twice if I run them 100 times.
Are you seeing them fail more consistently than this? Are we sure this is an issue on the server side?

guoye-zhang · 2026-04-14T18:08:13Z

I'm actually reproducing it 100% on my Mac running the test in Xcode. GitHub CI reproduced it as well. Previous workaround with Connection: close never failed for me.

# Conflicts: # Sources/NIOHTTPServer/NIOHTTPServer+HTTP1_1.swift # Sources/NIOHTTPServer/NIOHTTPServer+SecureUpgrade.swift # Sources/NIOHTTPServer/NIOHTTPServer.swift

This reverts commit 51d83be.

gjcairo · 2026-05-05T12:41:31Z


                        case .body(let element):
+                            nonisolated(unsafe) let iter = iterator
+                            self.readerState.putIterator(iter)


This is unfortunate because we need to go via the lock after each read - may slow things down considerably. Not sure what the alternative is other than not put any of this behind a lock, which I believe should be okay since we shouldn't be consuming the reader from more than one place at a time.

FranzBusch · 2026-05-05T12:56:46Z

+        // If the handler didn't fully consume the request body, drain the remaining
+        // parts so the iterator is positioned at the start of the next request.
+        // Errors during draining are not propagated — if the drain fails, we simply
+        // cannot reuse this connection.
+        if !readerState.wrapped.withLock({ $0.finishedReading }) {
+            do {
+                drainLoop: while true {
+                    switch try await recoveredIterator.next(isolation: #isolation) {


We shouldn't do this. If the handler didn't fully consume the request body then we should close the connection in my opinion. Otherwise this could allow for attacks where an attacker crafts a request that leads to the handler responding early without having ready everything but keep the connection in an infinite draining state.

Hm yeah okay, you're right about the security concern. But wouldn't it potentially be a valid use case for a handler to respond early without consuming the whole request? The conformance tests actually do this for some requests: they don't read the request body (and thus they don't read .end) because they only look at the headers (or even nothing). Are we saying we should document that all handler implementations should make sure they always fully consume the request?

I just took a quick look what Go does and they cap the drain to 256KB to stop bad actors. https://cs.opensource.google/go/go/+/master:src/net/http/server.go;l=1137?q=server.go&ss=go%2Fgo

@fabianfett WDYT?

I think this is reasonable. The only thing is that we can't really send back Connection:close by the time we run this drain loop (which is what Go does), because a response (and thus headers) may have already been sent back. We would just close the connection - which maybe we're okay with bur just pointing it out.

@FranzBusch I've implemented this change.

may have already been sent back.

can we check, if they already have been sent back? if not add the connection: close header?

They will have been sent if the response sender was consumed, which happens whenever a Response is sent back to the client (by calling responseSender.send(...). It would be odd to implement a handler that sends nothing back to the client and just returns; but yes, we could check whether responseSender.send has been called if we think that's useful.

We discussed this offline and decided that we would have a different approach: instead of the somewhat arbitrary 256kb payload drain, I've added an H1-only channel handler that adds the Connection:close header whenever we write a response .end or start sending back a response body when we haven't yet seen a request .end. After sending the request .end part, we'll close the connection.

fabianfett · 2026-05-18T10:00:09Z

+            // Begin a new request. (Any previous request's response must have
+            // completed already since HTTPServerPipelineHandler enforces ordering.)


i think this is only true if pipelining is disabled.

Hm, from the HTTPServerPipelineHandler docs:

This handler ensures that HTTP server pipelines only process one request at a time.

https://github.com/apple/swift-nio/blob/6338e9fb5e136af8337b48eee65f8868d571a3d7/Sources/NIOHTTP1/HTTPPipelineSetup.swift#L232

yes, if we ensure that this is always enabled.

That option is enabled so we currently have this handler set up, which means this should be safe. We don't provide a way to change this configuration.

fabianfett · 2026-05-18T10:05:24Z

+        }
+        context.fireChannelRead(data)
+    }
+


I think this also needs a timeout in the form of channelReadComplete. We should check the state in channelReadComplete and flush the head.

I'm trying to keep the timeouts in #72 - I haven't updated it since we had our chat though, I'll re-assign to you when I do.

channelReadComplete isn't a timeout though :) It's another NIO callback.

I know, but then I don't think I understand your original comment :D

fabianfett · 2026-05-18T10:05:55Z

+/// Informational (1xx) responses pass through unchanged and do not affect buffering
+/// state.
+@available(macOS 26.2, iOS 26.2, watchOS 26.2, tvOS 26.2, visionOS 26.2, *)
+final class HTTPKeepAliveHandler: ChannelDuplexHandler {


I really think we should unify the number of handlers here significantly. Doing this in a follow up is fine.

Sure, but I don't want to do it at this point - the timeouts are in a separate PR, and having them separate makes reviewing easier IMO. I will consolidate them in a follow up.

gjcairo added the 🆕 semver/minor Adds new public API. label Apr 2, 2026

aryan-25 reviewed Apr 2, 2026

View reviewed changes

Comment thread Sources/NIOHTTPServer/HTTPRequestConcludingAsyncReader.swift Outdated

gjcairo force-pushed the http1-fixes branch from 0fc6e24 to 25ce8b2 Compare April 9, 2026 15:09

gjcairo requested a review from aryan-25 April 13, 2026 16:44

gjcairo force-pushed the http1-fixes branch from bf400bc to fa9de56 Compare April 13, 2026 16:48

gjcairo added 7 commits April 24, 2026 11:04

Fix multiple requests on same HTTP1 connection

637487d

# Conflicts: # Sources/NIOHTTPServer/NIOHTTPServer+HTTP1_1.swift # Sources/NIOHTTPServer/NIOHTTPServer+SecureUpgrade.swift # Sources/NIOHTTPServer/NIOHTTPServer.swift

Format

4e5ad11

Exit request loop if task is cancelled

cfc55a4

Simplify iterator sharing cycle

fd80541

Fix flaky test

3f0a85a

Disable 6.2

0362155

Drain request when not done reading

89ec767

gjcairo force-pushed the http1-fixes branch 2 times, most recently from b96e87c to 4947a0d Compare April 30, 2026 10:35

Add some additional temporary logging

51d83be

gjcairo force-pushed the http1-fixes branch from 4947a0d to 51d83be Compare April 30, 2026 11:02

Revert "Add some additional temporary logging"

cc71a04

This reverts commit 51d83be.

guoye-zhang approved these changes Apr 30, 2026

View reviewed changes

gjcairo commented May 5, 2026

View reviewed changes

FranzBusch reviewed May 5, 2026

View reviewed changes

gjcairo added 3 commits May 6, 2026 10:47

Drain requests up to 254KB at end of handler in H1

3d25f81

Add HTTPKeepAliveHandler

ba2dde7

Format

4dba699

gjcairo requested review from FranzBusch, ehaydenr and fabianfett May 15, 2026 16:09

fabianfett reviewed May 18, 2026

View reviewed changes

gjcairo requested a review from fabianfett May 18, 2026 10:55

Format

dc7f30e

		// Begin a new request. (Any previous request's response must have
		// completed already since HTTPServerPipelineHandler enforces ordering.)

Conversation

gjcairo commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

guoye-zhang commented Apr 2, 2026

Uh oh!

gjcairo commented Apr 7, 2026

Uh oh!

0xTim commented Apr 9, 2026

Uh oh!

gjcairo commented Apr 13, 2026

Uh oh!

guoye-zhang commented Apr 14, 2026

Uh oh!

gjcairo commented Apr 14, 2026

Uh oh!

guoye-zhang commented Apr 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabianfett May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gjcairo May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabianfett May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gjcairo commented Apr 2, 2026 •

edited

Loading

fabianfett May 6, 2026 •

edited

Loading

gjcairo May 19, 2026 •

edited

Loading

fabianfett May 18, 2026 •

edited

Loading