code review 4538111: http: ServeFile shouldn't send Content-Length when Cont...

http: ServeFile shouldn't send Content-Length when Content-Encoding is set

Fixes issue 1905

[bradfitz, 2011-06-02 20:08:29]

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

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

[bradfitz, 2011-06-02 20:10:55]

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

Please take another look.

[bradfitz, 2011-06-02 20:11:19]

er, added "Fixes issue nnn" and %q -> %d.

On Thu, Jun 2, 2011 at 1:08 PM, <bradfitz@golang.org> wrote:

> Reviewers: rsc,
>
> Message:
> Hello rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg
>
>
> Description:
> http: ServeFile shouldn't send Content-Length when Content-Encoding is
> set
>
> Please review this at http://codereview.appspot.com/4538111/
>
> Affected files:
>  M src/pkg/http/fs.go
>  M src/pkg/http/fs_test.go
>
>
> Index: src/pkg/http/fs.go
> ===================================================================
> --- a/src/pkg/http/fs.go
> +++ b/src/pkg/http/fs.go
> @@ -175,7 +175,9 @@
>        }
>
>        w.Header().Set("Accept-Ranges", "bytes")
> -       w.Header().Set("Content-Length", strconv.Itoa64(size))
> +       if w.Header().Get("Content-Encoding") == "" {
> +               w.Header().Set("Content-Length", strconv.Itoa64(size))
> +       }
>
>        w.WriteHeader(code)
>
> Index: src/pkg/http/fs_test.go
> ===================================================================
> --- a/src/pkg/http/fs_test.go
> +++ b/src/pkg/http/fs_test.go
> @@ -109,6 +109,21 @@
>        get(ctype)
>  }
>
> +func TestServeFileWithContentEncoding(t *testing.T) {
> +       ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r
> *Request) {
> +               w.Header().Set("Content-Encoding", "foo")
> +               ServeFile(w, r, "testdata/file")
> +       }))
> +       defer ts.Close()
> +       resp, err := Get(ts.URL)
> +       if err != nil {
> +               t.Fatal(err)
> +       }
> +       if g, e := resp.ContentLength, int64(-1); g != e {
> +               t.Errorf("Content-Length mismatch: got %q, want %q", g, e)
> +       }
> +}
> +
>  func getBody(t *testing.T, req Request) (*Response, []byte) {
>        r, err := DefaultClient.Do(&req)
>        if err != nil {
>
>
>

[rsc, 2011-06-02 20:12:05]

I don't understand.
When is it okay to set Content-Encoding behind ServeFile's back?

[bradfitz, 2011-06-02 20:16:19]

On Thu, Jun 2, 2011 at 1:12 PM, <rsc@golang.org> wrote:

> I don't understand.
> When is it okay to set Content-Encoding behind ServeFile's back?


Whenever you want? e.g. people doing something like
http://nf.id.au/roll-your-own-gzip-encoded-http-handler first

This is ServeFile, not FileServer.

[rsc, 2011-06-02 20:23:14]

Okay, interesting.  Does the http server know enough
to close the connection after such a trick?
Or should it add chunked?  (Maybe it does?)

[bradfitz, 2011-06-02 20:24:00]

On Thu, Jun 2, 2011 at 1:23 PM, Russ Cox <rsc@golang.org> wrote:

> Okay, interesting.  Does the http server know enough
> to close the connection after such a trick?
> Or should it add chunked?  (Maybe it does?)
>

It always goes into chunking by default when there's no Content-Length.

[rsc, 2011-06-02 20:28:44]

LGTM

[bradfitz, 2011-06-02 20:36:56]

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

http: ServeFile shouldn't send Content-Length when Content-Encoding is set

Fixes issue 1905

R=rsc
CC=golang-dev
http://codereview.appspot.com/4538111