Avoid emitting WINDOW_UPDATE frames when receiving stream closed #233
Conversation
This file contains 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
glbrntt
added
the
patch-version-bump-only
Lukasa
reviewed
Aug 24, 2020
There was a problem hiding this comment.
I think passing this as a flag doesn't quite work: if for any reason we get a re-entrant call to read0 we'll lose track of the state.
Perhaps this should be state on the InboundWindowManager: we can flip a bit there to say that there is no point sending window update frames anymore. This can then be done in a bunch of places, including when we receive END_STREAM.
glbrntt
force-pushed
the
gb-no-such-stream
branch
from
007171c
to
fc90c57
Compare
Lukasa
reviewed
Aug 24, 2020
| @@ -67,7 +71,16 @@ struct InboundWindowManager { | |||
| return self.calculateWindowIncrement(windowSize: lastWindowSize + self.bufferedBytes) | |||
| } | |||
|
|
|||
| mutating func close() { | |||
There was a problem hiding this comment.
I don't think the indirection is needed; lets just have closed be an internal property on the window manager and set it directly.
glbrntt
force-pushed
the
gb-no-such-stream
branch
from
fc90c57
to
754d458
Compare
Motivation: In some cases it's possible for a stream to emit a WINDOW_UPDATE frame after receiving close from the network. This is unfortunate because in the eyes of the `NIOHTTP2Handler` the stream no longer exists which results in a connection-level error. Modifications: - Add a `close()` to the `InboundWindowManager`, no new frame sizes will be produced if close has been called - Use the above functionality when the stream is closing and when we have seen end stream. Result: WINDOW_UPDATE frames are no longer emitted from the stream as a result of delivering pending reads while closing.
glbrntt
force-pushed
the
gb-no-such-stream
branch
from
754d458
to
643b923
Compare
Lukasa
approved these changes
Aug 24, 2020
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:
In some cases it's possible for a stream to emit a WINDOW_UPDATE frame
after receiving close from the network. This is unfortunate because in
the eyes of the
NIOHTTP2Handlerthe stream no longer exists whichresults in a connection-level error.
Modifications:
deliverPendingReadstelling it whether it is allowedto emit a WINDOW_UPDATE frame
Result:
WINDOW_UPDATE frames are no longer emitted from the stream as a result
of delivering pending reads while closing.