code review 12583043: net/http: treat HEAD requests like GET requests

net/http: treat HEAD requests like GET requests

A response to a HEAD request is supposed to look the same as a
response to a GET request, just without a body.

HEAD requests are incredibly rare in the wild.

The Go net/http package has so far treated HEAD requests
specially: a Write on our default ResponseWriter returned
ErrBodyNotAllowed, telling handlers that something was wrong.
This was to optimize the fast path for HEAD requests, but:

1) because HEAD requests are incredibly rare, they're not
   worth having a fast path for.

2) Letting the http.Handler handle but do nop Writes is still
   very fast.

3) this forces ugly error handling into the application.
   e.g. https://code.google.com/p/go/source/detail?r=6f596be7a31e
   and related.

4) The net/http package nowadays does Content-Type sniffing,
   but you don't get that for HEAD.

5) The net/http package nowadays does Content-Length counting
   for small (few KB) responses, but not for HEAD.

6) ErrBodyNotAllowed was useless. By the time you received it,
   you had probably already done all your heavy computation
   and I/O to calculate what to write.

So, this change makes HEAD requests like GET requests.

We now count content-length and sniff content-type for HEAD
requests. If you Write, it doesn't return an error.

If you want a fast-path in your code for HEAD, you have to do
it early and set all the response headers yourself. Just like
before. If you choose not to Write in HEAD requests, be sure
to set Content-Length if you know it. We won't write
"Content-Length: 0" because you might've just chosen to not
write (or you don't know your Content-Length in advance).

Fixes issue 5454

[bradfitz, 2013-08-07 00:52:12]

Hello golang-dev@googlegroups.com,

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

[dsymonds, 2013-08-07 01:01:47]

LGTM

I'm a little rusty on this code these days, but it looks sane. Just one place
that raised my eyebrows; it could just be that I'm misunderstanding it.

https://codereview.appspot.com/12583043/diff/4001/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/12583043/diff/4001/src/pkg/net/http/server.go#...
src/pkg/net/http/server.go:743: if w.handlerDone && header.get("Content-Length")
== "" && (!isHEAD || len(p) > 0) {
doesn't this trigger for HEAD responses if a Write happens? chunkWriter.Write
will call this function with that data, but we don't want a Content-Length
header written, no?

[bradfitz, 2013-08-07 01:06:50]

On Tue, Aug 6, 2013 at 6:01 PM, <dsymonds@golang.org> wrote:

> LGTM
>
> I'm a little rusty on this code these days, but it looks sane. Just one
> place that raised my eyebrows; it could just be that I'm
> misunderstanding it.
>
>
> https://codereview.appspot.**com/12583043/diff/4001/src/**
>
pkg/net/http/server.go<https://codereview.appspot.com/12583043/diff/4001/src/pkg/net/http/server.go>
> File src/pkg/net/http/server.go (right):
>
> https://codereview.appspot.**com/12583043/diff/4001/src/**
>
pkg/net/http/server.go#**newcode743<https://codereview.appspot.com/12583043/diff/4001/src/pkg/net/http/server.go#newcode743>
> src/pkg/net/http/server.go:**743: if w.handlerDone &&
>
> header.get("Content-Length") == "" && (!isHEAD || len(p) > 0) {
> doesn't this trigger for HEAD responses if a Write happens?
> chunkWriter.Write will call this function with that data, but we don't
> want a Content-Length header written, no?
>

We do want a Content-Length header written when responding to HEAD.

But we just can't write "Content-Length: 0", because we don't know the
difference between a Handler writing nothing (a real Content-Length: 0) or
the handler being clever and choosing to not write.  So we keep it
ambiguous in that case.

So this means with both GET and HEAD, if you Write <=
bufferBeforeChunkingSize (2k currently) but don't set a Content-Length
header, we send one for you.

[bradfitz, 2013-08-07 01:07:21]

On Tue, Aug 6, 2013 at 6:06 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

>
>
>
> On Tue, Aug 6, 2013 at 6:01 PM, <dsymonds@golang.org> wrote:
>
>> LGTM
>>
>> I'm a little rusty on this code these days, but it looks sane. Just one
>> place that raised my eyebrows; it could just be that I'm
>> misunderstanding it.
>>
>>
>> https://codereview.appspot.**com/12583043/diff/4001/src/**
>>
pkg/net/http/server.go<https://codereview.appspot.com/12583043/diff/4001/src/pkg/net/http/server.go>
>> File src/pkg/net/http/server.go (right):
>>
>> https://codereview.appspot.**com/12583043/diff/4001/src/**
>>
pkg/net/http/server.go#**newcode743<https://codereview.appspot.com/12583043/diff/4001/src/pkg/net/http/server.go#newcode743>
>> src/pkg/net/http/server.go:**743: if w.handlerDone &&
>>
>> header.get("Content-Length") == "" && (!isHEAD || len(p) > 0) {
>> doesn't this trigger for HEAD responses if a Write happens?
>> chunkWriter.Write will call this function with that data, but we don't
>> want a Content-Length header written, no?
>>
>
> We do want a Content-Length header written when responding to HEAD.
>
> But we just can't write "Content-Length: 0", because we don't know the
> difference between a Handler writing nothing (a real Content-Length: 0) or
> the handler being clever and choosing to not write.  So we keep it
> ambiguous in that case.
>
> So this means with both GET and HEAD, if you Write <=
> bufferBeforeChunkingSize (2k currently) but don't set a Content-Length
> header, we send one for you.
>

... and this all covered by tests.

[dsymonds, 2013-08-07 01:18:18]

On 7 August 2013 11:06, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> We do want a Content-Length header written when responding to HEAD.

Why is that so important? If it's a handler that writes a large chunk
of data in dribs and drabs (e.g. text/template), grabbing the first
chunk for sniffing is okay, but it looks like this code uses the size
of the first chunk as a Content-Length. Isn't a wrong value worse than
no value, even more so for HEAD than for GET?

[bradfitz, 2013-08-07 01:29:03]

On Tue, Aug 6, 2013 at 6:18 PM, David Symonds <dsymonds@golang.org> wrote:

> On 7 August 2013 11:06, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
> > We do want a Content-Length header written when responding to HEAD.
>
> Why is that so important?


The whole point of this CL is to make the answers to HEAD requests look the
same as answers to GET requests, if a handler isn't checking for HEAD
(which almost none do).

This makes the behavior the same.  This is what we do for GET.


> If it's a handler that writes a large chunk
> of data in dribs and drabs (e.g. text/template), grabbing the first
> chunk for sniffing is okay, but it looks like this code uses the size
> of the first chunk as a Content-Length. Isn't a wrong value worse than
> no value, even more so for HEAD than for GET?
>

We do it correctly for GET and HEAD.

See the comment above that line:

        // *If the handler is done* but never sent a Content-Length
        // response header and *this is our first (and last) write*, set
        // it, even to zero. This helps HTTP/1.0 clients keep their
        // "keep-alive" connections alive.
        if *w.handlerDone* && header.get("Content-Length") == "" &&
(!isHEAD || len(p) > 0) {

[bradfitz, 2013-08-07 01:33:08]

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

net/http: treat HEAD requests like GET requests

A response to a HEAD request is supposed to look the same as a
response to a GET request, just without a body.

HEAD requests are incredibly rare in the wild.

The Go net/http package has so far treated HEAD requests
specially: a Write on our default ResponseWriter returned
ErrBodyNotAllowed, telling handlers that something was wrong.
This was to optimize the fast path for HEAD requests, but:

1) because HEAD requests are incredibly rare, they're not
   worth having a fast path for.

2) Letting the http.Handler handle but do nop Writes is still
   very fast.

3) this forces ugly error handling into the application.
   e.g. https://code.google.com/p/go/source/detail?r=6f596be7a31e
   and related.

4) The net/http package nowadays does Content-Type sniffing,
   but you don't get that for HEAD.

5) The net/http package nowadays does Content-Length counting
   for small (few KB) responses, but not for HEAD.

6) ErrBodyNotAllowed was useless. By the time you received it,
   you had probably already done all your heavy computation
   and I/O to calculate what to write.

So, this change makes HEAD requests like GET requests.

We now count content-length and sniff content-type for HEAD
requests. If you Write, it doesn't return an error.

If you want a fast-path in your code for HEAD, you have to do
it early and set all the response headers yourself. Just like
before. If you choose not to Write in HEAD requests, be sure
to set Content-Length if you know it. We won't write
"Content-Length: 0" because you might've just chosen to not
write (or you don't know your Content-Length in advance).

Fixes issue 5454

R=golang-dev, dsymonds
CC=golang-dev
https://codereview.appspot.com/12583043

[dsymonds, 2013-08-07 01:53:01]

LGTM