code review 6351052: net/http: support multiple byte ranges in ServeContent

net/http: support multiple byte ranges in ServeContent

Fixes issue 3784

[bradfitz, 2012-06-29 00:52:44]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2012-06-29 00:53:40]

R=adg or rsc or whoever likes/knows this sort of stuff.

On Thu, Jun 28, 2012 at 5:52 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net/http: support multiple byte ranges in ServeContent
>
> Fixes issue 3784
>
> Please review this at
http://codereview.appspot.com/**6351052/<http://codereview.appspot.com/6351052/>
>
> Affected files:
>  M src/pkg/net/http/fs.go
>  M src/pkg/net/http/fs_test.go
>  M src/pkg/net/http/range_test.go
>
>
>

[adg, 2012-06-29 05:36:49]

http://codereview.appspot.com/6351052/diff/5001/src/pkg/net/http/fs.go
File src/pkg/net/http/fs.go (right):

http://codereview.appspot.com/6351052/diff/5001/src/pkg/net/http/fs.go#newcod...
src/pkg/net/http/fs.go:200: return
do you really want to just throw these errors away?

at the top of this func, you could have

var readErr error
defer pw.CloseWithError(readErr)

and then here

readErr = err
return

with the same for CopyN (and for the CreatePart call, too, although I don't
think it's actually necessary)

[bradfitz, 2012-06-29 13:45:00]

Hello golang-dev@googlegroups.com, adg@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-06-29 13:47:04]

PTAL

On Thu, Jun 28, 2012 at 10:36 PM, <adg@golang.org> wrote:

>
>
http://codereview.appspot.com/**6351052/diff/5001/src/pkg/net/**http/fs.go<ht...
> File src/pkg/net/http/fs.go (right):
>
> http://codereview.appspot.com/**6351052/diff/5001/src/pkg/net/**
>
http/fs.go#newcode200<http://codereview.appspot.com/6351052/diff/5001/src/pkg/net/http/fs.go#newcode200>
> src/pkg/net/http/fs.go:200: return
> do you really want to just throw these errors away?
>

Fixed.  They're now propagate up, where io.Copy will now throw them away.
 I could log if the io.Seeker doesn't seek, maybe (it's documented that it
must work), but the other errors shouldn't log.

at the top of this func, you could have
>
> var readErr error
> defer pw.CloseWithError(readErr)
>
> and then here
>
> readErr = err
>

defer evaluates its arguments immediately.  would need to be defer func() {
.... }()

I've restructured it differently.

[adg, 2012-06-29 14:38:43]

LGTM

On 2012/06/29 13:47:04, bradfitz wrote:
> Fixed.  They're now propagate up, where io.Copy will now throw them away.
>  I could log if the io.Seeker doesn't seek, maybe (it's documented that it
> must work), but the other errors shouldn't log.

Hm. Not a blocker for this CL, but let's chat about logging. Seems weird to me
that there's no way to see the read error here.

http://codereview.appspot.com/6351052/diff/3004/src/pkg/net/http/fs.go
File src/pkg/net/http/fs.go (right):

http://codereview.appspot.com/6351052/diff/3004/src/pkg/net/http/fs.go#newcod...
src/pkg/net/http/fs.go:436: // rangesMIMESize returns the nunber of bytes it'll
take to encode the
s/it'll take/it takes/

[bradfitz, 2012-06-29 14:44:11]

*** Submitted as http://code.google.com/p/go/source/detail?r=070604630d24 ***

net/http: support multiple byte ranges in ServeContent

Fixes issue 3784

R=golang-dev, adg
CC=golang-dev
http://codereview.appspot.com/6351052

[r, 2012-07-03 16:53:04]

http://codereview.appspot.com/6351052/diff/1002/src/pkg/net/http/fs_test.go
File src/pkg/net/http/fs_test.go (right):

http://codereview.appspot.com/6351052/diff/1002/src/pkg/net/http/fs_test.go#n...
src/pkg/net/http/fs_test.go:125: t.Errorf("range=%q content-type = %q; lacks
boundary", rt.r, ct)
continue?

http://codereview.appspot.com/6351052/diff/1002/src/pkg/net/http/fs_test.go#n...
src/pkg/net/http/fs_test.go:128: t.Errorf("range=%q Content-Length = %d; want
%d", rt.r, g, w)
continue?

http://codereview.appspot.com/6351052/diff/1002/src/pkg/net/http/fs_test.go#n...
src/pkg/net/http/fs_test.go:134: t.Fatalf("range=%q, reading part index %d: %v",
rt.r, ri, err)
why fatal?

http://codereview.appspot.com/6351052/diff/1002/src/pkg/net/http/fs_test.go#n...
src/pkg/net/http/fs_test.go:138: t.Fatalf("range=%q, reading part index %d body:
%v", rt.r, ri, err)
why fatal?

http://codereview.appspot.com/6351052/diff/1002/src/pkg/net/http/fs_test.go#n...
src/pkg/net/http/fs_test.go:146: t.Errorf("range=%q: part Content-Range = %q;
want %q", rt.r, g, w)
continue?