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

The connection should emit a WindowUpdated event on INITIAL_WINDOW_SIZE change #1193

Open
pgjones opened this issue Jul 8, 2019 · 2 comments
Open

The connection should emit a WindowUpdated event on INITIAL_WINDOW_SIZE change #1193

pgjones opened this issue Jul 8, 2019 · 2 comments

Comments

@pgjones
Copy link
Member

pgjones commented Jul 8, 2019 •
edited
Loading

I think the connection should emit a WindowUpdated event on receipt of a settings frame that changes the INITIAL_WINDOW_SIZE. This is because this frame is valid after the receipt of headers and hence it implicitly updates the window (see _flow_control_change_from_settings). Without emitting this frame implementations have to specifically look for this settings change and respond appropriately themselves.

(I'll implement this, just wanted to get views on whether this is the correct thing to do).
pgjones added a commit to pgjones/hypercorn that referenced this issue Jul 8, 2019
@pgjones
Bugfix HTTP/2 flow control handling
10b214a 
There are two bugs fixed here, firslty negative flow control values
are truthy (only 0 is False), and whilst negative flow control values
are possible only positive ones should allow data to be sent.

Secondly if the INITIAL_WINDOW_SIZE setting is changed (after the
headers have been sent) h2 does not emit a WindowUpdated event (see
python-hyper/h2#1193) yet the window
should be updated.

This should fix the intermittent (race-condition) errors with h2spec
and the Trio worker.
@Lukasa
Copy link
Member

Lukasa commented Jul 8, 2019

I think that’s reasonable enough, yeah.

@vmagamedov
Copy link
Contributor

I had the same issue in grpclib. This is not obvious (all the ways window can change) but current implementation sticks more closely to the spec than proposed change. And you can always point to the RFC 7540 Section 6.9.2 in the documentation to explain how flow-control works.

Proposed change requires to generate fake WindowUpdated events for all current streams. This introduces additional (small?) overhead and this is just feels weird for me: WindowUpdate event doesn't mean WINDOW_UPDATE frame anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@vmagamedov @Lukasa @pgjones