go
go copied to clipboard
golang
x/net/http2: support consuming PUSH_PROMISE streams in the client
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.
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?
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.
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.
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?
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)
Looks good to me. If there's anything I can do to help this along, let me know.
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.
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.
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.
@snadrus, what does grpc have to do with PUSH_PROMISE? I see nothing in http://www.grpc.io/docs/guides/wire.html
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.
@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!).
@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.
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.
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?
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
@brunomcustodio, zero chance. There's not even code, and also https://golang.org/wiki/Go-Release-Cycle
Change https://golang.org/cl/85577 mentions this issue: http2: support consuming PUSH_PROMISE streams in the client
@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 - Please take a look at https://github.com/golang/go/wiki/NoPlusOne.
@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?
@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
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. 👍
Change https://golang.org/cl/181497 mentions this issue: http2: support consuming PUSH_PROMISE streams in the client
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?