Conversation
|
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 |
|
@guoye-zhang hm, interesting. I'll take a look at that. |
|
Just to but in here, this has fixed a load of test failures in Vapor that were failing with |
|
@guoye-zhang the failing conformance test now passes for me. |
|
I updated apple/swift-http-api-proposal#137 and it's still running into these errors with AHC conformance tests: |
|
|
|
I'm actually reproducing it 100% on my Mac running the test in Xcode. GitHub CI reproduced it as well. Previous workaround with |
# Conflicts: # Sources/NIOHTTPServer/NIOHTTPServer+HTTP1_1.swift # Sources/NIOHTTPServer/NIOHTTPServer+SecureUpgrade.swift # Sources/NIOHTTPServer/NIOHTTPServer.swift
gjcairo
force-pushed
the
http1-fixes
branch
2 times, most recently
from
b96e87c to
4947a0d
Compare
This reverts commit 51d83be.
|
|
||
| case .body(let element): | ||
| nonisolated(unsafe) let iter = iterator | ||
| self.readerState.putIterator(iter) |
There was a problem hiding this comment.
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.
| // 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@FranzBusch I've implemented this change.
There was a problem hiding this comment.
may have already been sent back.
can we check, if they already have been sent back? if not add the connection: close header?
There was a problem hiding this comment.
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.
6 participants
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.