-
-
Notifications
You must be signed in to change notification settings - Fork 15.9k
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
Always consume bytes for closed HTTP/2 streams. #3672
Conversation
Motivation: The current local flow controller does not guarantee that unconsumed bytes for a closed stream will be restored to the connection window. This may lead to degradation of the connection window over time. Modifications: Modified DefaultHttp2LocalFlowController to guarantee that any unconsumed bytes are returned to the connection window as soon as the stream is closed. We also immediately consume any bytes when receiving DATA for a closed stream. Result: Fixes netty#3668
@Scottmitch can you take a look? |
} | ||
|
||
@Override | ||
public int unconsumedBytes(Http2Stream stream) { | ||
return state(stream).unconsumedBytes(); | ||
return isClosed(stream) ? 0 : state(stream).unconsumedBytes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to do this? If we are forcing the unconsumed bytes to 0 in the connection listener, is that good enough...or is this method used while the stream is being closed before the listener is notified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't need to do this in this PR. This was the result of some of my work for adding stream history, where closed streams would no longer have a FlowState
property. Since we still need to apply flow control for closed streams, I didn't want these methods to require that the FlowState
exist. WDYT ... revert this for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah lets revert for now.
@Scottmitch PTAL |
// Update the number of active streams initiated by the endpoint. | ||
stream.createdBy().numActiveStreams--; | ||
} | ||
notifyClosed(stream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Scottmitch this is somewhat unrelated, but I noticed that this seemed wrong. The previous logic would only let us close/remove if the stream had at one point become active. This change lifts this restriction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nmittler - Good catch. I'm wondering if this should even be in this method. Would it make sense in DefaultStream.close()
after state = CLOSED
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Scottmitch possibly. Although when onClosed()
is called I would like it to be also removed from the active list. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave it per our discussion.
@nmittler - Finished another round. Just 1 real question. |
@Scottmitch I've replied to your question with a question :) |
cherry-picked into master and 4.1 |
Thanks @nmittler! |
Motivation:
The current local flow controller does not guarantee that unconsumed bytes for a closed stream will be restored to the connection window. This may lead to degradation of the connection window over time.
Modifications:
Modified DefaultHttp2LocalFlowController to guarantee that any unconsumed bytes are returned to the connection window as soon as the stream is closed. We also immediately consume any bytes when receiving DATA for a closed stream.
Result:
Fixes #3668