code review 7220076: net/http: fix when server deadlines get extended

net/http: fix when server deadlines get extended

Deadlines should be extended at the beginning of
a request, not at the beginning of a connection.

Fixes issue 4676

[bradfitz, 2013-02-01 21:44:57]

Hello golang-dev@googlegroups.com,

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

[albert.strasheim, 2013-02-01 21:52:49]

Howdy

I suspect there might be a bug in this area. I asked about it on golang-nuts the
other day, but no word there:

https://groups.google.com/d/topic/golang-nuts/vBvpMSJeezc/discussion

In short, setting a WriteTimeout without a ReadTimeout doesn't work.

Code here:

https://gist.github.com/4640746

Cheers

Albert

[bradfitz, 2013-02-01 22:02:30]

That's a different bug, then.  This fixes a real bug, as shown in the test.


On Fri, Feb 1, 2013 at 1:52 PM, <fullung@gmail.com> wrote:

> Howdy
>
> I suspect there might be a bug in this area. I asked about it on
> golang-nuts the other day, but no word there:
>
>
https://groups.google.com/d/**topic/golang-nuts/vBvpMSJeezc/**discussion<http...
>
> In short, setting a WriteTimeout without a ReadTimeout doesn't work.
>
> Code here:
>
> https://gist.github.com/**4640746 <https://gist.github.com/4640746>
>
> Cheers
>
> Albert
>
>
https://codereview.appspot.**com/7220076/<https://codereview.appspot.com/7220...
>

[bradfitz, 2013-02-01 22:06:43]

On Fri, Feb 1, 2013 at 1:52 PM, <fullung@gmail.com> wrote:

> Howdy
>
> I suspect there might be a bug in this area. I asked about it on
> golang-nuts the other day, but no word there:
>
>
https://groups.google.com/d/**topic/golang-nuts/vBvpMSJeezc/**discussion<http...


HTTP handlers don't immediately flush to the client.  There's various
amounts of buffering.

[albert.strasheim, 2013-02-01 22:09:49]

Hello

On Sat, Feb 2, 2013 at 12:06 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> On Fri, Feb 1, 2013 at 1:52 PM, <fullung@gmail.com> wrote:
>> I suspect there might be a bug in this area. I asked about it on
>> golang-nuts the other day, but no word there:
>> https://groups.google.com/d/topic/golang-nuts/vBvpMSJeezc/discussion
> HTTP handlers don't immediately flush to the client.  There's various
> amounts of buffering.

I'm pretty sure it gets stuck forever waiting for the read part to shut down.

Run the server, do a request and then SIGQUIT it after a while.

I've filed http://code.google.com/p/go/issues/detail?id=4741

Cheers

Albert

[bradfitz, 2013-02-01 22:14:10]

On Fri, Feb 1, 2013 at 2:09 PM, Albert Strasheim <fullung@gmail.com> wrote:

> Hello
>
> On Sat, Feb 2, 2013 at 12:06 AM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > On Fri, Feb 1, 2013 at 1:52 PM, <fullung@gmail.com> wrote:
> >> I suspect there might be a bug in this area. I asked about it on
> >> golang-nuts the other day, but no word there:
> >> https://groups.google.com/d/topic/golang-nuts/vBvpMSJeezc/discussion
> > HTTP handlers don't immediately flush to the client.  There's various
> > amounts of buffering.
>
> I'm pretty sure it gets stuck forever waiting for the read part to shut
> down.
>
> Run the server, do a request and then SIGQUIT it after a while.
>
> I've filed http://code.google.com/p/go/issues/detail?id=4741
>
>
Can you write a minimal test?

The current gist is like a "my program doesn't work", but I'm not exactly
sure what you're trying to illustrate.

Like, something without a "chanBreaker" or http.CloseNotifier or a "gg".

[pah, 2013-02-01 23:13:08]

On 2013/02/01 21:44:57, bradfitz wrote:
> Hello mailto:golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

