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: support consuming PUSH_PROMISE streams in the client #18594

Open
glasser opened this issue Jan 10, 2017 · 30 comments

Comments

@glasser
Copy link
Contributor

commented Jan 10, 2017

Go 1.8 will contain support for servers to produce PUSH_PROMISE streams, but there is no support for clients to receive them.

Specifically, it would be nice if proxies built with net/http/httputil's ReverseProxy to be able to proxy server pushes.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 10, 2017

Let's ignore ReverseProxy for the purpose of this bug. Although, ReverseProxy being in std would require that new API be added to net/http.Client.

I think more immediately we'd want to add experimental, opt-in API only to golang.org/x/net/http2.Transport. (Unless opted-in, the Transport would continue to advertise SETTINGS_ENABLE_PUSH == 0)

Got any API proposals?

@glasser

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2017

I admit I'm not an http2 expert. I maintain a proxy based on ReverseProxy and "proxy http2 server pushes" is a feature request I got from a user.

My understanding is that receiving a push is effectively similar to receiving a request and a response from a server. So perhaps there's a new field on http2.Transport which is a ReceivePusher, where

type ReceivePusher interface {
  ReceivePush(*http.Request, *http.Response) error
}

If the field is non-nil, then the Transport advertises SETTINGS_ENABLE_PUSH == 1, and upon receiving a PUSH_PROMISE frame, the Transport creates a Request and a Response that match the PUSH_PROMISE and passes it to the ReceivePusher. Just like with RoundTrip, it is the responsibility of ReceivePush to read from and Close the response's Body.

I'm not sure what is the best way to indicate "I don't want this push" (ie send a RST_STREAM I think?) — maybe any non-nil error return value? Maybe a second return value other than the error? Some other interface passed in that it can invoke? Some method on the Transport?

My understanding is that these pushes can be received at any time during the connection: concurrently with RoundTrip or after it. Not really sure how that should affect the API.

@tombergan

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2017

It seems right to translate each PUSH_PROMISE into an http.Request that has no request body. A common thing in browsers is to inspect the PUSH_PROMISE, check if it's already cached, and if so, cancel the push with RST_STREAM(CANCEL). Whatever the API looks like, it should allow canceling the pushed stream before any bytes of the response have been received.

A second point is that every PUSH_PROMISE must be associated with a client request. This is required by RFC 7540. It may be useful to communicate that associated request to the client's push handler.

@nilslice

This comment has been minimized.

Copy link

commented Feb 17, 2017

I think the interface proposed by @glasser is good, though would likely need some additional control methods, for example to cancel/block the PUSH_PROMISE frame. as per h2 spec:

If the client determines, for any reason, that it does not wish to receive the pushed response from the server or if the server takes too long to begin sending the promised response, the client can send a RST_STREAM frame, using either the CANCEL or REFUSED_STREAM code and referencing the pushed stream's identifier.

To keep it limited to the http2.Transport suggested by @bradfitz, it would seem like some configuration to the RoundTripOpt would be required to state whether the Transport.RoundTrip Response should check for some cache of stream IDs, but I'm not sure where it makes sense to do the bookkeeping.

The ReceivePush method could return an error signaling the decision CANCEL/REFUSE_STREAM, but how it comes to that conclusion should be based on some configurable rules. If it accepts the push, does the Transport keep track of the stream IDs to read later?

@tombergan

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2017

How about something like the following?

package http2

// PushHandler consumes a pushed response.
//
// HandlePush will be called once for every PUSH_PROMISE received
// from the server. If HandlePush returns before the pushed stream
// has completed, the stream will be canceled.
//
// Maybe: allow setting the RST_STREAM code via the error message from
// HandlePush. Or, remove the error return and allow sending a RST_STREAM
// via an explicit PushedRequest.Cancel(errCode int32) method.
type PushHandler interface {
  HandlePush(r *PushedRequest) error
}

// PushedRequest describes a request that was pushed from the server.
type PushedRequest struct {
  // Promise summarizes the HTTP/2 PUSH_PROMISE message. The promised
  // request does not have a body. Handlers should not modify Promise.
  //
  // Promise.RemoteAddr is the address of the server that started this push request.
  Promise *http.Request

  // Maybe? Or just have OriginalURL? Or remove entirely?
  // If we keep this, we'd probably need to deep-copy the original http.Request.
  //
  // OriginalRequest is the original client request that triggered the push.
  // Every PUSH_PROMISE must be sent in response to some client request,
  // so this is never nil.
  OriginalRequest *http.Request
}

// ReadResponse reads the pushed response. If ctx is done before the
// response headers are fully received, ReadResponse will fail.
func (pr *PushedRequest) ReadResponse(ctx context.Context) (*http.Response, error)
@rayvbr

This comment has been minimized.

Copy link

commented Mar 13, 2017

Looks good to me. If there's anything I can do to help this along, let me know.

@tombergan

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2017

Once @bradfitz is happy with the interface, we'd be happy for a volunteer to do the implementation. I won't have a chance to do this for a few weeks, at least, and I'm not sure if Brad will either.

@rayvbr

This comment has been minimized.

Copy link

commented Mar 13, 2017

Although I'm not sure whether my expertise of both the innards of x/net/http as well as the HTTP/2 spec is sufficient to bring this task to completion, with some guidance I'm definitely willing to give it a shot.

@snadrus

This comment has been minimized.

Copy link

commented Mar 30, 2017

