Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DefaultHttp2RemoteFlowController not re-visiting nodes that still have streamable bytes #4266

Closed
Scottmitch opened this issue Sep 23, 2015 · 7 comments
Closed

DefaultHttp2RemoteFlowController not re-visiting nodes that still have streamable bytes #4266

Scottmitch opened this issue Sep 23, 2015 · 7 comments
Assignees
@Scottmitch
Labels
defect
Milestone
4.1.0.Beta7

Comments

@Scottmitch
Copy link
Member

The allocation algorithm may not distributed all the available bytes even though the streams may be able to use all the bytes. This is caused by the children that have streamable bytes not being considered hungry because the nextConnectionWindow has collapsed.

Also related to DefaultHttp2RemoteFlowController not taking into account the streamable bytes of peers.

int connectionWindowChunk = max(1, (int) (connectionWindow * (child.weight() / (double) totalWeight)));
@Scottmitch Scottmitch self-assigned this Sep 23, 2015
@Scottmitch Scottmitch added this to the 4.1.0.Beta7 milestone Sep 23, 2015
@Scottmitch
Copy link
Member Author

@nmittler - FYI

@Scottmitch
Copy link
Member Author

This is also related to #4242

@blucas
Copy link
Contributor

blucas commented Sep 24, 2015

@Scottmitch - Could you give an example for when this would happen? I haven't had this happen to me yet when testing HTTP/2 with Chrome/FF.

@nmittler
Copy link
Member

+1

@Scottmitch as discussed offline, the first thing we need is a new test in DefaultHttp2RemoteFlowControllerTest that demonstrates this.

@Scottmitch
Copy link
Member Author

@nmittler - On it.

@Scottmitch
Copy link
Member Author

@blucas - This issue was really the root cause of the issue #4242 you reported. However the fix for #4258 (which is also necessary for other reasons) should mask the negative effects which may result from this issue. I think the only "visible" effects from this should be extra tree traversal(s) to make sure all bytes are written.

@Scottmitch Scottmitch mentioned this issue Sep 25, 2015
Closed
Scottmitch added a commit to Scottmitch/netty that referenced this issue Sep 25, 2015
@Scottmitch
DefaultHttp2RemoteFlowController not allocating all available bytes
f1077c9 
Motivation:
DefaultHttp2RemoteFlowController's allocation algorithm may not allocate all bytes that are available in the connection window. If the 'fair share' based upon weight is not fully used by sibling nodes it was not correctly re-distributed to other sibilings which may be able to utilize part / all of that share.

Modifications:
- Add a unit test which demonstrates the issue.
- Modify the allocation algorithm to ensure all available bytes are allocated.

Result:
Fixes netty#4266
Scottmitch added a commit that referenced this issue Sep 25, 2015
@Scottmitch
DefaultHttp2RemoteFlowController not allocating all available bytes
93011dd 
Motivation:
DefaultHttp2RemoteFlowController's allocation algorithm may not allocate all bytes that are available in the connection window. If the 'fair share' based upon weight is not fully used by sibling nodes it was not correctly re-distributed to other sibilings which may be able to utilize part / all of that share.

Modifications:
- Add a unit test which demonstrates the issue.
- Modify the allocation algorithm to ensure all available bytes are allocated.

Result:
Fixes #4266
@Scottmitch
Copy link
Member Author

Fixed by #4282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Scottmitch Scottmitch

Labels
defect
Projects
None yet
Milestone
4.1.0.Beta7
Development

No branches or pull requests
3 participants
@blucas @nmittler @Scottmitch