I like this. I hesitate on whether a separate idle timeout is needed. The
existing read timeout makes sense on the first request, but on subsequent
requests maybe a larger idle timeout should apply? Perhaps after the first byte
of a subsequent request is received, the idle timeout should be replaced with
the read timeout?

[bradfitz, 2013-02-02 01:17:16]

On Fri, Feb 1, 2013 at 3:13 PM, <patrick.allen.higgins@gmail.com> wrote:

> On 2013/02/01 21:44:57, bradfitz wrote:
>
>> Hello mailto:golang-dev@**googlegroups.com <golang-dev@googlegroups.com>,
>>
>
>  I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>
> I like this. I hesitate on whether a separate idle timeout is needed.
> The existing read timeout makes sense on the first request, but on
> subsequent requests maybe a larger idle timeout should apply? Perhaps
> after the first byte of a subsequent request is received, the idle
> timeout should be replaced with the read timeout?
>

Replaced with? There is no idle timeout, so how can it be replaced?

"1 byte" doesn't typically matter: the HTTP request headers almost always
arrive in one packet.

[pah, 2013-02-02 01:48:11]

On 2013/02/02 01:17:16, bradfitz wrote:
> On Fri, Feb 1, 2013 at 3:13 PM, <mailto:patrick.allen.higgins@gmail.com>
wrote:
> 
> > On 2013/02/01 21:44:57, bradfitz wrote:
> >
> >> Hello mailto:golang-dev@**googlegroups.com
<mailto:golang-dev@googlegroups.com>,
> >>
> >
> >  I'd like you to review this change to
> >> https://go.googlecode.com/hg/
> >>
> >
> > I like this. I hesitate on whether a separate idle timeout is needed.
> > The existing read timeout makes sense on the first request, but on
> > subsequent requests maybe a larger idle timeout should apply? Perhaps
> > after the first byte of a subsequent request is received, the idle
> > timeout should be replaced with the read timeout?
> >
> 
> Replaced with? There is no idle timeout, so how can it be replaced?

I propose an IdleTimeout be added, though I'm not sure it's really needed.

Imagine ReadTimeout is 5s, IdleTimeout is 10s. Now imagine 9.5s after a response
is written, a packet arrives. The deadline is reset so that the rest of the
request must be read within 5s.

> "1 byte" doesn't typically matter: the HTTP request headers almost always
> arrive in one packet.

Understood. I am using the "time to first byte" terminology, even though data
arrives in packets.

The read timeout applies to the request body as well as headers, right?

[adg, 2013-02-04 02:58:51]

Looks pretty good overall

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

https://codereview.appspot.com/7220076/diff/2001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:425: c.rwc.SetWriteDeadline(time.Now().Add(d))
I don't understand why this needs to be deferred, and why it is being set here.
Does this function even write?

[bradfitz, 2013-02-04 17:25:43]

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

https://codereview.appspot.com/7220076/diff/2001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:425: c.rwc.SetWriteDeadline(time.Now().Add(d))
On 2013/02/04 02:58:51, adg wrote:
> I don't understand why this needs to be deferred, and why it is being set
here.
> Does this function even write?

Because the http Handler will be writing, which happens after this function
returns.

I'm using a defer here to avoid writing
c.rwc.SetWriteDeadline(time.Now().Add(d)) at both return paths in this function
(even on the error path we'll be writing to the client's net.Conn, to tell them
Bad Request or whatever).

And I used a closure to defer because I didn't want to evaluate time.Now() until
it's actually returning, since it may take awhile to read the request.

[adg, 2013-02-04 21:45:15]

LGTM

[bradfitz, 2013-02-04 21:52:48]

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

net/http: fix when server deadlines get extended

Deadlines should be extended at the beginning of
a request, not at the beginning of a connection.

Fixes issue 4676

R=golang-dev, fullung, patrick.allen.higgins, adg
CC=golang-dev
https://codereview.appspot.com/7220076