code review 86660043: net/http: document, test, define, clean up Request.Trailer

net/http: document, test, define, clean up Request.Trailer

Go's had pretty decent HTTP Trailer support for a long time, but
the docs have been largely non-existent. Fix that.

In the process, re-learn the Trailer code, clean some stuff
up, add some error checks, remove some TODOs, fix a minor bug
or two, and add tests.

[bradfitz, 2014-04-10 20:57:36]

Hello golang-codereviews@googlegroups.com (cc: adg@golang.org,
dsymonds@golang.org, rsc@golang.org),

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

[adg, 2014-04-10 23:08:38]

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/request.go
File src/pkg/net/http/request.go (right):

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/request.g...
src/pkg/net/http/request.go:185: // The trailer contains any additional headers
that are sent
Trailer specifies additional headers that are sent after the request body.

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/request.g...
src/pkg/net/http/request.go:187: // keys and values are sent at different times.
The keys of
the keys are available initially? what does that mean? are they sent to the
server first?

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/request.g...
src/pkg/net/http/request.go:200: // values may be nil or their final values.
Additionally, the
s/Additionally, t/T/

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/request.g...
src/pkg/net/http/request.go:204: // returns EOF, the caller must stop mutating
Trailer.
s/stop mutating/not mutate/

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/request.g...
src/pkg/net/http/request.go:206: // Be aware that very few HTTP clients,
servers, or proxies
s/Be aware that very f/F/

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/transfer.go
File src/pkg/net/http/transfer.go (right):

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/transfer....
src/pkg/net/http/transfer.go:181: // TODO: could do better allocation-wise here,
but trailers are rare,
TODO(bradfitz)

[bradfitz, 2014-04-10 23:14:36]

LG after I make the changes below?

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/request.go
File src/pkg/net/http/request.go (right):

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/request.g...
src/pkg/net/http/request.go:187: // keys and values are sent at different times.
The keys of
On 2014/04/10 23:08:38, adg wrote:
> the keys are available initially? what does that mean? are they sent to the
> server first?

Yes. In the Trailer (main) header. It's a "FYI gonna send these ones later heads
up".

[adg, 2014-04-10 23:26:34]

I'd like to have a look at the revised wording before you submit, thanks.

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/request.go
File src/pkg/net/http/request.go (right):

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/request.g...
src/pkg/net/http/request.go:187: // keys and values are sent at different times.
The keys of
On 2014/04/10 23:14:36, bradfitz wrote:
> On 2014/04/10 23:08:38, adg wrote:
> > the keys are available initially? what does that mean? are they sent to the
> > server first?
> 
> Yes. In the Trailer (main) header. It's a "FYI gonna send these ones later
heads
> up".

I think you should probably cut down the first paragraph as much of it is
repeated in the next paragraph.

[bradfitz, 2014-04-10 23:47:43]

PTAL

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/request.go
File src/pkg/net/http/request.go (right):

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/request.g...
src/pkg/net/http/request.go:185: // The trailer contains any additional headers
that are sent
On 2014/04/10 23:08:38, adg wrote:
> Trailer specifies additional headers that are sent after the request body.

Done.

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/request.g...
src/pkg/net/http/request.go:200: // values may be nil or their final values.
Additionally, the
On 2014/04/10 23:08:38, adg wrote:
> s/Additionally, t/T/

Done.

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/request.g...
src/pkg/net/http/request.go:204: // returns EOF, the caller must stop mutating
Trailer.
On 2014/04/10 23:08:38, adg wrote:
> s/stop mutating/not mutate/

Done.

https://codereview.appspot.com/86660043/diff/40001/src/pkg/net/http/request.g...
src/pkg/net/http/request.go:206: // Be aware that very few HTTP clients,
servers, or proxies
On 2014/04/10 23:08:38, adg wrote:
> s/Be aware that very f/F/

Done.

[bradfitz, 2014-04-10 23:47:53]

Hello golang-codereviews@googlegroups.com, adg@golang.org (cc:
dsymonds@golang.org, golang-codereviews@googlegroups.com, rsc@golang.org),

Please take another look.

[adg, 2014-04-10 23:52:00]

LGTM

[bradfitz, 2014-04-11 00:01:25]

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

net/http: document, test, define, clean up Request.Trailer

Go's had pretty decent HTTP Trailer support for a long time, but
the docs have been largely non-existent. Fix that.

In the process, re-learn the Trailer code, clean some stuff
up, add some error checks, remove some TODOs, fix a minor bug
or two, and add tests.

LGTM=adg
R=golang-codereviews, adg
CC=dsymonds, golang-codereviews, rsc
https://codereview.appspot.com/86660043