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
Comments
/cc @tombergan |
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.
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. |
In the original request header values like 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 |
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 |
Accepting pending details to be worked out by @tombergan, @bradfitz, etc including checks against #18997. |
Awesome to hear! Thank you for the very fast response and decision on this. Can't wait for Go1.10 |
Bump to Go1.11 ? |
How's it going @romainmenke? Might you be interested in making a CL for Go1.11? |
Another use case that may require to add contextual information from the main request to pushed requests is "recursive" pushes. By the spec: 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. |
In some situations I abuse the
http.Header
fromhttp.PushOptions
to pass contextual information from the original request to the pushed request. This feels wrong.I think context.Context would be great here :
I believe it is a really powerful way to manage passing values, so I was wondering if
http.PushOptions
could be extended to have actx
field? I think this could mirrorhttp.Request
. The context added tohttp.PushOptions
would then be accessible throughrequest.Context()
.A possible pitfall would be if someone added the original request context to
http.PushOptions
without fully understanding the consequences (deadlines, cancels,...)The text was updated successfully, but these errors were encountered: