code review 5345041: http: make httputil's chunked reader/writer code a dire...

http: make httputil's chunked reader/writer code a direct copy

Arrange the code so that it's easier to keep edits in sync.

[adg, 2011-11-04 03:25:32]

Hello golang-dev@googlegroups.com,

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

[mikio, 2011-11-04 04:12:49]

http://codereview.appspot.com/5345041/diff/8001/src/pkg/Makefile
File src/pkg/Makefile (left):

http://codereview.appspot.com/5345041/diff/8001/src/pkg/Makefile#oldcode131
src/pkg/Makefile:131: net/http/fcgi\
sort?

e.g.,
net/http\
net/http/chunked\
net/http/cgi\
net/http/fcgi\
net/http/httptest\
net/http/httputil\
net/http/pprof\

[adg, 2011-11-04 04:16:26]

http://codereview.appspot.com/5345041/diff/8001/src/pkg/Makefile
File src/pkg/Makefile (left):

http://codereview.appspot.com/5345041/diff/8001/src/pkg/Makefile#oldcode131
src/pkg/Makefile:131: net/http/fcgi\
On 2011/11/04 04:12:49, mikioh wrote:
> sort?
> 
> e.g.,
> net/http\
> net/http/chunked\
> net/http/cgi\
> net/http/fcgi\
> net/http/httptest\
> net/http/httputil\
> net/http/pprof\

Done.

[mikio, 2011-11-04 04:28:02]

