Fix crash when receiving 2xx response before stream is complete. #591
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation It's totally acceptable for a HTTP server to respond before a request upload has completed. If the response is an error, we should abort the upload (and we do), but if the response is a 2xx we should probably just finish the upload. In this case it turns out we'll actually hit a crash when we attempt to deliver an empty body message. This is no good! Once that bug was fixed it revealed another: while we'd attempted to account for this case, we hadn't tested it, and so it turns out that shutdown would hang. As a result, I've also cleaned that up. Modifications - Tolerate empty circular buffers of bytes when streaming an upload. - Notify the connection that the task is complete when we're done. Result Fewer crashes and hangs.
Lukasa
force-pushed
the
cb-crash-receiving-body-parts
branch
from
eb0d918 to
6752103
Compare
fabianfett
approved these changes
Jun 6, 2022
| @@ -267,6 +267,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { | |||
| writePromise.futureResult.whenComplete { result in | |||
| switch result { | |||
| case .success: | |||
| self.connection.taskCompleted() | |||
There was a problem hiding this comment.
Would you mind adding a comment here? Something like:
If our final action is sendRequestEnd this means, that we have already received the complete response. For this reason we must mark the connection as idle to the pool, once we have uploaded all body parts.
dnadoba
approved these changes
Jun 7, 2022
| @@ -1253,6 +1253,37 @@ class HTTPEchoHandler: ChannelInboundHandler { | |||
| } | |||
| } | |||
|
|
|||
| class HTTP200DelayedHandler: ChannelInboundHandler { | |||
There was a problem hiding this comment.
super tiny nit
Suggested change
| class HTTP200DelayedHandler: ChannelInboundHandler { | |
| final class HTTP200DelayedHandler: ChannelInboundHandler { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
It's totally acceptable for a HTTP server to respond before a request
upload has completed. If the response is an error, we should abort the
upload (and we do), but if the response is a 2xx we should probably just
finish the upload.
In this case it turns out we'll actually hit a crash when we attempt to
deliver an empty body message. This is no good!
Once that bug was fixed it revealed another: while we'd attempted to
account for this case, we hadn't tested it, and so it turns out that
shutdown would hang. As a result, I've also cleaned that up.
Modifications
Result
Fewer crashes and hangs.
Resolves #562