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
Assignees
Milestone

Comments

@taralx
Copy link

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

This comment has been minimized.

Copy link
Contributor

@paranoiacblack paranoiacblack commented Jan 24, 2018

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

@bradfitz bradfitz added this to the Go1.11 milestone Jan 25, 2018
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jan 25, 2018

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jan 25, 2018

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

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Jan 25, 2018

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

This comment has been minimized.

Copy link
Contributor

@spf13 spf13 commented Mar 26, 2018

ping @bradfitz @tombergan for decision

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Jul 12, 2018

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

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 28, 2018

Punting to Go 1.13.

@rsc rsc added the early-in-cycle label Nov 28, 2018
@rsc rsc modified the milestones: Go1.12, Go1.13 Nov 28, 2018
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 29, 2019

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

@drags

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 12, 2019

WithContext works there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants
You can’t perform that action at this time.