And please re-run hg sync, all.bash at go/src or gomake testshort at go/src/pkg.
Perhaps net/http/cgi/*_test.go files require a few import pkgs modification.

On 2011/11/04 04:16:26, adg wrote:
> http://codereview.appspot.com/5345041/diff/8001/src/pkg/Makefile
> File src/pkg/Makefile (left):
> 
> http://codereview.appspot.com/5345041/diff/8001/src/pkg/Makefile#oldcode131
> src/pkg/Makefile:131: net/http/fcgi\
> On 2011/11/04 04:12:49, mikioh wrote:
> > sort?
> > 
> > e.g.,
> > net/http\
> > net/http/chunked\
> > net/http/cgi\
> > net/http/fcgi\
> > net/http/httptest\
> > net/http/httputil\
> > net/http/pprof\
> 
> Done.

[bradfitz, 2011-11-04 04:31:47]

+0, deferring to r*.

I think this is cleaner, but I have no objections against small, focused
packages.

I remember once trying to add something to one of the files in io/ioutil
but got hit instead with circular dependency problems with the unrelated
file in ioutil.

If this goes in, I could also argue for os/temp.New{File,Directory}.
But I forgot the reason.
Just that I retain a grudge against ioutil for some historical reason.


On Thu, Nov 3, 2011 at 8:25 PM, <adg@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> http: move chunked Reader and Writer to package net/http/chunked
> http: fix target path in httptest Makefile
>
> This removes the need for the hacky work-around in httputil, and gives
> chunked.Reader and chunked.Writer idiomatic names.
>
> Please review this at
http://codereview.appspot.com/**5345041/<http://codereview.appspot.com/5345041/>
>
> Affected files:
>  M src/pkg/Makefile
>  M src/pkg/net/http/Makefile
>  A src/pkg/net/http/chunked/**Makefile
>  A src/pkg/net/http/chunked/**reader.go
>  M src/pkg/net/http/chunked/**writer.go
>  M src/pkg/net/http/client_test.**go
>  M src/pkg/net/http/fs_test.go
>  M src/pkg/net/http/httptest/**Makefile
>  M src/pkg/net/http/httputil/**Makefile
>  R src/pkg/net/http/httputil/**chunked.go
>  R src/pkg/net/http/httputil/**chunked_test.go
>  M src/pkg/net/http/httputil/**dump.go
>  M src/pkg/net/http/request.go
>  M src/pkg/net/http/request_test.**go
>  M src/pkg/net/http/response_**test.go
>  M src/pkg/net/http/serve_test.go
>  M src/pkg/net/http/sniff_test.go
>  M src/pkg/net/http/transfer.go
>  M src/pkg/net/http/transport_**test.go
>
>
>

[adg, 2011-11-04 04:54:17]

This change reveals a bug in the way trailers are handled. The behaviors of the
original chunkedReader and transport reader were complimentary (and therefore
correct when used together) but each wrong in isolation.

I corrected the chunkedReader behavior, so the net/http/chunked tests pass, but
now the net/http tests do not.

http://codereview.appspot.com/5345041/diff/8003/src/pkg/net/http/transfer.go
File src/pkg/net/http/transfer.go (right):

http://codereview.appspot.com/5345041/diff/8003/src/pkg/net/http/transfer.go#...
src/pkg/net/http/transfer.go:569: // TODO(petar): Put trailer reader code here
I think we need to absorb the trailing \r\n here. I tried the naive approach to
this (ReadBytes('\n')) but it still caused strange warning messages.

Brad - can you take a look?

[adg, 2011-11-04 21:54:42]

No that trailers are handled properly this is ready for review

Any objections to the net/http/chunked package?

On Friday, 4 November 2011,  <adg@golang.org> wrote:
> This change reveals a bug in the way trailers are handled. The behaviors
> of the original chunkedReader and transport reader were complimentary
> (and therefore correct when used together) but each wrong in isolation.
>
> I corrected the chunkedReader behavior, so the net/http/chunked tests
> pass, but now the net/http tests do not.
>
>
>
http://codereview.appspot.com/5345041/diff/8003/src/pkg/net/http/transfer.go
> File src/pkg/net/http/transfer.go (right):
>
>
http://codereview.appspot.com/5345041/diff/8003/src/pkg/net/http/transfer.go#...
> src/pkg/net/http/transfer.go:569: // TODO(petar): Put trailer reader
> code here
> I think we need to absorb the trailing \r\n here. I tried the naive
> approach to this (ReadBytes('\n')) but it still caused strange warning
> messages.
>
> Brad - can you take a look?
>
> http://codereview.appspot.com/5345041/
>

[andybalholm, 2011-11-05 16:52:53]

On 2011/11/04 21:54:42, adg wrote:
> No that trailers are handled properly this is ready for review
> 
> Any objections to the net/http/chunked package?

It sounds like a good idea.

I would like to test my package (go-icap.googlecode.com) with the new package,
but I can't get this CL to apply at tip.

[adg, 2011-11-05 19:50:11]

Just synced and uploaded. Try now.

On 6 November 2011 03:52,  <andybalholm@gmail.com> wrote:
> On 2011/11/04 21:54:42, adg wrote:
>>
>> No that trailers are handled properly this is ready for review
>
>> Any objections to the net/http/chunked package?
>
> It sounds like a good idea.
>
> I would like to test my package (go-icap.googlecode.com) with the new
> package, but I can't get this CL to apply at tip.
>
> http://codereview.appspot.com/5345041/
>

[andybalholm, 2011-11-05 23:26:45]

On 2011/11/05 19:50:11, adg wrote:
> Just synced and uploaded. Try now.

Building anything right now is a real pain, since all the package renames broke
goinstall, but the chunked package seems to work for me.

Andy

[rsc, 2011-11-07 16:53:57]

I object.

Why is this necessary?  Who uses them?
What is the hacky workaround in httputil?
Why do we need an entire package for two
functions?

[andybalholm, 2011-11-07 17:18:31]

On 2011/11/07 16:53:57, rsc wrote:
> I object.
> 
> Why is this necessary?  Who uses them?

I do, in go-icap.

If it is too much clutter, I can just copy and paste the code from http to my
own package. But I thought having NewChunkedReader exported was a good way to
avoid code duplication. See http://codereview.appspot.com/4634112

(NewChunkedWriter was already exported.)

[bradfitz, 2011-11-07 17:30:21]

On Mon, Nov 7, 2011 at 9:18 AM, <andybalholm@gmail.com> wrote:

> On 2011/11/07 16:53:57, rsc wrote:
>
>> I object.
>>
>
>  Why is this necessary?  Who uses them?
>>
>
> I do, in go-icap.
>

And this is why I preserved them when we moved stuff from http to httputil.

What adg is objecting to is my implementation of NewChunkedReader in
httputil:

// NewChunkedReader returns a new reader that translates the data read from
r
// out of HTTP "chunked" format before returning it.

// The reader returns io.EOF when the final 0-length chunk is read.

//

// NewChunkedReader is not needed by normal applications. The http package

// automatically decodes chunking when reading response bodies.

func NewChunkedReader(r io.Reader) io.Reader {
        // This is a bit of a hack so we don't have to copy chunkedReader
into
        // httputil.  It's a bit more complex than chunkedWriter, which is
copied
        // above.

        req, err := http.ReadRequest(bufio.NewReader(io.MultiReader(
                strings.NewReader("POST / HTTP/1.1\r\nTransfer-Encoding:
chunked\r\n\r\n"),
                r,
                strings.NewReader("\r\n"))))
        if err != nil {
                panic("bad fake request: " + err.Error())
        }
        return req.Body
}

[andybalholm, 2011-11-07 17:36:39]

On 2011/11/07 17:30:21, bradfitz wrote:

> What adg is objecting to is my implementation of NewChunkedReader in
> httputil:

I can see why a new package would be better than that.

[bradfitz, 2011-11-07 17:41:18]

On Mon, Nov 7, 2011 at 9:36 AM, <andybalholm@gmail.com> wrote:

> On 2011/11/07 17:30:21, bradfitz wrote:
>
>  What adg is objecting to is my implementation of NewChunkedReader in
>> httputil:
>>
>
> I can see why a new package would be better than that.
>

We could also just copy the implementation into httputil.  I thought this
was the middle road between code duplication and package proliferation.

I personally don't find this that gross.

[rsc, 2011-11-07 17:50:48]

Yes, the httputil implementation is a little kludgy.
I'd rather just copy the implementation over
than to create a new package.  It is sometimes
okay to duplicate code.

[bradfitz, 2011-11-07 17:55:31]

On Mon, Nov 7, 2011 at 9:50 AM, Russ Cox <rsc@golang.org> wrote:

> Yes, the httputil implementation is a little kludgy.
> I'd rather just copy the implementation over
> than to create a new package.  It is sometimes
> okay to duplicate code.
>

Well, until my recent trailers CL, the chunked reader in http was wrong.  I
especially didn't want to duplicate wrong code in two places.

I did duplicate the chunked writer, though, and didn't feel bad about that
because it was both correct and tiny.  The chunked reader isn't as tiny or
trivial.

[rsc, 2011-11-07 17:58:59]

On Mon, Nov 7, 2011 at 12:55, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Well, until my recent trailers CL, the chunked reader in http was wrong.  I
> especially didn't want to duplicate wrong code in two places.
> I did duplicate the chunked writer, though, and didn't feel bad about that
> because it was both correct and tiny.  The chunked reader isn't as tiny or
> trivial.

I only really care about the API: I'd like a solution that doesn't
involve a new package.  Whether it copies code or finds a way
to reuse http doesn't bother me.

Russ

[adg, 2011-11-07 20:38:46]

What is your objection to the new package? Packages are not expensive and
the chunked reader and writer have no dependencies.

This is what packages are for. This change resolves an awkward kludge and
removes >100 lines of duplicated code. What's not to like?

On Tuesday, 8 November 2011, Russ Cox <rsc@golang.org> wrote:
> On Mon, Nov 7, 2011 at 12:55, Brad Fitzpatrick <bradfitz@golang.org>
wrote:
>> Well, until my recent trailers CL, the chunked reader in http was wrong.
 I
>> especially didn't want to duplicate wrong code in two places.
>> I did duplicate the chunked writer, though, and didn't feel bad about
that
>> because it was both correct and tiny.  The chunked reader isn't as tiny
or
>> trivial.
>
> I only really care about the API: I'd like a solution that doesn't
> involve a new package.  Whether it copies code or finds a way
> to reuse http doesn't bother me.
>
> Russ
>

[rsc, 2011-11-07 21:57:40]

On Mon, Nov 7, 2011 at 15:38, Andrew Gerrand <adg@golang.org> wrote:
> This is what packages are for. This change resolves an awkward kludge and
> removes >100 lines of duplicated code. What's not to like?

Packages are for holding code and APIs that we want to support.
http/httputil is where all the low-level 'you probably don't need
to know about this' http stuff is, stuck in one package.
I do not want to have a separate package for every aspect of
http, and that is the road that this CL starts us down.

100 lines of duplicate code is hardly a sin.
Everyone talks about DRY like it is some inviolate commandment.
Don't repeat yourself too often or too much, but repeating yourself
once in a while is fine.  Creating a new package should be done
because there is an API we want to support, not to avoid duplicating
a page of code.

Russ

[adg, 2011-11-07 23:30:52]

Hello golang-dev@googlegroups.com, mikioh.mikioh@gmail.com, bradfitz@golang.org,
andybalholm@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[mikio, 2011-11-08 22:43:01]

LGTM

On 2011/11/07 23:30:52, adg wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:mikioh.mikioh@gmail.com,
mailto:bradfitz@golang.org,
> mailto:andybalholm@gmail.com, mailto:rsc@golang.org (cc:
mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

[adg, 2011-11-09 03:56:01]

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

http: make httputil's chunked reader/writer code a direct copy

Arrange the code so that it's easier to keep edits in sync.

R=golang-dev, mikioh.mikioh, bradfitz, andybalholm, rsc
CC=golang-dev
http://codereview.appspot.com/5345041

[bradfitz, 2011-11-09 04:00:51]

*shrug*

I liked it more as it was, but whatever.


On Tue, Nov 8, 2011 at 7:56 PM, <adg@golang.org> wrote:

> *** Submitted as
>
http://code.google.com/p/go/**source/detail?r=0ca985c8186d<http://code.google...
>
> http: make httputil's chunked reader/writer code a direct copy
>
> Arrange the code so that it's easier to keep edits in sync.
>
> R=golang-dev, mikioh.mikioh, bradfitz, andybalholm, rsc
> CC=golang-dev
> http://codereview.appspot.com/**5345041<http://codereview.appspot.com/5345041>
>
>
>
http://codereview.appspot.com/**5345041/<http://codereview.appspot.com/5345041/>
>