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

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

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

deltamualpha opened this issue Jun 16, 2022 · 7 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@deltamualpha
Copy link

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 Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 16, 2022
@seankhliao
Copy link
Member

cc @neild

@gopherbot
Copy link

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

@neild
Copy link
Contributor

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

Thanks for the quick turnaround!

@silencev
Copy link

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
Member

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

@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?

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 15, 2022
@dmitshur dmitshur added this to the Go1.20 milestone Aug 15, 2022
@golang golang locked and limited conversation to collaborators Aug 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants