code review 69860049: net/http: fix location of StateHijacked and StateActive

net/http: fix location of StateHijacked and StateActive

1) Move StateHijacked callback earlier, to make it
panic-proof.  A Hijack followed by a panic didn't previously
result in ConnState getting fired for StateHijacked.  Move it
earlier, to the time of hijack.

2) Don't fire StateActive unless any bytes were read off the
wire while waiting for a request. This means we don't
transition from New or Idle to Active if the client
disconnects or times out. This was documented before, but not
implemented properly.

This CL is required for an pending fix for Issue 7264

[bradfitz, 2014-03-03 21:13:05]

Hello josharian@gmail.com (cc: golang-codereviews@googlegroups.com),

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

[bradfitz, 2014-03-03 21:47:37]

Actually I'll add a specific test for this.

I also found another bad example.




On Mon, Mar 3, 2014 at 1:13 PM, <bradfitz@golang.org> wrote:

> Reviewers: josharian,
>
> Message:
> Hello josharian@gmail.com (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net/http: move StateHijacked callback earlier, to make it panic-proof
>
> A Hijack followed by a panic didn't previously result
> in ConnState getting fired for StateHijacked.
>
> Move it earlier, to the time of hijack.
>
> No test here because an existing test found this
> (TestHandlerPanicWithHijack) combined with a pending fix for
> Issue 7264.
>
> Please review this at https://codereview.appspot.com/69860049/
>
> Affected files (+1, -1 lines):
>   M src/pkg/net/http/server.go
>
>
> Index: src/pkg/net/http/server.go
> ===================================================================
> --- a/src/pkg/net/http/server.go
> +++ b/src/pkg/net/http/server.go
> @@ -139,6 +139,7 @@
>         buf = c.buf
>         c.rwc = nil
>         c.buf = nil
> +       c.setState(rwc, StateHijacked)
>         return
>  }
>
> @@ -1176,7 +1177,6 @@
>                 // in parallel even if their responses need to be
> serialized.
>                 serverHandler{c.server}.ServeHTTP(w, w.req)
>                 if c.hijacked() {
> -                       c.setState(origConn, StateHijacked)
>                         return
>                 }
>                 w.finishRequest()
>
>
>

[bradfitz, 2014-03-03 22:53:30]

Hello josharian@gmail.com (cc: golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-03-03 22:57:02]

Hello josharian@gmail.com (cc: golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-03-03 22:57:42]

(The last version adds the previously-forgotten serve_test.go ....)



On Mon, Mar 3, 2014 at 2:57 PM, <bradfitz@golang.org> wrote:

> Hello josharian@gmail.com (cc: golang-codereviews@googlegroups.com),
>
> Please take another look.
>
>
> https://codereview.appspot.com/69860049/
>

[josharian, 2014-03-04 00:25:31]

LGTM with one nit

I do wonder whether there are any other subtle cases lurking. I can't think of
any, but that doesn't mean much.

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

https://codereview.appspot.com/69860049/diff/70001/src/pkg/net/http/server.go...
src/pkg/net/http/server.go:1149: // TODO(bradfitz): could push this StateActive
earlier
Given that this location is better, either (1) cut this comment or (2) make it
not a TODO and explain the decision for it to be here instead of earlier.

[josharian, 2014-03-04 00:26:50]

Huh, lost one comment during publish.

Nit: s/if the there/if there/ in the CL description section (2).

[bradfitz, 2014-03-04 01:23:28]

Hello josharian@gmail.com (cc: golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-03-04 01:23:50]

PTAL. I just implemented StateActive correctly now, instead of "better".
With new test and updated CL description.



On Mon, Mar 3, 2014 at 4:26 PM, <josharian@gmail.com> wrote:

> Huh, lost one comment during publish.
>
> Nit: s/if the there/if there/ in the CL description section (2).
>
> https://codereview.appspot.com/69860049/
>

[josharian, 2014-03-04 02:34:06]

LGTM

> PTAL. I just implemented StateActive correctly now, instead of "better".
> With new test and updated CL description.

Thanks, much better.

[bradfitz, 2014-03-04 02:58:33]

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

net/http: fix location of StateHijacked and StateActive

1) Move StateHijacked callback earlier, to make it
panic-proof.  A Hijack followed by a panic didn't previously
result in ConnState getting fired for StateHijacked.  Move it
earlier, to the time of hijack.

2) Don't fire StateActive unless any bytes were read off the
wire while waiting for a request. This means we don't
transition from New or Idle to Active if the client
disconnects or times out. This was documented before, but not
implemented properly.

This CL is required for an pending fix for Issue 7264

LGTM=josharian
R=josharian
CC=golang-codereviews
https://codereview.appspot.com/69860049

[gobot, 2014-03-04 03:16:48]

This CL appears to have broken the windows-386-ec2 builder.