I'd like this to be a way for grpc have a server-initiated rpc call for connected clients by using this with an http call for the response. Then grpc covers this significant use case that previously required websocket.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Mar 30, 2017

@snadrus, what does grpc have to do with PUSH_PROMISE? I see nothing in http://www.grpc.io/docs/guides/wire.html

@snadrus

This comment has been minimized.

Copy link

commented Mar 30, 2017

Nothing currently, however server-initiated rpc calls are missing from grpc, so enabling this through push frames may be an option. I'm investigating this with the grpc team.

@tombergan

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2017

@snadrus does your desired use in grpc require specific API features? If so, do you believe the API suggested above will work, or do you have other suggestions for an API?

At this point, we're more interested in API suggestions and critiques than "me toos" (we realize this is a desired feature!).

@romainmenke

This comment has been minimized.

Copy link

commented Apr 24, 2017

@glasser For a proxy it might be interesting to look at the Cloudflare approach : CF Push Blog

I wrote an http.Handler wrapper to enable this, it has been tested extensively but we aren't using this in production yet.
github.com/romainmenke/pusher/tree/master/link

I would also be very interested in helping out in any way I can with a client implementation as a push capable client would be a great help when writing tests for pushes from a server.

@ajackson-cpi

This comment has been minimized.

Copy link

commented Apr 24, 2017

@tombergan Yes, this works for the use I brought up.

@captncraig

This comment has been minimized.

Copy link

commented May 15, 2017

I would like client support for push in order to write automated verification tests for our http2 infrastructure. I am not very familiar with the mechanics of push, but I am really mostly interested in knowing that the pushes are initiated properly. I don't even need to read them, although that would be bonus.

@twdkeule

This comment has been minimized.

Copy link

commented Jul 31, 2017

Is there a use-case where we need the OriginalRequest? Because in normal operation the client does not really care where the resource comes from.

Would ReadResponse actually make the flow window decrease (reading from the stream) or would there be some kind of buffering so the connection does not become flow limited if no one is reading this pushed resource?

@romainmenke

This comment has been minimized.

Copy link

commented Jul 31, 2017

Maybe it would be best to also use context.Context instead http.Request as a way to identify what triggered the push, just to avoid implementations like this : #20948

I already opened an issue to add this information on the server side : #20566

@bmcstdio

This comment has been minimized.

Copy link

commented Aug 2, 2017

Is there any chance that this will make it to Go 1.9?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 2, 2017

@brunomcustodio, zero chance. There's not even code, and also https://golang.org/wiki/Go-Release-Cycle

@twdkeule

This comment has been minimized.

Copy link

commented Nov 12, 2017

Any update on this?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 13, 2017

Nope.

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11, Unplanned Nov 13, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Dec 28, 2017

Change https://golang.org/cl/85577 mentions this issue: http2: support consuming PUSH_PROMISE streams in the client

@meirf

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2018

@nilslice @romainmenke looks like y'all have h2 push implementation experience. Any interest in giving feedback on https://golang.org/cl/85577? Even partial feedback/minor comments would be helpful.

@himulawang

This comment has been minimized.

Copy link

commented Aug 11, 2018

Any update? We really need this feature.

@agnivade

This comment has been minimized.

Copy link
Member

commented Aug 11, 2018

@the729

This comment has been minimized.

Copy link

commented May 25, 2019

@meirf I see the review marathon on your https://golang.org/cl/85577. Are you still interested in working on it? I'm happy to help implementing/merging/working on this issue.
Do you have a public repo that contains your patch commits?

@the729

This comment has been minimized.

Copy link

commented May 25, 2019

@meirf I also feel this is a change too large to be merged once. Probably split into Framer + Transport. It seems the Transport API is still experimental, but changes to Framer is far less controversial.

the729 pushed a commit to HuanTeng/golang-x-net that referenced this issue May 26, 2019

http2: support consuming PUSH_PROMISE streams in the client
Introduces an opt-in API on the client which enables push
(SETTINGS_ENABLE_PUSH), and handles PUSH_PROMISEs and the
promised responses.

According to the h2 spec, HEADERS and PUSH_PROMISE frames
can be continued with CONTINUATION frames, but x/net/http2 only
supports HEADERS+CONTINUATIONs, which are merged/decoded into
MetaHeadersFrame. This adds support for PUSH_PROMISE+CONTINUATIONs,
which are merged/decoded into MetaPushPromiseFrame.

Fixes golang/go#18594

Change-Id: I1711e7bc0382f1eb1320eb000ec8575f74d5ac32
@meirf

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

@the729

Are you still interested in working on it?

I don't have the bandwidth to work on this for the foreseeable future. I welcome someone to take it over. (My only request is that the original CL url https://golang.org/cl/85577 be included in the commit message of new CLs.)

I also feel this is a change too large to be merged once. Probably split into Framer + Transport. It seems the Transport API is still experimental, but changes to Framer is far less controversial.

I agree that splitting it up starting with the less controversial changes is a great strategy and probably what I should have done. 👍

@gopherbot

This comment has been minimized.

Copy link

commented Jun 10, 2019

Change https://golang.org/cl/181497 mentions this issue: http2: support consuming PUSH_PROMISE streams in the client

@the729

This comment has been minimized.

Copy link

commented Jul 12, 2019

I will take it over from @meirf to work on this issue, and I have created a new CL https://golang.org/cl/181497.

Since @bradfitz is on leave recently, is there anyone else interested in reviewing this CL?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.