code review 5643067: net/http: add ServeContent

net/http: add ServeContent

Fixes issue 2039

[bradfitz, 2012-02-09 03:12:39]

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

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

[r, 2012-02-09 03:13:16]

new API needs to be in go1.html

[rsc1, 2012-02-09 03:24:45]

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

http://codereview.appspot.com/5643067/diff/4003/src/pkg/net/http/fs.go#newcod...
src/pkg/net/http/fs.go:111: // If the content also has a Stat method, use it to
find the
The comment says nothing about this, and it seems magical
and potentially unwanted.  Handling this information is nice, but I'd rather see
something like

func ServeContent(w ResponseWriter, req *Request, name string, mtime time.Time,
content io.ReadSeeker)

And then the comment should say that it handles the 
If-Modified-Since and also uses the file name to deduce
the MIME type, otherwise falling back to content.
The size can (and should) be gotten from the ReadSeeker.
If the seek to the end returns an error, a 500 seems fine.
If a ReadSeeker can't Seek, that's not our fault.

All that said, I really like how this is turning out.
It feels like the right primitive.

http://codereview.appspot.com/5643067/diff/4003/src/pkg/net/http/fs.go#newcod...
src/pkg/net/http/fs.go:158: ctype = "text/plain; charset=utf-8"
This should be DetectMIMEType or whatever we called the sniffer.

[bradfitz, 2012-02-09 03:30:52]

On Wed, Feb 8, 2012 at 7:24 PM, <rsc@google.com> wrote:

>
>
http://codereview.appspot.com/**5643067/diff/4003/src/pkg/net/**http/fs.go<ht...
> File src/pkg/net/http/fs.go (right):
>
> http://codereview.appspot.com/**5643067/diff/4003/src/pkg/net/**
>
http/fs.go#newcode111<http://codereview.appspot.com/5643067/diff/4003/src/pkg/net/http/fs.go#newcode111>
> src/pkg/net/http/fs.go:111: // If the content also has a Stat method,
> use it to find the
> The comment says nothing about this, and it seems magical
> and potentially unwanted.  Handling this information is nice, but I'd
> rather see something like
>
> func ServeContent(w ResponseWriter, req *Request, name string, mtime
> time.Time, content io.ReadSeeker)
>
> And then the comment should say that it handles the
> If-Modified-Since and also uses the file name to deduce
> the MIME type, otherwise falling back to content.
>

Perfect.


> The size can (and should) be gotten from the ReadSeeker.
> If the seek to the end returns an error, a 500 seems fine.
> If a ReadSeeker can't Seek, that's not our fault.
>

I was wondering about that and forgot to ask.  I like 500 in that case.

All that said, I really like how this is turning out.
> It feels like the right primitive.
>

I think it made the code simpler, too.  Smaller pieces.


> http://codereview.appspot.com/**5643067/diff/4003/src/pkg/net/**
>
http/fs.go#newcode158<http://codereview.appspot.com/5643067/diff/4003/src/pkg/net/http/fs.go#newcode158>
> src/pkg/net/http/fs.go:158: ctype = "text/plain; charset=utf-8"
> This should be DetectMIMEType or whatever we called the sniffer.


This is just code movement, but yes, this needs updating, and dsymonds
agrees.  This just predates the sniffing.

[rsc, 2012-02-09 03:37:09]

On Wed, Feb 8, 2012 at 22:30, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> This is just code movement, but yes, this needs updating, and dsymonds
> agrees.  This just predates the sniffing.

Okay.  Fix it or file an issue, but it's a 1-line change.

[bradfitz, 2012-02-09 03:37:56]

On Wed, Feb 8, 2012 at 7:37 PM, Russ Cox <rsc@golang.org> wrote:

> On Wed, Feb 8, 2012 at 22:30, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > This is just code movement, but yes, this needs updating, and dsymonds
> > agrees.  This just predates the sniffing.
>
> Okay.  Fix it or file an issue, but it's a 1-line change.
>

yeah, fixed.  1 line change and lot of deletes, too!

[bradfitz, 2012-02-09 03:58:29]

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

Please take another look.

[niemeyer, 2012-02-09 04:07:11]

Very cool indeed, thanks Brad.

Just a few suggestions.

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

http://codereview.appspot.com/5643067/diff/4004/src/pkg/net/http/fs.go#newcode98
src/pkg/net/http/fs.go:98: content.Seek(0, os.SEEK_SET)
It would be nice to check the error here too. Non-file seekers may fail more
arbitrarily.

http://codereview.appspot.com/5643067/diff/4004/src/pkg/net/http/fs.go#newcod...
src/pkg/net/http/fs.go:118: n, _ := io.ReadFull(content, buf[:])
Feels like this error shouldn't be ignored as well.

http://codereview.appspot.com/5643067/diff/4004/src/pkg/net/http/fs.go#newcod...
src/pkg/net/http/fs.go:121: content.Seek(0, os.SEEK_SET) // rewind to output
whole file
Ditto.

[r, 2012-02-09 04:13:13]

still needs release notes.

[r2, 2012-02-09 04:36:48]

all right, maybe not necessary.

-rob

[bradfitz, 2012-02-09 04:48:10]

Hello r@golang.org, rsc@golang.org, n13m3y3r@gmail.com, r@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-02-09 04:50:18]

PTAL

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

http://codereview.appspot.com/5643067/diff/4004/src/pkg/net/http/fs.go#newcode98
src/pkg/net/http/fs.go:98: content.Seek(0, os.SEEK_SET)
On 2012/02/09 04:07:12, niemeyer wrote:
> It would be nice to check the error here too. Non-file seekers may fail more
> arbitrarily.

Done.

