Skip to content

ChannelRead because of closing connection: Remove preconditions #430

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 20, 2021

Conversation

fabianfett
Copy link
Member

Motivation

NIO may send channelRead events without a handlers requesting more data with a context.read() invocation. This happens if the remote has closed the connection and NIO wants to inform the handlers as soon as possible.

Changes

  • Don't precondition on channelRead events anymore.
  • Close channel if we received an http end without a context.read() invocation

@fabianfett fabianfett added the 🔨 semver/patch No public API change. label Sep 20, 2021
@fabianfett fabianfett added this to the HTTP/2 support milestone Sep 20, 2021
@fabianfett fabianfett force-pushed the ff-http-final-messages branch from b2dc4ae to 4424dec Compare September 20, 2021 13:03
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one request for comment.

@@ -536,16 +536,21 @@ struct HTTPRequestStateMachine {
assert(producerState == .paused, "Expected to have paused the request body stream, when the head was received. Invalid state: \(self.state)")

return self.avoidingStateMachineCoW { state -> Action in
let remainingBuffer = responseStreamState.end()
let (remainingBuffer, _) = responseStreamState.end()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to explain in a comment why here and above you don't care about the second tuple element.

@fabianfett fabianfett force-pushed the ff-http-final-messages branch from 4424dec to 1acdb0c Compare September 20, 2021 13:18
Copy link
Collaborator

@dnadoba dnadoba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fabianfett fabianfett force-pushed the ff-http-final-messages branch from 596c58f to 2189eb6 Compare September 20, 2021 13:43
@fabianfett fabianfett requested a review from Lukasa September 20, 2021 16:08
@fabianfett fabianfett merged commit bef8878 into swift-server:main Sep 20, 2021
@fabianfett fabianfett deleted the ff-http-final-messages branch September 20, 2021 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants