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 push_promise frame support #13443

Closed
bradfitz opened this issue Dec 1, 2015 · 28 comments
Closed

x/net/http2: add push_promise frame support #13443

bradfitz opened this issue Dec 1, 2015 · 28 comments
Assignees
Milestone

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 1, 2015

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

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Dec 1, 2015

@ncdc

This comment has been minimized.

Copy link

@ncdc ncdc commented Dec 1, 2015

@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?

cc @lavalamp @thockin @smarterclayton

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Dec 1, 2015

Sure, create a new bug (I think). I don't exactly understand.

@DanielMorsing

This comment has been minimized.

Copy link
Contributor

@DanielMorsing DanielMorsing commented Dec 1, 2015

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.

@rsc rsc added this to the Unreleased milestone Dec 28, 2015
@banks

This comment has been minimized.

Copy link

@banks banks commented Jan 13, 2016

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.

@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Feb 18, 2016

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.

@gaillard

This comment has been minimized.

Copy link

@gaillard gaillard commented May 27, 2016

Milestone?

@ianlancetaylor ianlancetaylor modified the milestones: Go1.8, Unreleased May 27, 2016
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 27, 2016

Setting to 1.8 to see if somebody wants to work on it.

@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented May 27, 2016

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).

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 20, 2016

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

@quentinmit quentinmit added the NeedsFix label Sep 29, 2016
@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Oct 25, 2016

@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?

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Oct 25, 2016

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?

@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Oct 25, 2016

SGTM.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 25, 2016

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

gopherbot pushed a commit that referenced this issue Oct 25, 2016
This interface will be implemented by golang.org/x/net/http2 in
https://go-review.googlesource.com/c/29439/.

Updates #13443

Change-Id: Ib6bdd403b0878cfe36fa9875c07c2c7239232556
Reviewed-on: https://go-review.googlesource.com/32012
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Oct 27, 2016

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".

@fbettag

This comment has been minimized.

Copy link

@fbettag fbettag commented Jan 9, 2017

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.
@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Jan 9, 2017

@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.

@fbettag

This comment has been minimized.

Copy link

@fbettag fbettag commented Jan 9, 2017

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.

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Jan 9, 2017

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?

@fbettag

This comment has been minimized.

Copy link

@fbettag fbettag commented Jan 9, 2017

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

@nathany

This comment has been minimized.

Copy link
Contributor

@nathany nathany commented Jan 9, 2017

@fbettag Try running go get -u golang.org/x/net/http2 and recompile.

@fbettag

This comment has been minimized.

Copy link

@fbettag fbettag commented Jan 9, 2017

ah crap, i just saw that it's 2016. narf. yeah "glide" doesn't seem to update this one. sorry

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Jan 9, 2017

@fbettag, yes, that's way too old. Either do what @nathany said, or don't use http2 explicitly and use the built-in http2 support.

@fbettag

This comment has been minimized.

Copy link

@fbettag fbettag commented Jan 9, 2017

seems to work now! thanks again and sorry for this!

@glasser

This comment has been minimized.

Copy link
Contributor

@glasser glasser commented Jan 10, 2017

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.

@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Jan 10, 2017

There is no support for client transports receiving pushes and I don't see an open issue. Feel free to file a new one.

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Jan 10, 2017

@glasser, no, there is no push support in Go's http2 client.

@glasser

This comment has been minimized.

Copy link
Contributor

@glasser glasser commented Jan 10, 2017

Thanks, #18594.

@golang golang locked and limited conversation to collaborators Jan 10, 2018
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
This makes x/net/http2's ResponseWriter implement the new interface,
http.Pusher. This new interface requires Go 1.8. When compiled against
older versions of Go, the ResponseWriter does not have a Push method.

Fixes golang/go#13443

Change-Id: I8486ffe4bb5562a94270ace21e90e8c9a4653da0
Reviewed-on: https://go-review.googlesource.com/29439
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.