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 · 9 comments

Comments

Projects
None yet
6 participants
@romainmenke

romainmenke commented Jun 3, 2017

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 from Proposal : Add Context to http.Pusher interface to proposal : Add Context to http.Pusher interface Jun 3, 2017

@romainmenke romainmenke changed the title from proposal : Add Context to http.Pusher interface to proposal : add context.Context to http.PushOptions Jun 3, 2017

@rsc rsc changed the title from proposal : add context.Context to http.PushOptions to proposal: net/http: add context.Context to PushOptions Jun 5, 2017

@rsc

This comment has been minimized.

Contributor

rsc commented Jun 5, 2017

@gopherbot gopherbot added this to the Proposal milestone Jun 5, 2017

@gopherbot gopherbot added the Proposal label Jun 5, 2017

@tombergan

This comment has been minimized.

Contributor

tombergan commented Jun 5, 2017

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

This comment has been minimized.

romainmenke commented Jun 6, 2017

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

This comment has been minimized.

Contributor

tombergan commented Jun 6, 2017

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

This comment has been minimized.

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 from proposal: net/http: add context.Context to PushOptions to net/http: add context.Context to PushOptions Jun 12, 2017

@romainmenke

This comment has been minimized.

romainmenke commented Jun 12, 2017

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

@romainmenke

This comment has been minimized.

romainmenke commented Nov 3, 2017

Bump to Go1.11 ?

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Nov 3, 2017

@odeke-em

This comment has been minimized.

Member

odeke-em commented Apr 24, 2018

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

@romainmenke

This comment has been minimized.

romainmenke commented May 2, 2018

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

@bradfitz bradfitz modified the milestones: Go1.11, Unplanned Jun 5, 2018

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