x/net/http2: add push_promise frame support
bradfitz opened this issue · 28 comments
This bug is about http2 support for PUSH_PROMISE frames, letting http.Handlers push their own streams to clients.
Previously:
bradfitz/http2#23
bradfitz/http2#34
Let's do this first in x/net/http2 and only add API to the standard library after it's baked long enough in x/net.
@bradfitz thanks for the cc. I'm actually interested in being able to create streams in the client (e.g. something like c := conn.Hijack(); s := c.createStream(...). I don't want to steal the purpose of this issue - should we create a new one for that?
Sure, create a new bug (I think). I don't exactly understand.
I think the Pusher interface way of doing this that I put in the first CL is good and we can probably reuse it.
Do we want to do anything smart with the headers we send with the push_promise? Things like cookies and which type of element the resource appears in, changes how browsers request it.
One motivator for integrating Push into http.Handler is that the draft Web Push protocol uses it.
Go is generally a great candidate for writing these new standards-based push backends and I'm already considering it for one open source real-time project in centrifugal/centrifugo#62.
I imagine (but have not yet got past reading docs/source) that the support for the frames in http2 would make it possible to manually implement by hijacking the http conn, but this is a much higher barrier to writing a conforming service/library.
Just found this issue. This is a feature I would love to see make it into x/net/http2. I recently needed to do some local testing with HTTP/2 push, so I implemented push using a design that happened to be roughly similar to bradfitz/http2#23, but instead of adding a Push method to the H2 ResponseWriter, I added an H2Stream method to the ResponseWriter that returns a handle that exposes various H2-specific features:
func (*h2responseWriter) H2Stream() H2Stream { ... }
type H2Stream struct { ... }
// Push sends a push promise frame and dispatches a pseudo-request to the Server's handler.
// Note that method must be GET or HEAD according to the spec (see Section 8.2).
// Returns the StreamID of the pushed response.
func (H2Stream) Push(method, url string, h http.Header) (uint32, error) { ... }
// StreamID returns the unique identifier of this stream, per spec Section 5.1.1.
func (H2Stream) StreamID() uint32 { ... }
// Other methods could be added, such as:
func (H2Stream) SetPriority(dependency uint32, weight uint8) error { ... }If adding a method to the ResponseWriter is too magical, another design is to add an H2-specific handler function to the http2.Server object where the signature is:
package http2
type Server struct {
...
// if not specified, defaults to a wrapper around the http.Server.Handler passed to ConfigureServer
Handler func(H2Stream, ResponseWriter, *Request)
}Implementation note: I believe the code at bradfitz/http2#23 misses a subtlety in the spec. Section 5.1.1 says "The identifier of a newly established stream MUST be numerically greater than all streams that the initiating endpoint has opened or reserved." This implies that push promise frames MUST be written to the wire in promised-id-order -- this requires some changes to http2/writesched.go.
I'm happy to do the leg work of the implementation, if needed, once we reach consensus on a design.
Do we want to do anything smart with the headers we send with the push_promise? Things like cookies and which type of element the resource appears in, changes how browsers request it.
My opinion is an emphatic no. If the caller does not specify any request headers, the push promise frame should not include any headers (other than the standard psuedo-headers like ":path", etc.). For example, it's impossible in general to know if the cookies on the initial request apply to the specific path of the url being pushed -- this is something that will be specific to each cookie and each server.
Milestone?
Setting to 1.8 to see if somebody wants to work on it.
I told Brad I'd do this after Go 1.7, so you can assign this to me (looks like I can't assign things to myself).
CL https://golang.org/cl/29439 mentions this issue.
@bradfitz and I were kicking around options for how the API would look. Ideally, we want something like the following:
package golang.org/x/net/http2
type PushOptions struct {
PromisedMethod string // defaults to GET
PromisedHeader http.Header
// ... might add other things later, like priorities, an option to suppress duplicate pushes, etc.
}
// The http2 ResponseWriter implements this interface. The push method
// sends a PUSH_PROMISE then creates a pseudo-request that is handled
// with the normal request handler.
type Pusher interface {
Push(url string, opts PushOptions) error
}
// golang.org/x/net/http2/server.go
func (w *responseWriter) Push(url string, opts PushOptions) error { ... }The trick is making this play nicely with the http2 code that is bundled into net/http. The current bundler imports http2 into net/http without exporting any types. Without PushOptions exported, the caller cannot define the Pusher interface, which means they cannot test if their ResponseWriter implements Pusher. One option is to modify the bundler tool to export PushOptions as net/http.PushOptions or similar. However, this may generate confusion since the caller would need to use different type names depending on whether they're using net/http.Server or x/net/http2.Server. This becomes a real annoyance if a request handler is defined in a library that is sometimes used by a net/http.Server and other times used by a x/net/http2.server.
A second option is to define these types directly in the net/http package:
package net/http
type PushOptions struct { ... }
type Pusher interface { Push(url string, opts PushOptions) error }
// golang.org/x/net/http2/server.go
func (w *responseWriter) Push(url string, opts http.PushOptions) error { ... }This is my favored option. If there are objections to adding new types to net/http, a third option avoids exporting any types:
package golang.org/x/net/http2
// These types are not exported when they're bundled into net/http.
type Pusher interface {
SetPromisedMethod(string)
SetPromisedHeader(http.Header)
Start() error
}
type CreatePusher interface {
Push(url string) interface{} // actually returns a Pusher
}Note that Push() actually returns a Pusher, but we can't write Push(url string) Pusher because that would force the caller to name the Pusher type, which they cannot do (at least not for the bundled version in net/http). The above API would be used like this:
// Can redefine these interfaces anywhere using a subset of the full list of methods.
type Pusher interface {
Start() error
}
type CreatePusher interface {
Push(url string) interface{}
}
func MyHandler(rw http.ResponseWriter, req *http.Request) {
// Push something. This code works the same if the handler is installed in either
// `golang.org/x/net/http2.Server` or `net/http`.
if p, ok := rw.(CreatePusher); ok {
p.Push(url).(Pusher).Start()
}Thoughts?
I'm fine with those two new types in net/http.
Should Push take a pointer *PushOptions instead, though, so the caller can write nil more easily?
SGTM.
CL https://golang.org/cl/32012 mentions this issue.
Plug: anyone interested in using this feature may be interested in reading a doc some of us at Google wrote about the challenges of using HTTP/2 server push: "Rules of Thumb for HTTP/2 Push".
Somehow this i broken again. When i used the golang HEAD from back in October, it works fine. Now when i run the same code with HEAD (ffedff7), i get this
2017/01/09 21:04:00 http2: panic serving ip:port: interface conversion: *http2.responseWriter is not http.Pusher: missing method Push
goroutine 10500 [running]:
golang.org/x/net/http2.(*serverConn).runHandler.func1(0xc421e6bfaf, 0xc424118fc0, 0xc420aa00e0)
/Users/fbettag/go-projects/wasted/go-proxy/src/golang.org/x/net/http2/server.go:1704 +0xba
panic(0x83f7e0, 0xc42409b3c0)
/usr/local/Cellar/go/HEAD-ffedff7_1/libexec/src/runtime/panic.go:489 +0x2cf
main.(*OurHttpReverseProxy).ServeHTTP(0xc421b2a8c0, 0xaa5c00, 0xc420aa00e0, 0xc4215424a0, 0xc423536300, 0xc421e6ba28, 0xc42022a960)
/Users/fbettag/go-projects/wasted/go-proxy/src/http_reverse_proxy.go:222 +0x921
main.(*HttpServer).handleProxyRequest(0xc420cd49c0, 0xaa5c00, 0xc420aa00e0, 0xc425d14b00)```
Also i cannot find a single line of code anywhere that gets me a http2.responseWriter.
@fbettag, your x/net/http2 is probably too old. Your Go HEAD version is mostly irrelevant, given the stack trace is showing you're using golang.org/x/net/http2 directly.
Uhm ok, so i have to decide which one i use? I just recompiled the code i had with latest HEAD and try to solve this for a few hours now.
You don't have to decide, but you may. If you do nothing, Go has HTTP/2 support by default, using a snapshot of x/net/http2 that we include once per release. If you want to use a newer or older version, you can import golang.org/x/net/http2 explicitly, which appears to be what you're doing, in which case your Go version is irrelevant, since you're not using Go's built-in http2, but you're using whatever you have on your disk. Which git version of of golang.org/x/net/http2 do you have?
commit f315505cf3349909cdf013ea56690da34e96a451
Author: Brad Fitzpatrick bradfitz@golang.org
Date: Fri Jan 22 02:14:55 2016 +0000
net/context/ctxhttp: fix case where Body could leak and not be closed
ah crap, i just saw that it's 2016. narf. yeah "glide" doesn't seem to update this one. sorry
seems to work now! thanks again and sorry for this!
If I understand correctly, Go 1.8 will have server-side support for HTTP/2 server push, thanks to the work done on this issue.
Is there any support for reacting to server push in the HTTP/2 client? I'm specifically curious about proxying server pushes with net/http/httputil's ReverseProxy. I don't see anything about it in the current code, but maybe I'm missing something? I imagine you would set some sort of field on http.Request with some interface that you use to handle incoming server pushes, and I don't see such a thing.
If this doesn't exist already, should I open an issue? Or maybe two issues — one for a client API, and one for ReverseProxy support? It also might be nice to be explicit in the docs that this is not yet supported.
There is no support for client transports receiving pushes and I don't see an open issue. Feel free to file a new one.