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.

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

Send window update after the peer sends half the limit on a stream or connection #486

Merged
merged 1 commit into from
Jan 27, 2014
Merged

Send window update after the peer sends half the limit on a stream or connection #486

merged 1 commit into from
Jan 27, 2014

Conversation

codefromthecrypt
Copy link

This is the final change for connection-level flow control in spdy or http/2.

*/
int initialWindowSize = 65535;
// Visible for testing
long bytesLeftBeforeReadAck;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer the way the SpdyStream class tracks the inverse of this, unacknowledgedBytes. My issue with bytesLeftBeforeReadAck is that it suggests a single hard deadline (0) but for maximum throughput we should ack earlier than necessary.

@swankjesse
Copy link
Collaborator

One missing synchronized block. Is it safe to grab the connection lock while holding the stream lock? I'm hoping it is.

@codefromthecrypt
Copy link
Author

grabbed a connection lock, outside the stream lock. should be good to go!

* account max frame size per variant.
*/
@Test @Ignore public void readSendsWindowUpdateHttp2() throws Exception {
@Test public void readSendsWindowUpdateHttp2() throws Exception {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

swankjesse added a commit that referenced this pull request Jan 27, 2014
Send window update after the peer sends half the limit on a stream or connection
@swankjesse swankjesse merged commit 7bf4853 into square:master Jan 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants