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

Jump to bottom

net/http: Broken If-Range handling in ServeFile and friends unless caller sets ETag manually #8367

Closed
gopherbot opened this issue Jul 14, 2014 · 7 comments
Closed

net/http: Broken If-Range handling in ServeFile and friends unless caller sets ETag manually #8367

gopherbot opened this issue Jul 14, 2014 · 7 comments
Labels
FrozenDueToAge
Milestone
Go1.4

Comments

@gopherbot
Copy link 

What does 'go version' print?

go version go1.3 linux/amd64

What steps reproduce the problem?

1. You will need an MPEG4 version 2 video file that your browser agrees to play. It
needs to be the specific kind of mp4 that has metadata at the end of the file. I'm sorry
but I don't currently have a publicly available example of one. Put it as video.mp4 in
the directory with the attached files.

2. Download the attached *.go and *.html and "go run buggy-logging.go"

3. Open http://localhost:8000/
tested with Chrome 36.0.1985.97 beta on linux

4. Click play

As the bug depends on browser caching behavior, I find I typically need to "Empty
cache and hard reload", and then perform steps 3 & 4 twice to trigger it.

5. You should see just a black screen, no video playing. Log attached as bug.log. I have
done a similar run with "go run buggy.go", which skips the logging, to prove
that my logging kludge does not change behavior.

Log attached as "bug.log"

6. Control-C the buggy-logging version, go run etag-logging.go

7. "Empty cache and hard reload" and perform steps 3 & 4 (twice?) again.

8. You should see the video play.

Log attached as "etag.log"




The difference in the logs is:

GET /video.mp4
Range: bytes=798054665-798503344
If-Range: Wed, 25 Jun 2014 17:12:18 GMT

200 OK  this causes the bug

vs

GET /video.mp4
Range:bytes=798054665-798503344
If-Range: fake

206 OK  this works
Content-Range: bytes 798054665-798503344/798503345


The above seems to be caused by net/http not sending an Etag, only Last-Modified, but
only accepting ETags for If-Range. I would recommend either

1. Make net/http serveFile provide an ETag (if not already set in ResponseWriter) from
hashing FileInfo.Size(), .ModTime().Unix(), .ModTime().Nanosecond(), and on unix,
syscall.Stat_t.Ctim.Sec, .Ctim.Nsec, .Dev, .Ino
OR
2. checkETag() should fall back to w.Header().get("Last-Modified") if the
response has no ETag

The first one seems preferable, and the stat call is done already, so there's no real
cost to it.

Chrome should also probably know to handle the failure right. It seems like this is
reported as https://code.google.com/p/chromium/issues/detail?id=74852 -- but net/http
should not trigger that logic when no actual file change happened.

Attachments:

  1. index.html (209 bytes)
  2. buggy-logging.go (842 bytes)
  3. buggy.go (484 bytes)
  4. etag-logging.go (875 bytes)
  5. bug.log (1082 bytes)
  6. etag.log (2295 bytes)
@ianlancetaylor
Copy link
Contributor

Comment 1:

Labels changed: added repo-main, release-go1.4.

@taruti
Copy link
Contributor

taruti commented Jul 14, 2014

Comment 2:

This issue also was a problem for us, did workaround by modifying net/http/fs.go
locally. The issue is that Chrome/ium sends a date in If-Range and that causes
net/http/fs.go to zero the Range. Around line 300 in fs.go there is a TODO about this.
Occurs in e.g. seeking large mp4 files with html5 videos.

@bradfitz
Copy link
Contributor

Comment 3:

Let me try to re-summarize this bug more succinctly, without attachments. Please correct
me if this is wrong (or confirm if this is correct).
Browsers may send an If-Range header containing either an ETag or a date:
http://tools.ietf.org/html/rfc7233#section-3.2
    ....
    If-Range = entity-tag / HTTP-date
    ....
The Go code only deals with the entity-tag half, and in fact has a comment about nobody
seeming to care about sending a date, and the spec being unclear. From fs.go:
    if ir := r.Header.get("If-Range"); ir != "" && ir != etag {
        // TODO(bradfitz): handle If-Range requests with Last-Modified                                                  
        // times instead of ETags? I'd rather not, at least for                                                         
        // now. That seems like a bug/compromise in the RFC 2616, and                                                   
        // I've never heard of anybody caring about that (yet).                                                         
        rangeReq = ""
    }
I like the "yet" part. That time apparently is now and Chrome does send a date in
If-Range if the handler never sent an ETag.
Note that the comment is discussing RFC 2616, and not the clarification RFC 7233.  For
reference, 2616 said: http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.27
I'd rather not get into the business of guessing ETags for users. Maybe for on-disk
files that's acceptable, but it should be a separate bug.
For this bug, I think simply accepting an If-Range date matching the Last-Modified time
should be an acceptable fix.

Status changed to Accepted.

@gopherbot
Copy link
Author

Comment 4:

Yes, the point of the attachments was purely to demonstrate that this is not a
theoretical concern: net/http cannot serve html5 video without workarounds, currently. 
ServeFile is already talking about on-disk files, so I don't see why an ETag *there*
would actively hurt. I don't think net/http should provide ETags for just any resource.

@bradfitz
Copy link
Contributor

Comment 5:

ServeFile is a thin wrapper around ServeContent. The missing If-Range date handling is
in ServeContent.
Adding ETag in the way proposed could actively hurt: imagine a load-balanced environment
with Go servers on many backends with different filesystems. The inode number could
differ and cause problems if mixed in to the ETag. Again, please open a separate bug for
that if you feel strongly and it can be discussed or designed separately. This bug
should be about the date handling only.

@gopherbot
Copy link
Author

Comment 6:

CL https://golang.org/cl/116300044 mentions this issue.

@bradfitz
Copy link
Contributor

Comment 7:

This issue was closed by revision f5037ee.

Status changed to Fixed.

@rsc rsc added this to the Go1.4 milestone Apr 14, 2015
@rsc rsc removed the release-go1.4 label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018
@bradfitz @wheatman
net/http: make ServeContent support dates in If-Range headers
fc36c4e 
Fixes golang#8367

LGTM=adg
R=adg
CC=golang-codereviews
https://golang.org/cl/116300044
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018
@bradfitz @wheatman
net/http: make ServeContent support dates in If-Range headers
5f41d8a 
Fixes golang#8367

LGTM=adg
R=adg
CC=golang-codereviews
https://golang.org/cl/116300044
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.4
Development

No branches or pull requests
5 participants
@bradfitz @taruti @rsc @ianlancetaylor @gopherbot