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: http.FileServer Partial Content returns no content #27085

Closed
klauspost opened this issue Aug 19, 2018 · 13 comments

Comments

Projects
None yet
7 participants
@klauspost
Copy link
Contributor

commented Aug 19, 2018

What version of Go are you using (go version)?

Go 1.11 RC1 go version go1.11rc1 windows/amd64

Does this issue reproduce with the latest release?

No, new for 1.11

What operating system and processor architecture are you using (go env)?

c:\temp\problem>go env
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\mfiles\AppData\Local\go-build
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=e:\gopath\
set GOPROXY=
set GORACE=
set GOROOT=c:\go
set GOTMPDIR=
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=e:\temp\wintemp\go-build429745914=/tmp/go-build -gno-record-gcc-switches

What did you do?

https://download.klauspost.com/partial-content.zip

  • Unzip
  • In folder, execute go run main.go
  • Open http://localhost:8080 (reproducible in both Chrome+Firefox)
  • Press play on controller

Music will stop playing after a short while.

The browser shows that while headers are returned correctly no content is sent.

Request:

GET /music/music.mp3 HTTP/1.1
Host: localhost:8080
Connection: keep-alive
Pragma: no-cache
Cache-Control: no-cache
Accept-Encoding: identity;q=1, *;q=0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.106 Safari/537.36
Accept: */*
Referer: http://localhost:8080/
Accept-Language: en-US,en;q=0.9,da;q=0.8
Range: bytes=65536-

Response headers:

HTTP/1.1 206 Partial Content
Accept-Ranges: bytes
Content-Type: audio/mpeg
Last-Modified: Sun, 22 Jul 2018 19:37:12 GMT
Date: Sun, 19 Aug 2018 16:29:30 GMT
Content-Range: bytes 65536-6909673/6909674
Content-Length: 6844138

But no content is actually sent.

What did you expect to see?

The data for the requested range to be included in the response.

What did you see instead?

0 bytes are included in the response.

@meirf

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2018

(I'm not able to reproduce this with go version go1.11rc1 darwin/amd64)

@minaevmike

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

my guess that error happens in io.CopyN, but unfortunately this error ignores https://github.com/golang/go/blob/master/src/net/http/fs.go#L296 . I think that this error must be handled. mb there is some windows-specific error here but we can't see it.
For check it you can try to modify this code locally and try to see the error.

@klauspost

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2018

Changing the line above to:

	if r.Method != "HEAD" {
		n, err := io.CopyN(w, sendContent, sendSize)
		fmt.Println(n, err)
	}

Prints out:

Serving from: c:\temp\problem on http://localhost:8080
316 <nil>
0 EOF
0 EOF
0 EOF
0 EOF
0 EOF
...

Printing a bit more it goes through this part of the previous code. The content.Seek does not return an error (ra.start is perfectly reasonable value).

The content is an *os.File. If I am checking the returned position from the Seek call, it is a valid value.

NOW for the really funky part. If I insert a content.Read([]byte{0}) after the Seek, but before the CopyN everything works fine (except for the slight corruption).

It seems like the w ResponseWriter - which is an *http.response implements io.ReaderFrom, so that is used for the io.CopyN.

However I have a hard time seeing the specific problems, it seems there is a problem when the ReaderFrom is used on a Windows file right after seek has been called on it.

@klauspost

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2018

ok, digging deeper n0, err := rf.ReadFrom(src) returns 0, nil when stuff is failing, I guess indicating that it could not read anything from the file.

@alresvor

This comment has been minimized.

Copy link

commented Aug 21, 2018

The commit that introduced the new behaviour: af4d604

@meirf

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

@klauspost, on a whim, does it work if you reverse:

+	o.o.OffsetHigh = uint32(curpos)
+	o.o.Offset = uint32(curpos >> 32)

And change OffsetHigh to Offset and vice-versa?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Aug 22, 2018

Change https://golang.org/cl/130855 mentions this issue: net: add a sendfile test involving an initial seek

@klauspost

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2018

@bradfitz Yes, that fixes the issue on my end (and also looks correct).

@gopherbot

This comment has been minimized.

Copy link

commented Aug 22, 2018

Change https://golang.org/cl/130895 mentions this issue: [release-branch.go1.11] internal/poll, net: fix sendfile on Windows, add test

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

@klauspost, thanks. I also was able to repro (with new test) and confirm the fix. CLs outstanding.

@gopherbot gopherbot closed this in d35135b Aug 22, 2018

gopherbot pushed a commit that referenced this issue Aug 22, 2018

[release-branch.go1.11] internal/poll, net: fix sendfile on Windows, …
…add test

Fixes #27085

Change-Id: I4eb3ff7c76e0b8e4d8fe0298f739b0284d74a031
Reviewed-on: https://go-review.googlesource.com/130895
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

tmm1 added a commit to fancybits/go that referenced this issue Sep 17, 2018

internal/poll, net: fix sendfile on Windows, add test
Fixes golang#27085

Change-Id: I4eb3ff7c76e0b8e4d8fe0298f739b0284d74a031
Reviewed-on: https://go-review.googlesource.com/130855
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Nov 1, 2018

Change https://golang.org/cl/146797 mentions this issue: [release-branch.go1.10] internal/poll, net: fix sendfile on Windows, add test

gopherbot pushed a commit that referenced this issue Nov 1, 2018

[release-branch.go1.10] internal/poll, net: fix sendfile on Windows, …
…add test

Cherry pick of CL 130855, done manually to avoid a merge conflict on the test.

Fixes #27085

Change-Id: I7c4939cf5db23253a824c46c3f00fab4edec86b4
Reviewed-on: https://go-review.googlesource.com/c/146797
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.