Comments (15)
ETag
and Last-Modified
are Validator Header Fields, for which there is some specific language in the RFC relating to this question:
Note that, depending on the status code semantics, the selected representation for a given response is not necessarily the same as the representation enclosed as response payload.
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 If-None-Match
/If-Modified-Since
/etc to check if that cached representation is still current, but I can't find an explicit connection between those two ideas in the specification text.
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 Error
behave differently depending on the status code, which seems unnecessarily complicated and perhaps a little too clever/surprising.
from go.
from go.
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.
It is not always wrong, but it can be wrong. In my own use case, I set immutable
caching header with etag for the contents I am planing to serve (which I have in metadata in db) and then I go to serve the file. If that file serving fails for some reason (Read
returns an error, or if range request is invalid, etc.) an error is returned. That error might be permanent or transient. For transient errors those caching headers and etag and last-modified should be removed. But it is hard to know if it is transient, so removing those headers is reasonable in my view.
Another perspective on this is that etag and last-modified is about content to be served or returned, but Error
serves some completely different content. So they are probably misleading.
I propose we do (4), because:
I am OK with that.
GODEBUG=httpcleanerrorheaders=0 disables the Etag, Last-Modified, and Cache-Control deletions
One approach done in one iteration of past CLs was to extend Error
with varargs list of headers to keep. So by default a set of headers is deleted, but one can pass Etag
string as the varag argument to make sure it is kept. httpcleanerrorheaders
could still control the default list of headers to keep.
from go.
Perhaps we should have a GODEBUG that controls the last three deletions?
Let's say httpcleanerrorheaders=0 preserves Etag, Last-Modified, and Cache-Control.
That would mean that if, say, a library wants to call http.Error
with Cache-Control
showing up in the response, they now have to require all consumers to set httpcleanerrorheaders=0
instead of it "just working".
One approach done in one iteration of past CLs was to extend Error with varargs list of headers to keep.
This is a breaking change, since today I can do:
var f func(http.ResponseWriter, string, int) = http.Error
from go.
This is a breaking change, since today I can do:
True. That was an internal error function used just by ServeContent
.
Maybe one solution could be that http.Error
remove all headers listed above except for Cache-Control
and that ServeContent
additionally removes Cache-Control
on errors (by using a small internal wrapper around http.Error
).
External callers who want to remove Cache-Control
as well on errors, could use a similar wrapper they make, or we could make some additional http.ErrorBis
or something which does that as well and users can slowly migrate to it if they want. But the issue is that this should be then also called from most other places in stdlib and elsewhere.
My personal take is that I like current proposal that http.Error
does remove all those headers because in almost all cases this is what improves the response and httpcleanerrorheaders
seems like a good alternative to switch this off.
from go.
(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.)
Error
can't be all things to all users: It's reasonable to send an error with a Cache-Control
header. It's also reasonable to want to send an error with no Cache-Control
. We can satisfy some users by clearing Cache-Control
and others by not, but either way someone is going to get behavior they don't want.
Error
is also a very simple function; it isn't difficult for users to write their own version that does exactly what they want.
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 GODEBUG
and the concomitant cognitive complexity. "Error
does this simple thing, which might not be perfect in all circumstances" is better than "Error
does this or that depending on a global variable".
Content-Type
: Since its introduction,Error
has always setContent-Type: text/plain; charset=utf-8
.Content-Length
: This obviously shouldn't be set, and soError
should delete it. (As it does now.)Content-Encoding
: I can't come up with a scenario where this can reasonably have a value. You theoretically could provide gzipped content toError
and setContent-Type: gzip
, but that seems unlikely in practice.Error
could probably delete it.Etag
,Last-Modified
,Cache-Control
,X-Content-Type-Options
: There are scenarios where a user might want to delete these. There are also scenarios where a user might be setting one of them deliberately.Error
should leave them alone.
from go.
(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 hope it was not a rough case.
Error
is also a very simple function; it isn't difficult for users to write their own version that does exactly what they want.
Yes, unless Error
is called from inside the standard library directly. In that case it should do the right thing and the result should be correct. The motivation behind all this was that http.ServeContent
was returning invalid headers and that it has a contract that you have to set some headers (like Etag
) before calling it, but it was not clearing them.
My first CL to fix http.ServeContent
had internal "error" function doing the correct thing (wrapping http.Error
). Then we moved its logic to Error
and now there is push back on that.
Maybe we should reintroduce this new "error" function which does more header cleanup and call it from standard library (at least from http.ServeContent
) while keeping changes to http.Error
limited to only those which work in all (reasonable) cases.
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 http.Error
in a similar way? But maybe it is useful to make it public?
from go.
I agree that we should add an internal error function for ServeContent
. I think it's fine for it to stay internal; it would be easy for us to provide something that is useful for some users some of the time, but the bar for adding a new API is higher than that. (If we can do better than Error
for most users most of the time, that'd probably meet the bar, but I'm not sure that any of the suggested options would satisfy that.)
from go.
I also noticed in our own code, that we do set Content-Length
before calling http.Error
. We in fact have such a wrapper:
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 Content-Length
. Maybe Content-Length
should be removed if the value does not match the length of the content to be written (+1)?
from go.
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.
from go.
This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group
from go.
Following @neild's suggestion, I revise this proposal to:
- Delete Content-Length (already done, obviously correct)
- Delete Content-Encoding (obviously correct)
and nothing else.
from go.
But we can still delete the other headers in the response from ServeContent
, yes? So the proposal is just to not delete them in general from Error
?
from go.
@mitar what specific headers would ServeContent delete before calling Error? From https://pkg.go.dev/net/http#ServeContent it looks like just Etag?
from go.
@rsc: Etag
, Last-Modified
, Cache-Control
(or override it to no-cache
).
from go.
Related Issues (20)
- cmd/go: TestScript/gotoolchain_loop failures HOT 3
- build: build failure on x_tools-go1.21-darwin-amd64-nocgo HOT 1
- build: build failure on x_tools-go1.22-linux-amd64-boringcrypto HOT 1
- os/signal: TestNotifyContextNotifications failures HOT 2
- cmd/internal/testdir: Test/goto.go failures HOT 1
- cmd/go: TestScript/test_env_term failures HOT 1
- cmd/go: TestScript/mod_pseudo_cache failures HOT 2
- cmd/go: TestScript/mod_download_exec_toolchain failures HOT 2
- build: os/signal failures HOT 2
- build: build failure on x_vulndb-gotip-darwin-amd64_13 HOT 1
- x/build: openbsd builder fail with "resource temporarily unavailable" HOT 2
- os/signal: TestSignalTrace failures HOT 3
- crypto/tls: TestBogoSuite failures HOT 20
- x/tools/gopls: cgocall analyzer: "token.Pos not within package" bug (reported by telemetry) HOT 4
- path/filepath: Windows Path Issue HOT 2
- crypto/tls: bogo suite on Windows is flakey HOT 3
- crypto/tls: bogo test output is hard to parse
- runtime: use frame pointer unwind to get backtrace in goroutine profile HOT 4
- compiler: crash on assert HOT 1
- x/mobile: Is gomobile still maintained? HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from go.