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: clarify use-cases of WithContext vs Clone on requests #53413

Open
deltamualpha opened this issue Jun 16, 2022 · 7 comments
Open

net/http: clarify use-cases of WithContext vs Clone on requests #53413

deltamualpha opened this issue Jun 16, 2022 · 7 comments
Labels
Documentation NeedsInvestigation

Comments

@deltamualpha
Copy link

@deltamualpha deltamualpha commented Jun 16, 2022

The http.Request struct had a Clone method added in go 1.13, prompted by this issue: #23544. At that time, I believe the documentation for WithContext was updated to suggest that most uses of that method could be replaced with Clone.

However, that issue also had this follow-up exchange: #23544 (comment)

@bradfitz I just came across Clone as a first time user of contexts within net/http. I had a question about this bit of documentation on WithContext:

...To change the context of a request (such as an incoming) you then also want to modify to send back out, use Request.Clone. Between those two uses, it's rare to need WithContext.

I wanted to ask if Clone should be preferred over WithContext in all situations. Specifically I'm writing a server middleware and only need to annotate a request (adding a logged in user) so it can be used by other handlers.

Is WithContext not appropriate for this situation?

And @bradfitz responded:

WithContext works there.

And this has come up occasionally in Slack as well, with people writing very simple middleware that wrap http.Handler and just stick a value into the context on the request using WithContext. An extremely simple version, for which Clone seems like overkill, might be:

func WithRequestId(base http.Handler) http.Handler {
   return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
      ctx := context.WithValue(r.Context(), "request_id", uuid.NewString())
      base.ServeHTTP(w, r.WithContext(ctx))
   })
}

So this issue is mostly asking: is this usage of WithContext in this sort of middleware context correct, and furthermore can we augment the WithContext documentation block to be less prescriptive that Clone is the obvious way to go, and that there are plenty of value use-cases for WithContext, especially when you're not looking to "reuse" the request to make a subsequent request?

@seankhliao seankhliao added Documentation NeedsInvestigation labels Jun 16, 2022
@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Jun 16, 2022

cc @neild

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 16, 2022

Change https://go.dev/cl/412778 mentions this issue: net/http: make Request.WithContext documentation less prescriptive

@neild
Copy link
Contributor

@neild neild commented Jun 16, 2022

I don't see anything wrong with that usage of Request.WithContext. I just mailed CL 413778 to update the docs.

@deltamualpha
Copy link
Author

@deltamualpha deltamualpha commented Jun 16, 2022

Thanks for the quick turnaround!

@silencev
Copy link

@silencev silencev commented Jun 22, 2022

I'm curious why WithContext cannot simply set the ctx to current Request but need to shallow copy Request?
what's the purpose of shallow copy?

@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Jun 22, 2022

for the same reason it is unexported:

// It is unexported to prevent people from using Context wrong
// and mutating the contexts held by callers of the same request.

@silencev
Copy link

@silencev silencev commented Jun 23, 2022

@seankhliao thanks for your comment.
I am still confused about the "using Context wrong". Any bad consequence if it's exported field to let call set?

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

No branches or pull requests

5 participants