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

net/http: add context.Context to PushOptions #20566

Open
romainmenke opened this issue Jun 3, 2017 · 10 comments
Open

net/http: add context.Context to PushOptions #20566

romainmenke opened this issue Jun 3, 2017 · 10 comments

Comments

@romainmenke
Copy link

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
}
@romainmenke romainmenke changed the title Proposal : Add Context to http.Pusher interface proposal : Add Context to http.Pusher interface Jun 3, 2017
@romainmenke romainmenke changed the title proposal : Add Context to http.Pusher interface proposal : add context.Context to http.PushOptions Jun 3, 2017
@rsc rsc changed the title proposal : add context.Context to http.PushOptions proposal: net/http: add context.Context to PushOptions Jun 5, 2017
@rsc
Copy link
Contributor

rsc commented Jun 5, 2017

/cc @tombergan

@gopherbot gopherbot added this to the Proposal milestone Jun 5, 2017
@tombergan
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

rsc commented Jun 12, 2017

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

@rsc rsc modified the milestones: Go1.10, Proposal Jun 12, 2017
@rsc rsc changed the title proposal: net/http: add context.Context to PushOptions net/http: add context.Context to PushOptions Jun 12, 2017
@romainmenke
Copy link
Author

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

@romainmenke
Copy link
Author

Bump to Go1.11 ?

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Nov 3, 2017
@odeke-em
Copy link
Member

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

@romainmenke
Copy link
Author

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

@bradfitz bradfitz modified the milestones: Go1.11, Unplanned Jun 5, 2018
@dunglas
Copy link
Contributor

dunglas commented Sep 6, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants