-
Notifications
You must be signed in to change notification settings - Fork 17.4k
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: delete inappropriate headers in func Error #66343
Comments
It is not always wrong, but it can be wrong. In my own use case, I set Another perspective on this is that etag and last-modified is about content to be served or returned, but
I am OK with that.
One approach done in one iteration of past CLs was to extend |
That would mean that if, say, a library wants to call
This is a breaking change, since today I can do: var f func(http.ResponseWriter, string, int) = http.Error |
True. That was an internal error function used just by Maybe one solution could be that External callers who want to remove My personal take is that I like current proposal that |
The part "depending on the status code semantics" suggests that whether it is correct to delete these depends on which status code is being returned. However, I wasn't able to determine which part of this spec (or any other spec) explicitly describes "the status code semantics" for the purpose of this rule. Some of the status codes are described as "cacheable", which seems at least plausibly related to whether the response payload is "the selected representation", because otherwise it would not make sense to use With all of that said, it does seem like unsetting them is a good conservative default. It seems to me that doing anything "better" than that would require figuring out which status codes the above rule applies to and making |
(Apologies for not being around to deal with rollbacks; four years of dodging COVID-19 have come to an end, and I was out sick last week.)
I think, therefore, that we should err on the side of preserving the current behavior, except in cases where the current behavior is objectively wrong in all cases. I'd really like to avoid introducing a
|
I hope it was not a rough case.
Yes, unless My first CL to fix Maybe we should reintroduce this new "error" function which does more header cleanup and call it from standard library (at least from The question might then be: should this new "error" function be made public or should it be just internal? In a way, it is easy also for others to wrap |
I agree that we should add an internal error function for |
I also noticed in our own code, that we do set func Error(w http.ResponseWriter, _ *http.Request, code int) {
body := http.StatusText(code)
w.Header().Set("Cache-Control", "no-cache")
// http.Error appends a newline so we have to add 1.
w.Header().Set("Content-Length", strconv.Itoa(len(body)+1))
http.Error(w, body, code)
} So not sure even about removing |
Removing Content-Length is at best important and at worst harmless, since the protocol will figure out the content length either way. The GODEBUG was only meant as a transitional mechanism so that people control when the change happens, not a long-term knob. If we did the extra deletions, code that wanted to send those headers would inline the pieces they wanted of http.Error, not set the GODEBUG. That said I am also happy to keep deleting Content-Length, delete Content-Encoding, and stop there. |
This proposal has been added to the active column of the proposals project |
Following @neild's suggestion, I revise this proposal to:
and nothing else. |
But we can still delete the other headers in the response from |
@mitar what specific headers would ServeContent delete before calling Error? From https://pkg.go.dev/net/http#ServeContent it looks like just Etag? |
@rsc: |
Having ServeContent delete Etag, Last-Modified, and Cache-Control on its error path sounds fine. |
Great. What about setting |
Based on the discussion above, this proposal seems like a likely accept. The proposal is that http.Error will delete these headers:
and http.ServeContent will delete these headers before calling http.Error:
And these would be documented in the doc comments for these functions. |
I have a CL pending with a GODEBUG. |
No change in consensus, so accepted. 🎉 The proposal is that http.Error will delete these headers:
and http.ServeContent will delete these headers before calling http.Error:
And these would be documented in the doc comments for these functions. |
The GODEBUG I had was mainly for the more invasive behavior. I think we can drop the GODEBUG for this limited set. I will do that for now and add one later if we need it. |
@rsc I agree. Will you also add removing of headers to http.ServeContent errors? Or should that be done in a followup CL? |
I have it in the same CL. |
Change https://go.dev/cl/571995 mentions this issue: |
@rsc: Thank you for taking this on and making it happen! |
I've received one report of this change causing breakage in code which adds gzip compression in a middleware layer. The middleware sets I think this middleware should be using Perhaps this is an indication that we should revert the change to delete |
If I understand this correctly, it seems like the That said, this is evidently going to cause subtle breakage, so I think we need to roll back the |
I would just ask that if you do remove deleting I do see that |
I'm sorry, I can't parse that sentence. Can you rephrase? Thanks. |
I believe that's: Even if we stop deleting I tentatively agree with this. |
@neild Thanks for improving my language. Yes, that is what I meant. |
This is marked as a release blocker, is open, is not marked okay-after-RC1, and the scheduled cut for 1.23RC1 is roughly 24 hours from now. Where do we stand on this? |
To summarize:
Options:
|
I think we should keep the whole thing and add a GODEBUG. The GODEBUG creates a transition to a world where we can remove Content-Encoding unconditionally and the GODEBUG is gone. The GODEBUG also means that anyone who is broken can run bisect -godebug to find the exact stack trace that leads to the header change and is causing the problem. And we think the scale of breakage is likely to be small, which is exactly when a GODEBUG is appropriate. |
Change https://go.dev/cl/593157 mentions this issue: |
After some discussion, we're going to keep the change to Middleware which sets (The correct way to write middleware which does on-the-fly compression is to set It is valid to set However, in the case of |
Change https://go.dev/cl/593775 mentions this issue: |
The pre-Go 1.23 behavior is httpservecontentkeepheaders=1. For #66343 Change-Id: If6f92853b38522f19a8908ff11ac49b12f3dc3e0 Reviewed-on: https://go-review.googlesource.com/c/go/+/593775 Reviewed-by: David Chase <[email protected]> Auto-Submit: Damien Neil <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
Change https://go.dev/cl/593795 mentions this issue: |
…econtentkeepheaders The pre-Go 1.23 behavior is httpservecontentkeepheaders=1. For #66343 Change-Id: If6f92853b38522f19a8908ff11ac49b12f3dc3e0 Reviewed-on: https://go-review.googlesource.com/c/go/+/593775 Reviewed-by: David Chase <[email protected]> Auto-Submit: Damien Neil <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-on: https://go-review.googlesource.com/c/go/+/593795 Reviewed-by: Damien Neil <[email protected]> Reviewed-by: Michael Pratt <[email protected]>
This breaks the Go stdlib's http.Client which only expects Separately there are issues with http.Server as well. If you set a What I'm getting at is there are several spots in the http that don't support |
The pre-Go 1.23 behavior is httpservecontentkeepheaders=1. For golang#66343 Change-Id: If6f92853b38522f19a8908ff11ac49b12f3dc3e0 Reviewed-on: https://go-review.googlesource.com/c/go/+/593775 Reviewed-by: David Chase <[email protected]> Auto-Submit: Damien Neil <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
#50905 reported a bug about ServerContent serving bad headers when the request range is invalid. An example usage is:
In this case, if http.ServeContent calls http.Error, it already forces Content-Type back to text/plain, but it leaves the Content-Length and Etag headers in place. This ends up being wrong for the error, though they would have been correct for a successful response.
CL 554216 added a deletion of Content-Length in http.Error. That's obviously correct, since Error is writing the content: the caller cannot predict how large it is. There was no problem with that CL.
CL 544109 did more: it deletes Content-Encoding, Etag, and Last-Modified, and it forces Cache-Control to no-cache.
Deleting Content-Encoding is correct for the same reasons as Content-Length.
I am not convinced that deleting Etag and Last-Modified is correct. Those describe the resource at the URL, and it seems sensible to me to say that a response can be an error that says “something about your request was invalid” but still also send back information about the underlying resource. It's not obviously wrong in the way that using the underlying resource's Content-Encoding or Content-Length is obviously wrong.
I am also not convinced that forcing Cache-Control to no-cache is correct. Error results can absolutely be cached. https://cloud.google.com/media-cdn/docs/caching, for example, describes how Google's CDN handles cached errors. An argument might be made that in situations like http.ServeContent, the caller can only set Cache-Control for the successful response, and so Error should assume that's not the expiry for an error. Perhaps that is true.
CL 544109 forced Cache-Control: no-cache instead of deleting the header. That broke many tests inside and outside Google.
CL 569815 changed the logic to only force Cache-Control to no-cache when it was already set to something else. This still breaks some tests inside Google; I am not sure about outside. It still seems incorrect to me. If we believe that the content being returned by Error is not accurately described by w.Header() on entry in certain ways, then why should these two code snippets produce different headers?
versus
I cannot justify why one error should have “Cache-Control: no-cache” while the other should have no header at all. Always or never is easier to justify than “depends on a header we already established shouldn't apply to the error response”.
I rolled back CL 544109 and CL 569815 because of the Cache-Control breakage, and I am making this a proposal because of the compatibility implications (we've already had many broken tests).
It seems plenty defensible to me to change Error to do:
I propose we do those. I don't expect any objections here but won't be surprised if I am missing something.
For Cache-Control specifically, it seems like there are four options:
I propose we do (4), because:
If people are in favor of that proposal, then the next question is whether to gate these with a GODEBUG setting. The first two in the list seem like obvious bug fixes, because they are about the actual content form, and that's the job of func Error; the caller does not control that content. The last two are less defensible, because they are about the semantics of the response, and the caller may well want to be describing the error by setting those headers. For example consider code that tries to create a cacheable error today:
Perhaps we should have a GODEBUG that controls the last three deletions?
Let's say httpcleanerrorheaders=0 preserves Etag, Last-Modified, and Cache-Control.
Then the complete proposal is
The text was updated successfully, but these errors were encountered: