Skip to content

x/net/http2: flow control over payload length, not data length #16556

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
Closed
bradfitz opened this issue Jul 31, 2016 · 3 comments
Closed

x/net/http2: flow control over payload length, not data length #16556

bradfitz opened this issue Jul 31, 2016 · 3 comments
Labels
FrozenDueToAge
Milestone
Go1.7Maybe

Comments

@bradfitz
Copy link
Contributor

http://httpwg.org/specs/rfc7540.html#rfc.section.6.1 says:

The entire DATA frame payload is included in flow control, including the Pad Length and Padding fields if present.

We ignore the pad length and padding fields in our accounting.

I thought the fix was as easy as changing len(f.Data()) to f.Length in a few places, but then our accounting would be off when we return the flow control tokens on bytes read later. We'll probably want to just immediately return the flow control tokens for any padding immediately, in both the client and server.
@bradfitz bradfitz added this to the Go1.8Early milestone Jul 31, 2016
@bradfitz bradfitz self-assigned this Jul 31, 2016
@bradfitz bradfitz modified the milestones: Go1.7Maybe, Go1.8Early Jul 31, 2016
@bradfitz
Copy link
Contributor Author

Sent https://go-review.googlesource.com/25382 for this too. The CL should be pretty low risk for people who don't use padding (no new code paths), but pretty valuable to those who do, so I think we should probably include it in 1.7.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/25382 mentions this issue.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/25388 mentions this issue.

gopherbot pushed a commit that referenced this issue Aug 2, 2016
@bradfitz
net/http: update bundled http2
c558a53 
Updates bundled http2 to x/net/http2 rev 28d1bd4f for:

    http2: make Transport work around mod_h2 bug
    https://golang.org/cl/25362

    http2: don't ignore DATA padding in flow control
    https://golang.org/cl/25382

Updates #16519
Updates #16556
Updates #16481

Change-Id: I51f5696e977c91bdb2d80d2d56b8a78e3222da3f
Reviewed-on: https://go-review.googlesource.com/25388
Reviewed-by: Chris Broadfoot <[email protected]>
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
@golang golang locked and limited conversation to collaborators Aug 2, 2017
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
@bradfitz
http2: don't ignore DATA padding in flow control
898610a 
http://httpwg.org/specs/rfc7540.html#rfc.section.6.1 says:

> The entire DATA frame payload is included in flow control, including
> the Pad Length and Padding fields if present.

But we were just ignoring the padding and pad length, which could lead
to clients and servers getting out of sync and deadlock if peers used
padding. (Go never does, so it didn't affect Go<->Go)

In the process, fix a lingering bug from golang/go#16481 where we
didn't account for flow control returned to peers in the stream's
inflow, despite sending the peer a WINDOW_UPDATE.

Fixes golang/go#16556
Updates golang/go#16481

Change-Id: If7150fa8f0da92a60f34af9c3f754a0346526ece
Reviewed-on: https://go-review.googlesource.com/25382
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Andrew Gerrand <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.7Maybe
Development

No branches or pull requests
2 participants
@bradfitz @gopherbot