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: Request.WithContext should deep copy all pointer/map fields #23544

Closed
taralx opened this issue Jan 24, 2018 · 12 comments
Closed

net/http: Request.WithContext should deep copy all pointer/map fields #23544

taralx opened this issue Jan 24, 2018 · 12 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@taralx
Copy link

taralx commented Jan 24, 2018

Right now WithContext deep-copies URL only. At minimum, it should also deep-copy Headers for the same reason. For some reason the code distinguishes URL because it is "not a map", but maps are also pointer types.

@paranoiacblack
Copy link
Contributor

Related to #20068, which added URL deep-copying.

@bradfitz bradfitz added this to the Go1.11 milestone Jan 25, 2018
@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 25, 2018
@bradfitz
Copy link
Contributor

Probably, but this is also getting a little sad in terms of copies/allocs.

It might be time for a new NewRequest constructor variant (NewRequestContext?) that takes a context. Currently all code that wants to make a request with contexts has to make their Request, and then copy/alloc everything again, just to set the context.

Unfortunately the name NewRequestContext isn't exactly beautiful.

@taralx
Copy link
Author

taralx commented Jan 25, 2018

Alternatively we could export the context. There's nothing special about it compared to the other mutable parts IMO.

Then add Request.Clone() and have proper separation of concerns.

@bradfitz
Copy link
Contributor

Alternatively we could export the context. There's nothing special about it compared to the other mutable parts IMO.

It was an explicit decision to make it unexported, so people don't shoot themselves in the foot. Russ described the problem in the original bug(s).

@tombergan
Copy link
Contributor

I'm probably alone here: I am strongly opposed to turning WithContext into Clone. The vast majority of the time that I call WithContext, I don't need a deepy copy. If a deep copies are frequently needed, let's just add Request.Clone.

I also think https://golang.org/cl/41308 was a mistake. The CL description says "server.ServeHTTP mutates the request's URL", but net/http's server does not mutate Request.URL AFAIK. The original bug (#20068) is about net/http/httputil/reverseproxy and I think the change should have been restricted to that package.

@spf13
Copy link
Contributor

spf13 commented Mar 26, 2018

ping @bradfitz @tombergan for decision

@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 Jul 9, 2018
@bradfitz
Copy link
Contributor

bradfitz commented Jul 9, 2018

Punting to Go 1.12. But I think we do probably want to add a Clone and a new Context-accepting NewRequest. And maybe revert CL 41308.

But maybe this is all moot if we end up making a new http client interface. (#23707).

@odeke-em
Copy link
Member

I am checking in here to remind myself to be involved with the decision to rollback CL 41308 which I authored but also to be involved in this conversation, for Go1.12

@bradfitz bradfitz self-assigned this Nov 14, 2018
@rsc
Copy link
Contributor

rsc commented Nov 28, 2018

Punting to Go 1.13.

@rsc rsc added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Nov 28, 2018
@rsc rsc modified the milestones: Go1.12, Go1.13 Nov 28, 2018
@gopherbot
Copy link

Change https://golang.org/cl/174324 mentions this issue: net/http: add func NewRequestWithContext

@drags
Copy link

drags commented Dec 11, 2019

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

@bradfitz
Copy link
Contributor

WithContext works there.

@golang golang locked and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

9 participants