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: allow handlers to modify the http.Request #27277

Closed
nhooyr opened this issue Aug 27, 2018 · 4 comments
Closed

net/http: allow handlers to modify the http.Request #27277

nhooyr opened this issue Aug 27, 2018 · 4 comments
Labels
Documentation FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@nhooyr
Copy link
Contributor

nhooyr commented Aug 27, 2018

The docs say

Except for reading the body, handlers should not modify the provided Request.

At the moment, it seems like modifying the provided http request is safe and this guarantee is restrictive but not useful.

In particular, following this guarantee prevents the subrouting approach presented in https://blog.merovius.de/2017/06/18/how-not-to-use-an-http-router.html as the *http.Request's URL is modified when sending it to sub handlers so that they can route the remaining part of the URL on their own. The alternative would be to create a shallow copy of the request every time and then modifying the URL which can become expensive, as there would be a shallow copy for every single segment of the path.

@FiloSottile
Copy link
Contributor

@bradfitz can answer properly, but I believe the point is to avoid a situation where we can't look at the Request after it has been handled, and once we allow changes there's no going back. I find the performance argument against shallow copies weak, but I'll leave this for Brad.

@FiloSottile FiloSottile changed the title net/http: purpose of requiring that handlers not modify the http.Request? net/http: allow handlers to modify the http.Request Aug 30, 2018
@FiloSottile FiloSottile added Documentation NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Aug 30, 2018
@FiloSottile FiloSottile added this to the Go1.12 milestone Aug 30, 2018
@earthboundkid
Copy link
Contributor

@Merovius, what do you think about changing the blog post to use a shallow copy of the request?

@Merovius
Copy link
Contributor

A shallow copy wouldn't help (URL is a pointer). You would have to use a deep copy. Deep copies might not be too expensive to do on every request, but they are too expensive to do in every frame IMO. And personally, I think if Go ever starts relying on Request not being modified, tons and tons of Go code out there will break silently, for better or for worse (note, that the comment was only added in 2016). So, overall, I thought about this when writing the article but decided to simply not care. It might be worth mentioning the tradeoff though.

FWIW, there is another reason not to modify the URL, which is that you need the full path for relative redirects (blegh). I've been thinking about how to address that for a while. I don't have a really nice solution yet - personally, I use a modified ResponseWriter for redirects that takes this into account, but that comes with its own set of problems.

@rsc
Copy link
Contributor

rsc commented Sep 26, 2018

At the moment, it seems like modifying the provided http request is safe ...

I'm pretty sure that's not true. We only add these docs in response to real problems. At this point I don't think we can just change the semantics here - lots of real things will break, even if no real thing you use will break.

We clearly can't change this without breaking a lot of existing code.

@rsc rsc closed this as completed Sep 26, 2018
@golang golang locked and limited conversation to collaborators Sep 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation 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

6 participants