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

x/net/http2: add ability to specify priority to http2.Transport.RoundTrip #24289

jared2501 opened this issue Mar 7, 2018 · 2 comments
FeatureRequest NeedsDecision


Copy link

@jared2501 jared2501 commented Mar 7, 2018

Example diff:

Use case: I'm implementing an HTTP/2 reverse that would like to pass through the priority that the client sends to the upstream server.

@andybons andybons changed the title Add ability to specify priority to http2.Transport.RoundTrip x/net/http2: add ability to specify priority to http2.Transport.RoundTrip Mar 7, 2018
@andybons andybons added the NeedsDecision label Mar 7, 2018
Copy link

@andybons andybons commented Mar 7, 2018

/cc @bradfitz

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Mar 28, 2018
Copy link

@meirf meirf commented Apr 21, 2018

Some notes on the example code (just in case someone uses it as a basis for a CL):

RFC 7540 discusses two ways of sending priority information:

  1. include in HEADERS frame
  2. in a separate PRIORITY frame

A client can assign a priority for a new stream by including prioritization information in the HEADERS frame (Section 6.2) that opens the stream. At any other time, the PRIORITY frame (Section 6.3) can be used to change the priority of a stream.

It looks like the example code uses option 2:

if priority != nil {, *priority)

but I think what we want is option 1: pass RoundTripOpt.Priority to writeHeaders -> HeadersFrameParam.Priority

Separately, imo the API change (using RoundTripOpt) looks reasonable: minimal and non-breaking.

Lastly, for the record, there is also the case of sending priority when receiving a promise. See StreamPriority in this comment. But that code hasn't been merged and that case might not be so important so please ignore for the purpose of this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
FeatureRequest NeedsDecision
None yet

No branches or pull requests

5 participants