code review 4389048: http: client gzip support

http: client gzip support

[bradfitz, 2011-04-12 01:19:42]

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

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

[adg, 2011-04-12 01:30:02]

http://codereview.appspot.com/4389048/diff/3001/src/pkg/http/transport.go
File src/pkg/http/transport.go (right):

http://codereview.appspot.com/4389048/diff/3001/src/pkg/http/transport.go#new...
src/pkg/http/transport.go:481: // own value for Accept-Encoding. We only
attempted to
s/attempted/attempt/

http://codereview.appspot.com/4389048/diff/3001/src/pkg/http/transport.go#new...
src/pkg/http/transport.go:487: req.Header.Set("Accept-Encoding", "gzip")
should we support deflate too?

[bradfitz, 2011-04-12 01:37:42]

Anybody know a server that supports deflate to test against?  Apache 2
(mod_deflate) doesn't seem to.

On Mon, Apr 11, 2011 at 6:30 PM, <adg@golang.org> wrote:

>
> http://codereview.appspot.com/4389048/diff/3001/src/pkg/http/transport.go
> File src/pkg/http/transport.go (right):
>
>
>
http://codereview.appspot.com/4389048/diff/3001/src/pkg/http/transport.go#new...
> src/pkg/http/transport.go:481: // own value for Accept-Encoding. We only
> attempted to
> s/attempted/attempt/
>
>
>
http://codereview.appspot.com/4389048/diff/3001/src/pkg/http/transport.go#new...
> src/pkg/http/transport.go:487: req.Header.Set("Accept-Encoding", "gzip")
> should we support deflate too?
>
>
> http://codereview.appspot.com/4389048/
>

[rsc, 2011-04-12 03:20:59]

http://codereview.appspot.com/4389048/diff/3001/src/pkg/http/transport.go
File src/pkg/http/transport.go (right):

http://codereview.appspot.com/4389048/diff/3001/src/pkg/http/transport.go#new...
src/pkg/http/transport.go:487: req.Header.Set("Accept-Encoding", "gzip")
On 2011/04/12 01:30:02, adg wrote:
> should we support deflate too?

When someone finds a server that implements deflate and not gzip.
But not until then.

http://codereview.appspot.com/4389048/diff/3001/src/pkg/http/transport.go#new...
src/pkg/http/transport.go:513: if err != nil {
Why is this not return nil, err?

[bradfitz, 2011-04-12 03:36:21]

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

Please take another look.

[bradfitz, 2011-04-12 03:37:19]

On Mon, Apr 11, 2011 at 8:20 PM, <rsc@golang.org> wrote:

>
>
>
http://codereview.appspot.com/4389048/diff/3001/src/pkg/http/transport.go#new...
> src/pkg/http/transport.go:513: if err != nil {
> Why is this not return nil, err?


Oh, right.  I thought I was somewhere else.  Done.

[rsc, 2011-04-12 03:50:44]

LGTM

http://codereview.appspot.com/4389048/diff/5002/src/pkg/http/transport_test.go
File src/pkg/http/transport_test.go (right):

http://codereview.appspot.com/4389048/diff/5002/src/pkg/http/transport_test.g...
src/pkg/http/transport_test.go:361: fmt.Fprintf(gz, "%s", testString)
gz.Write([]byte(testString))

http://codereview.appspot.com/4389048/diff/5002/src/pkg/http/transport_test.g...
src/pkg/http/transport_test.go:378: t.Fatalf("Expected response body of %q; got
%q", e, g)
t.Fatalf("body = %q, want %q", g, e)

http://codereview.appspot.com/4389048/diff/5002/src/pkg/http/transport_test.g...
src/pkg/http/transport_test.go:380: }
func TestTransportGzipRecursive(t *testing.T) {
    ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
        w.Header().Set("Content-Encoding", "gzip")
        w.Write(rgz)
    }))
    defer ts.Close()

    c := &Client{Transport: &Transport{}}
    res, _, err := c.Get(ts.URL)
    if err != nil {
        t.Fatal(err)
    }
    body, err := ioutil.ReadAll(res.Body)
    if err != nil {
        t.Fatal(err)
    }
    if !bytes.Equal(body, rgz) {
        t.Fatalf("Incorrect result from recursive gz:\nhave=%x\nwant=%x", body,
rgz)
    }
}

