code review 206050: Added routines that dump the wire representation of htt...

Added routines that dump the wire representation of http.Request/Response
without affecting the underlying objects. I find myself using these routines
constantly while debugging.

[petar-m, 2010-02-09 20:56:31]

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

I'd like you to review this change.

[rsc, 2010-02-09 21:06:00]

Would it work just as well to use
testing/iotest's NewReadLogger and NewWriteLogger instead?
Maybe with the output changed from hexadecimal to
quoted string?

Russ

[petar_csail.mit.edu, 2010-02-10 19:33:58]

On Tue, Feb 9, 2010 at 4:05 PM, Russ Cox <rsc@golang.org> wrote:
> Would it work just as well to use
> testing/iotest's NewReadLogger and NewWriteLogger instead?

(1) This would imply that you want to print the Request as it is being sent
over the wire. So you wouldn't be able to print it ahead of time. This
may or may not be undesirable, it's a question of taste. But it may be
problematic because in an asynchronous client e.g. if two requests
are being written to the wire at the same time, they will be interlaced
on the logging output.

> Maybe with the output changed from hexadecimal to
> quoted string?

(2) Changing from hex output to text may involve as much code
as the current implementation of dump.go, no?

>
> Russ
>

What do you think?

--Petar

[rsc, 2010-02-10 19:37:09]

What's a typical use case?
Is this for debugging http itself or debugging
some code that is using http?

Russ

[petar_csail.mit.edu, 2010-02-10 19:42:15]

Right now, my end-test for all HTTP-stuff is
an evolving asychronous transparent proxy. (It is also
the gold-standard, in my opinion, for making
sure all HTTP code works and is mature.)

When a request or response arrives at the proxy
and is ready to be sent out, it is printed first
to make sure all fields are modified or received
correctly.

Often, I don't want to look at the body, but sometimes
I do. That's another feature that NewWriteLogger wouldn't
have.

--Petar


On Wed, Feb 10, 2010 at 2:37 PM, Russ Cox <rsc@golang.org> wrote:
> What's a typical use case?
> Is this for debugging http itself or debugging
> some code that is using http?
>
> Russ
>

[rsc, 2010-02-19 07:49:53]

looks pretty good.  a few small things.

http://codereview.appspot.com/206050/diff/1002/3
File src/pkg/http/dump.go (right):

http://codereview.appspot.com/206050/diff/1002/3#newcode19
src/pkg/http/dump.go:19: b1 := bytes.NewBuffer(nil)
This is a pretty neat trick.  The Reader lets you do some
neat things here.  Very nice.

I think you can simplify the code and avoid the copy.

var buf bytes.Buffer
if _, err = buf.ReadFrom(b); err != nil {
    return nil, nil, err
}
if err = b.Close(); err != nil {
    return nil, nil, err
}
return nopCloser{&buf}, nopCloser{bytes.NewBuffer(buf.Bytes())}, nil

http://codereview.appspot.com/206050/diff/1002/3#newcode49
src/pkg/http/dump.go:49: // Outputs a string with the wire representation of the
request. The body is
// DumpRequest returns the wire representation of req,
// optionally including the request body, for debugging.
// DumpRequest is semantically a no-op, but in order to
// dump the body, it reads the body data into memory and
// changes req.Body to refer to the in-memory copy.

http://codereview.appspot.com/206050/diff/1002/3#newcode53
src/pkg/http/dump.go:53: func DumpRequest(req *Request, body bool) (dump string,
err os.Error) {
You might find it more useful to return []byte and let the
caller do the string conversion.  It's easier to Write a []byte,
for example.

http://codereview.appspot.com/206050/diff/1002/3#newcode54
src/pkg/http/dump.go:54: b := bytes.NewBuffer(make([]byte, 0, 4096))
var b bytes.Buffer
is sufficient

http://codereview.appspot.com/206050/diff/1002/3#newcode73
src/pkg/http/dump.go:73: // Similar to DumpRequest.
// DumpResponse is like DumpRequest but dumps a response.

http://codereview.appspot.com/206050/diff/1002/3#newcode75
src/pkg/http/dump.go:75: b := bytes.NewBuffer(make([]byte, 0, 4096))
var b bytes.Buffer

[petar-m, 2010-02-19 14:15:53]

Done. I forgot to add dump.go to the makefile,
but I am going to wrap this change in the next CL,
which adds other new files.

http://codereview.appspot.com/206050/diff/1002/3
File src/pkg/http/dump.go (right):

http://codereview.appspot.com/206050/diff/1002/3#newcode19
src/pkg/http/dump.go:19: b1 := bytes.NewBuffer(nil)
Oh, cool. Done.

On 2010/02/19 07:49:53, rsc wrote:
> This is a pretty neat trick.  The Reader lets you do some
> neat things here.  Very nice.
> 
> I think you can simplify the code and avoid the copy.
> 
> var buf bytes.Buffer
> if _, err = buf.ReadFrom(b); err != nil {
>     return nil, nil, err
> }
> if err = b.Close(); err != nil {
>     return nil, nil, err
> }
> return nopCloser{&buf}, nopCloser{bytes.NewBuffer(buf.Bytes())}, nil
>

http://codereview.appspot.com/206050/diff/1002/3#newcode49
src/pkg/http/dump.go:49: // Outputs a string with the wire representation of the
request. The body is
On 2010/02/19 07:49:53, rsc wrote:
> // DumpRequest returns the wire representation of req,
> // optionally including the request body, for debugging.
> // DumpRequest is semantically a no-op, but in order to
> // dump the body, it reads the body data into memory and
> // changes req.Body to refer to the in-memory copy.
> 

Done.

http://codereview.appspot.com/206050/diff/1002/3#newcode53
src/pkg/http/dump.go:53: func DumpRequest(req *Request, body bool) (dump string,
err os.Error) {
On 2010/02/19 07:49:53, rsc wrote:
> You might find it more useful to return []byte and let the
> caller do the string conversion.  It's easier to Write a []byte,
> for example.
> 

Done.

http://codereview.appspot.com/206050/diff/1002/3#newcode54
src/pkg/http/dump.go:54: b := bytes.NewBuffer(make([]byte, 0, 4096))
On 2010/02/19 07:49:53, rsc wrote:
> var b bytes.Buffer
> is sufficient
> 

Done.

http://codereview.appspot.com/206050/diff/1002/3#newcode73
src/pkg/http/dump.go:73: // Similar to DumpRequest.
On 2010/02/19 07:49:53, rsc wrote:
> // DumpResponse is like DumpRequest but dumps a response.
> 

Done.

http://codereview.appspot.com/206050/diff/1002/3#newcode75
src/pkg/http/dump.go:75: b := bytes.NewBuffer(make([]byte, 0, 4096))
On 2010/02/19 07:49:53, rsc wrote:
> var b bytes.Buffer
> 

Done.

[rsc, 2010-02-19 15:51:54]

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

http: add DumpRequest, DumpResponse, for debugging

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

Committer: Russ Cox <rsc@golang.org>

[rsc, 2010-02-19 15:51:56]

LGTM

I'll throw the makefile line in before submitting,
just to keep everything building.