Handle NIOSSLError.uncleanShutdown correctly #472
Conversation
| // if we have already received the response head, the parser will ensure that we receive | ||
| // the complete response. we can ignore this error. we might see a HTTPParserError very | ||
| // soon. | ||
| if responseHead.headers.contains(name: "content-length") || responseHead.headers.contains(name: "transfer-encoding") { |
There was a problem hiding this comment.
This is not a sufficient check: there are cases where these headers are not present, but we are nonetheless confident of the length of the response. Specifically:
- Responses to a HEAD request
- Responses with a 204 or 304 status code
There was a problem hiding this comment.
The HTTPResponseDecoder will always emit a response .end after having received a response head, if the request had a HEAD or CONNECT method or the response status is 204 or 304.
https://github.com/apple/swift-nio/blob/main/Sources/NIOHTTP1/HTTPDecoder.swift#L301-L314
For this reason, we can expect that we will only be in the streaming state, if we have a "content-length" or "transfer-encoding" header or if the response is EOF terminated. I'm well aware that this is very implicit. If we want to have explicit checks here, I will need to come up with a way to preserve the request method a little longer.
There was a problem hiding this comment.
Hmm, can we get this logic into a comment then? The correctness of this code is not apparent merely from reading it.
|
|
||
| // if we have already received the response head, the parser will ensure that we receive | ||
| // the complete response. we can ignore this error. we might see a HTTPParserError very | ||
| // soon. |
There was a problem hiding this comment.
This comment seems to be out of step with the code: it implies that we can ignore the error, but then has logic that sometimes doesn't ignore the error.
fabianfett
force-pushed
the
ff-verify-unclean-shutdown-is-solved
branch
2 times, most recently
from
36a9c6d to
5b5931b
Compare
fabianfett
force-pushed
the
ff-verify-unclean-shutdown-is-solved
branch
from
5b5931b to
3e03472
Compare
3 participants
Motivation
Fixes #238 and #231.
Changes
HTTPClientTestsinto their own fileHTTPClientUncleanSSLConnectionShutdownTestsuncleanShutdownshould be handled automatically (where possible) #238 into the test fileignoreUncleanSSLShutdowneverywhereResult
ignoreUncleanSSLShutdownonHTTPClient.Configurationis deprecated and ignored.