account for padding in flow control, when received frame uses it #1076
Conversation
This file contains hidden or 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
Contributor
Author
|
Created pr to attempt to test/verify this btw, in grpc/grpc#9776. |
menghanl
reviewed
Feb 17, 2017
transport/http2_client.go
Outdated
| } | ||
| return | ||
| } | ||
| if f.Header().Flags.Has(http2.FlagDataPadded) { |
Contributor
There was a problem hiding this comment.
Can this be moved inside if size > 0?
Contributor
Author
There was a problem hiding this comment.
just updated to move this on client and server
apolcyn
force-pushed
the
account_for_padding
branch
from
5e3e6d5 to
3de1621
Compare
Contributor
|
LGTM. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
It looks like current transport doesn't account for padding when consuming/updating the flow control window. This accounts for it, maybe another option is to fail fast.
I'd like to add a large interop test for this situation. For now reproduced the sender deadlock locally by using
http2.Framer.WritePaddedon client, accounting for padding in client's send quota, and running a long-streaming benchmark which gets QPS 0.cc @ejona86 @MakMukhi
update: btw there's WIP for a generic interop test for this, but for the main grpc repo, separate from this pr