Zero length data frames should apply flow control #3348
Conversation
|
AUTOMATED MESSAGE for ADMINS: |
| @@ -252,7 +252,7 @@ public int onDataRead(final ChannelHandlerContext ctx, int streamId, ByteBuf dat | |||
| Http2LocalFlowController flowController = flowController(); | |||
| try { | |||
| // If we should apply flow control, do so now. | |||
| if (shouldApplyFlowControl) { | |||
| if (shouldApplyFlowControl && bytesToReturn > 0) { | |||
| flowController.receiveFlowControlledFrame(ctx, stream, data, padding, endOfStream); | |||
There was a problem hiding this comment.
entering this when bytesToReturn == 0 leads to assertion errors
|
@netkins accept |
|
@Scottmitch @nmittler please check |
| @@ -252,7 +252,7 @@ public int onDataRead(final ChannelHandlerContext ctx, int streamId, ByteBuf dat | |||
| Http2LocalFlowController flowController = flowController(); | |||
| try { | |||
| // If we should apply flow control, do so now. | |||
| if (shouldApplyFlowControl) { | |||
| if (shouldApplyFlowControl && bytesToReturn > 0) { | |||
There was a problem hiding this comment.
I think we should fix the assertion statement instead to accept 0 (i.e. assert dataLength >= 0). Not because 0 is expected to impact flow control but the endOfStream still needs to be processed. If more issues fall out from this we should investigate those too.
There was a problem hiding this comment.
Agreed, I think this statement was correct before. We should always be applying flow control if shouldApplyFlowControl is true.
Which assertion was failing? In the new test?
There was a problem hiding this comment.
Which assertion was failing? In the new test?
I am guessing @adriancole was referring to this assertion statement. Please correct me if I'm wrong (or if there are other asserts that are also failing).
There was a problem hiding this comment.
Ah right, I think that assert should just make sure the value passed in isn't negative (i.e. >=).
There was a problem hiding this comment.
@Scottmitch sorry, I read your original comment via e-mail so I didn't realize you had already provided the link ;)
There was a problem hiding this comment.
sorry, I read your original comment via e-mail so I didn't realize you had already provided the link ;)
No problem. Just to confuse things even more...I sometimes edit my comments to clarify/correct after the emails go out anyhow :). I always double check comments I get through email on github.
|
@adriancole - Good catch and thanks for the patch! I have left a few comments, let me know what you think. |
|
@adriancole good catch! :) |
codefromthecrypt
changed the title
|
Thanks'all. I've updated the code and the PR description. |
|
@adriancole could you please update your commit comment http://netty.io/wiki/writing-a-commit-message.html? |
Motivation: A downstream consumer of Netty failed as emitting zero-length http2 data frames in a unit test resulted in assertion errors in Http2LocalFlowController. Since zero-length frames are valid, an assertion that http2 data frame length must be positive is invalid. Modifications: Assertions of data length in Http2LocalFlowController now permit zero. Result: Those running netty with assertions on can now emit zero length http2 data frames.
|
big commit message applied :) |
|
@adriancole thanks buddy! Cherry-picked into master. @Scottmitch we need to remember to cherry-pick this one once http2 is backported into 4.1 |
|
@normanmaurer - Yes. Will do. |
|
cherry picked into 4.1 (c6bfc92) |
|
thanks for the review and pickings! |
Eventhough zero-length data frames don't have a WINDOW_UPDATE side-effect, flow-control logic should be applied. This allows for proper end-of-stream handling.