Go: net/http: add context.Context to PushOptions

[romainmenke]

In some situations I abuse the http.Header from http.PushOptions to pass contextual information from the original request to the pushed request. This feels wrong.

I think context.Context would be great here :

At Google, we require that Go programmers pass a Context parameter as the first argument to every function on the call path between incoming and outgoing requests. This allows Go code developed by many different teams to interoperate well. It provides simple control over timeouts and cancelation and ensures that critical values like security credentials transit Go programs properly.

I believe it is a really powerful way to manage passing values, so I was wondering if http.PushOptions could be extended to have a ctx field? I think this could mirror http.Request. The context added to http.PushOptions would then be accessible through request.Context().

A possible pitfall would be if someone added the original request context to http.PushOptions without fully understanding the consequences (deadlines, cancels,...)

// PushOptions describes options for Pusher.Push.
type PushOptions struct {
    // Method specifies the HTTP method for the promised request.
    // If set, it must be "GET" or "HEAD". Empty means "GET".
    Method string

    // Header specifies additional promised request headers. This cannot
    // include HTTP/2 pseudo header fields like ":path" and ":scheme",
    // which will be added automatically.
    Header Header

    ctx context.Context
}

func (p *PushOptions) Context() context.Context {
    if p.ctx != nil {
        return p.ctx
    }
    return context.Background()
}

func (p *PushOptions) WithContext(ctx context.Context) *PushOptions {
    if ctx == nil {
        panic("nil context")
    }
    p2 := new(PushOptions)
    *p2 = *p
    p2.ctx = ctx
    return p2
}

[rsc]

/cc @tombergan

[tombergan]

In some situations I abuse the http.Header from http.PushOptions to pass contextual information from the original request to the pushed request.

Can you give examples of the kind of information you want to pass? Your request seems reasonable at first glance, but it would help to see concrete uses before deciding on the right API.

A possible pitfall would be if someone added the original request context to http.PushOptions without fully understanding the consequences (deadlines, cancels,...)

Yes, that is my concern. What does it mean for the ctx to have a deadline? Moreover, each Push call generates an internal request that is dispatched to the server's ServeHTTP handler. That handler generates a request context by default, where req.Context() is scoped to the lifetime of the handler. How would PushOptions.ctx be merged with req.Context?

If we're going to use contexts for this, I prefer an API more like the following, since it's harder to accidentally mess up and it's more obvious how each context is scoped:

type PushOptions struct {
  Method string
  Header Header

  // UpdateRequestContext updates the context used by the pushed http.Request.
  // This hook can be used to pass information from the pusher to the pushee.
  UpdateRequestContext func(context.Context) context.Context
}

Also see this comment.

[romainmenke]

In the original request header values like referer give an indication of why a certain request comes in. This information is missing in a synthetic request. So for logging/monitoring purposses I pass on the path of the original request.

To prevent executing "pre-push" logic I add a flag to indicate that it is a synthetic request because recursive pushes are not supported.

It is not my intention to make the synthetic request in any way dependant on the original request in an http.Handler. Which is why I fully agree with your suggested API. It discourages tight coupling between the two.

[tombergan]

for logging/monitoring purposses I pass on the path of the original request.
to prevent executing "pre-push" logic I add a flag to indicate that it is a synthetic request

Enough people will want these things that I wonder if we should bake this in somehow, although I'm not sure how to do that. Probably the UpdateRequestContext API is simple enough to use that we won't need to bake anything in.

In any case, we won't get to this until Go 1.10. We should do this in parallel with #18997 so we settle on consistent terminology.

/cc @bradfitz

[rsc]

Accepting pending details to be worked out by @tombergan, @bradfitz, etc including checks against #18997.

[romainmenke]

Awesome to hear! Thank you for the very fast response and decision on this. Can't wait for Go1.10

[romainmenke]

Bump to Go1.11 ?

[odeke-em]

How's it going @romainmenke? Might you be interested in making a CL for Go1.11?

[romainmenke]

@odeke-em too late for Go1.11 now and still waiting for #18997.

[dunglas]

Another use case that may require to add contextual information from the main request to pushed requests is "recursive" pushes. By the spec: The server SHOULD send PUSH_PROMISE (Section 6.6) frames prior to sending any frames that reference the promised responses..

A common trick is to wait for all (recursive) pushes promises to be sent before starting to send the body of the main request. For instance, it's what Hades does: https://github.com/gabesullice/hades/blob/master/lib/server/pusher.go

Currently, the only way to do that is to assign an unique ID as a HTTP header, and to retrieve the corresponding Pusher instance associated with the main request using this header: https://github.com/gabesullice/hades/blob/master/lib/server/server.go#L39

Having a dedicated API such as the proposed one would be cleaner, and would prevent disclosing internal implementation details to the clients.