Open jech opened 6 days ago
Related Issues and Documentation
(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)
CC @neild
Maybe we should instead add a NoSeeker(io.Reader) io.ReadSeeker
and have http.ServeContent
gracefully handle errors.ErrUnsupported
by not handling range / content type sniffing. That way #44553 (fs.File
without Seek
) can also be made to work.
See previously: https://github.com/golang/go/issues/61791#issuecomment-1671917758 and https://github.com/golang/go/issues/42173
Basically, supporting range requests is really important for performance. I'm not sure net/http should make it easy to serve non-seekable streams, but if it does it should require explicit opt-in (e.g. with a separate function) and not happen automatically based on Seek
returning ErrUnsupported
as that would be a performance foot gun.
See previously: #61791 (comment) and #42173
I think that's orthogonal to what @seankhliao is suggesting.
@seankhliao is suggesting that ServeContent
shoul be able to serve unseekable streams. The comments cited by @AWGA argue that in case an unseekable stream is sent in reply to a range request, the request should return an error rather than returning the whole file.
There is no reason why we shouldn't follow @seankhliao suggestion and also return an error when a range request is applied to an unseekable file.
The motivation is to allow use of ServeContent's conditional request logic (If-None-Match, etc.) with non-seekable content.
Perhaps instead of modifying ServeContent to support non-seekable io.Readers, we should expose the conditional request logic independently of ServeContent?
package http
type Preconditions struct {
LastModified time.Time
EntityTag string
}
// CheckPreconditions checks RFC 9110 preconditions.
// It returns true if the request should be processed.
// It returns false if the request should not be processed.
//
// If p.LastModified is not zero, CheckPreconditions sets the Last-Modified header
// and applies the If-Modified-Since and If-Unmodified-Since preconditions.
//
// If p.EntityTag is not "", CheckPreconditions sets the Etag header
// and applies the If-Match and If-None-Match preconditions.
func CheckPreconditions(w ResponseWriter, r *Request, p Preconditions) (shouldServe bool)
(That's not a full design, just an idea. I don't particularly like the name, the bool return is confusing, and I'm not sure what should be done with If-Range requests.)
Perhaps instead of modifying ServeContent to support non-seekable io.Readers, we should expose the conditional request logic independently of ServeContent?
I think it might be worth doing both. ServeContent is a simple, bulletproof interface, while the conditional request logic is useful for custom handlers.
FWIW, here is what I'm currently using: https://github.com/jech/galene/blob/master/webserver/precondition.go#L91. The function either returns false, meaning that normal processing should be done by the caller, or else it calls ResponseWriter.WriteHeader
with the right code and returns true to indicate that all processing has been done.
Here's an example of usage: https://github.com/jech/galene/blob/master/webserver/api.go#L213
Proposal Details
The function
net/http.ServeContent
requires itscontent
parameter to implement theSeek
method. However, most of the functionality ofServeContent
does not requireSeek
functionality.I've recently had to serve some non-seekable streams, and I've had to reimplement much of the functionality of
ServeContent
, most notably the conditional request logic (If-None-Match
and friends). It would have been helpful ifServeContent
could take a non-seekable stream.Unfortunately,
ServeContent
cannot be modified to take anio.Reader
for backwards compatibility reasons (please correct me if I'm wrong). I therefore propose a new functionwhich works just like
ServeContent
except that:Seek
;Range
andIf-Range
) are ignored;Content-Length
header is not automatically generated, if one is required, then the caller must set it before the call.The function obeys the
If-Modified-Since
header if themodTime
parameter is set. It obeys all other conditional headers (If-None-Match
and friends) if theETag
header is set on the reply. I also suggest thatServeStream
should transparently callServeContent
if its last argument is anio.ReadSeeker
.