http://codereview.appspot.com/5643067/diff/4004/src/pkg/net/http/fs.go#newcod...
src/pkg/net/http/fs.go:118: n, _ := io.ReadFull(content, buf[:])
On 2012/02/09 04:07:12, niemeyer wrote:
> Feels like this error shouldn't be ignored as well.

This one doesn't matter much. It's only for content sniffing, and it'll be
re-read later, so the error can be returned then.

Well, I guess it won't be returned later, but I'd like to be consistent with how
we return errors, not as a function of the file extension.

Even if io.Copy below fails, we can't even reliably send an error if we've
already written headers.

*shrug*

I'm inclined to not care, since we never cared in the past.

http://codereview.appspot.com/5643067/diff/4004/src/pkg/net/http/fs.go#newcod...
src/pkg/net/http/fs.go:121: content.Seek(0, os.SEEK_SET) // rewind to output
whole file
On 2012/02/09 04:07:12, niemeyer wrote:
> Ditto.

Done.

[rog, 2012-02-09 09:39:40]

it would be nice if we had a convenient way of serving up a bunch of bytes as a
ReadSeeker.

if we had bytes.ReaderAt, then it would kill two birds with one stone.

package bytes
type ReaderAt []byte
func (r ReaderAt) ReadAt(p []byte, off int64) (int, error) {
	switch {
	case off > int64(len(r)):
		off = int64(len(r))
	case off < 0:
		off = 0
	}
	n := copy(p, r[0:int(off)])
	if n < len(p) {
		return n, io.EOF
	}
	return n, nil
}

then you can use io.SectionReader to turn it into a ReadSeeker.

i guess this wouldn't happen before Go 1 though.

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

http://codereview.appspot.com/5643067/diff/1004/src/pkg/net/http/fs.go#newcode86
src/pkg/net/http/fs.go:86: // The name parameter is used to set the response's
MIME type.
add "if the Content-Type header is not already set"?

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

http://codereview.appspot.com/5643067/diff/1004/src/pkg/net/http/fs_test.go#n...
src/pkg/net/http/fs_test.go:26: var ServeFileRangeTests = []struct {
since the range parsing is in ServeContent now, maybe it would be appropriate to
move these tests into TestServeContent?

[bradfitz, 2012-02-09 22:24:41]

Anything else, Russ?


On Thu, Feb 9, 2012 at 3:50 PM, <bradfitz@golang.org> wrote:

> PTAL
>
>
>
>
http://codereview.appspot.com/**5643067/diff/4004/src/pkg/net/**http/fs.go<ht...
> File src/pkg/net/http/fs.go (right):
>
> http://codereview.appspot.com/**5643067/diff/4004/src/pkg/net/**
>
http/fs.go#newcode98<http://codereview.appspot.com/5643067/diff/4004/src/pkg/net/http/fs.go#newcode98>
> src/pkg/net/http/fs.go:98: content.Seek(0, os.SEEK_SET)
> On 2012/02/09 04:07:12, niemeyer wrote:
>
>> It would be nice to check the error here too. Non-file seekers may
>>
> fail more
>
>> arbitrarily.
>>
>
> Done.
>
>
> http://codereview.appspot.com/**5643067/diff/4004/src/pkg/net/**
>
http/fs.go#newcode118<http://codereview.appspot.com/5643067/diff/4004/src/pkg/net/http/fs.go#newcode118>
> src/pkg/net/http/fs.go:118: n, _ := io.ReadFull(content, buf[:])
> On 2012/02/09 04:07:12, niemeyer wrote:
>
>> Feels like this error shouldn't be ignored as well.
>>
>
> This one doesn't matter much. It's only for content sniffing, and it'll
> be re-read later, so the error can be returned then.
>
> Well, I guess it won't be returned later, but I'd like to be consistent
> with how we return errors, not as a function of the file extension.
>
> Even if io.Copy below fails, we can't even reliably send an error if
> we've already written headers.
>
> *shrug*
>
> I'm inclined to not care, since we never cared in the past.
>
>
> http://codereview.appspot.com/**5643067/diff/4004/src/pkg/net/**
>
http/fs.go#newcode121<http://codereview.appspot.com/5643067/diff/4004/src/pkg/net/http/fs.go#newcode121>
> src/pkg/net/http/fs.go:121: content.Seek(0, os.SEEK_SET) // rewind to
> output whole file
> On 2012/02/09 04:07:12, niemeyer wrote:
>
>> Ditto.
>>
>
> Done.
>
>
http://codereview.appspot.com/**5643067/<http://codereview.appspot.com/5643067/>
>

[rsc, 2012-02-09 22:34:04]

LGTM

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

http://codereview.appspot.com/5643067/diff/1004/src/pkg/net/http/fs.go#newcode82
src/pkg/net/http/fs.go:82: // The main benefits of ServeContent over io.Copy is
that it handles Range
s/benefits/benefit/
or s/is/are/

http://codereview.appspot.com/5643067/diff/1004/src/pkg/net/http/fs.go#newcode86
src/pkg/net/http/fs.go:86: // The name parameter is used to set the response's
MIME type.
If the response's Content-Type header is not set, ServeContent first tries to
deduce the type from name's file extension and, if that fails, falls back to
reading the first block of the content and passing it to DetectContentType.
The name is otherwise unused; in particular it can be empty and is never
sent in the response.

If modtime is not the zero time, ServeContent includes it in a Last-Modified
header in the response.  If the request includes an If-Modified-Since
header, ServeContent uses modtime to decide whether the
content needs to be sent at all.

The content's Seek method must work: ServeContent uses 
a seek to the end of the content to determine its size.

[bradfitz, 2012-02-09 23:02:13]

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

net/http: add ServeContent

Fixes issue 2039

R=r, rsc, n13m3y3r, r, rogpeppe
CC=golang-dev
http://codereview.appspot.com/5643067