var rgz = []byte{
 0x1f, 0x8b, 0x08, 0x08, 0x00, 0x00, 0x00, 0x00,
 0x00, 0x00, 0x72, 0x65, 0x63, 0x75, 0x72, 0x73,
 0x69, 0x76, 0x65, 0x00, 0x92, 0xef, 0xe6, 0xe0,
 0x60, 0x00, 0x83, 0xa2, 0xd4, 0xe4, 0xd2, 0xa2,
 0xe2, 0xcc, 0xb2, 0x54, 0x06, 0x00, 0x00, 0x17,
 0x00, 0xe8, 0xff, 0x92, 0xef, 0xe6, 0xe0, 0x60,
 0x00, 0x83, 0xa2, 0xd4, 0xe4, 0xd2, 0xa2, 0xe2,
 0xcc, 0xb2, 0x54, 0x06, 0x00, 0x00, 0x17, 0x00,
 0xe8, 0xff, 0x42, 0x12, 0x46, 0x16, 0x06, 0x00,
 0x05, 0x00, 0xfa, 0xff, 0x42, 0x12, 0x46, 0x16,
 0x06, 0x00, 0x05, 0x00, 0xfa, 0xff, 0x00, 0x05,
 0x00, 0xfa, 0xff, 0x00, 0x14, 0x00, 0xeb, 0xff,
 0x42, 0x12, 0x46, 0x16, 0x06, 0x00, 0x05, 0x00,
 0xfa, 0xff, 0x00, 0x05, 0x00, 0xfa, 0xff, 0x00,
 0x14, 0x00, 0xeb, 0xff, 0x42, 0x88, 0x21, 0xc4,
 0x00, 0x00, 0x14, 0x00, 0xeb, 0xff, 0x42, 0x88,
 0x21, 0xc4, 0x00, 0x00, 0x14, 0x00, 0xeb, 0xff,
 0x42, 0x88, 0x21, 0xc4, 0x00, 0x00, 0x14, 0x00,
 0xeb, 0xff, 0x42, 0x88, 0x21, 0xc4, 0x00, 0x00,
 0x14, 0x00, 0xeb, 0xff, 0x42, 0x88, 0x21, 0xc4,
 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0x00, 0x00,
 0x00, 0xff, 0xff, 0x00, 0x17, 0x00, 0xe8, 0xff,
 0x42, 0x88, 0x21, 0xc4, 0x00, 0x00, 0x00, 0x00,
 0xff, 0xff, 0x00, 0x00, 0x00, 0xff, 0xff, 0x00,
 0x17, 0x00, 0xe8, 0xff, 0x42, 0x12, 0x46, 0x16,
 0x06, 0x00, 0x00, 0x00, 0xff, 0xff, 0x01, 0x08,
 0x00, 0xf7, 0xff, 0x3d, 0xb1, 0x20, 0x85, 0xfa,
 0x00, 0x00, 0x00, 0x42, 0x12, 0x46, 0x16, 0x06,
 0x00, 0x00, 0x00, 0xff, 0xff, 0x01, 0x08, 0x00,
 0xf7, 0xff, 0x3d, 0xb1, 0x20, 0x85, 0xfa, 0x00,
 0x00, 0x00, 0x3d, 0xb1, 0x20, 0x85, 0xfa, 0x00,
 0x00, 0x00,
}

[bradfitzgoog, 2011-04-12 04:43:11]

http://codereview.appspot.com/4389048/diff/5002/src/pkg/http/transport_test.go
File src/pkg/http/transport_test.go (right):

http://codereview.appspot.com/4389048/diff/5002/src/pkg/http/transport_test.g...
src/pkg/http/transport_test.go:380: }
On 2011/04/12 03:50:44, rsc wrote:
> func TestTransportGzipRecursive(t *testing.T) {

But this would fail.  It would've been gunzipped already.

What do you want tested?

[rsc, 2011-04-12 11:38:21]

>> func TestTransportGzipRecursive(t *testing.T) {
>
> But this would fail.  It would've been gunzipped already.

Should succeed.  It gunzips to itself.
Should not go into an infinite loop though.

Russ

[bradfitz, 2011-04-12 16:18:45]

On Tue, Apr 12, 2011 at 4:38 AM, Russ Cox <rsc@golang.org> wrote:

> >> func TestTransportGzipRecursive(t *testing.T) {
> >
> > But this would fail.  It would've been gunzipped already.
>
> Should succeed.  It gunzips to itself.
> Should not go into an infinite loop though.
>

Oh, one of your quines.  :-)

So basically all it tests is that it doesn't loop forever , which is more
appropriately a test for the gzip package, not http.  From http's
perspective we can't tell from this test whether the http decompress logic
happened 0, 1, or multiple times.

I'll add the test, but it's cute more than useful.

[bradfitz, 2011-04-12 16:35:18]

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

http: client gzip support

R=adg, rsc, bradfitzwork
CC=golang-dev
http://codereview.appspot.com/4389048

[rsc, 2011-04-12 18:24:08]

> So basically all it tests is that it doesn't loop forever , which is more
> appropriately a test for the gzip package, not http.  From http's
> perspective we can't tell from this test whether the http decompress logic
> happened 0, 1, or multiple times.
> I'll add the test, but it's cute more than useful.

Fair enough, but it's definitely an http test,
not a gzip test.  It's testing that the http client
does not do some kind of auto-sniffing to decide
whether to decompress, at least not recursively.

Just as an example, if you send BSD a compressed
IP packet that has as its payload the compressed
version of itself, BSD falls over recursively injecting
the packet back into the IP stack.  That's a bug in
the IP stack, not in its gzip library.

It's more cute than useful but still a little useful,
just like testing that compilers work against type T *T
(you'd be surprised how many tools that line has
broken).

Russ

[bradfitz, 2011-04-12 18:26:47]

On Tue, Apr 12, 2011 at 11:24 AM, Russ Cox <rsc@golang.org> wrote:

> > So basically all it tests is that it doesn't loop forever , which is more
> > appropriately a test for the gzip package, not http.  From http's
> > perspective we can't tell from this test whether the http decompress
> logic
> > happened 0, 1, or multiple times.
> > I'll add the test, but it's cute more than useful.
>
> Fair enough, but it's definitely an http test,
> not a gzip test.  It's testing that the http client
> does not do some kind of auto-sniffing to decide
> whether to decompress, ...


gross!  :)

But yeah, fair enough.

[tarm, 2011-04-21 20:42:19]

Please see
http://code.google.com/p/go/issues/detail?id=1724
which I believe was caused by this changeset.

Thanks,
Tarmigan