net/http: document that conn.Hijack might have left-over read/write timeouts on net.Conn

[encryptio]

What does 'go version' print?

Found with go-1.2rc3, present in go-1.3, and still present in the default branch as of
20272:90616fa61ef4.

What steps reproduce the problem?

Setting a ReadTimeout or WriteTimeout on an http.Server and then hijacking a http.conn
(passed as the ResponseWriter) from a request handler leaves the deadlines set on the
connection.

This causes unexpected read and write behavior, requiring an unnatural removal of the
deadlines after hijacking. I found this while implementing the server half of the HTML5
EventSource API, and it also affects any other hijack users (e.g. go.net's websocket
package does not have code to clear deadlines when it takes over a connection; I have
not tested it though.)

A simplified example of the issue is here (probably useful to derive a regression test
from): http://play.golang.org/p/bRyjSayMJN

Adding these two lines fixes the issue:

diff -r 90616fa61ef4 src/pkg/net/http/server.go
--- a/src/pkg/net/http/server.go    Fri Jun 27 18:30:09 2014 -0700
+++ b/src/pkg/net/http/server.go    Fri Jun 27 22:43:18 2014 -0700
@@ -128,18 +128,20 @@
 func (c *conn) hijack() (rwc net.Conn, buf *bufio.ReadWriter, err error) {
    c.mu.Lock()
    defer c.mu.Unlock()
    if c.hijackedv {
        return nil, nil, ErrHijacked
    }
    if c.closeNotifyc != nil {
        return nil, nil, errors.New("http: Hijack is incompatible with use of CloseNotifier")
    }
+   c.rwc.SetReadDeadline(time.Time{})
+   c.rwc.SetWriteDeadline(time.Time{})
    c.hijackedv = true
    rwc = c.rwc
    buf = c.buf
    c.rwc = nil
    c.buf = nil
    c.setState(rwc, StateHijacked)
    return
 }

[bradfitz]

Comment 1:

If this has always been like this (which I imagine it has), then this is just a
documentation issue. We can't change the behavior in case people were relying on these
still being set. That's odd, but it'd be surprising for us to change it.

Status changed to Accepted.

[encryptio]

Comment 2:

I disagree; this behavior is causing bugs in existing code due to it being unexpected,
and even if documented, would still be unexpected and would cause bugs to be written.
Both notable websocket libraries do not handle this case and thus have bugs in them:
- https://github.com/gorilla/websocket/blob/master/server.go#L100
- https://code.google.com/p/go/source/browse/websocket/server.go?repo=net#14
It's hard to search for other uses of the API; GitHub doesn't list any results for
"hijacker" in Go files.
I checked the release history:
- Go 1.0 has my bug. It also appears to have another fairly obvious bug related to
timeouts; it sets deadlines on Accept, and never sets them again.
- Go 1.1 has my bug. It also *changes this behavior* from Go 1.0 and sets deadlines on
Accept *and* on the start of a new request; this fixes a bug with long-lived http
connections and affects hijackers.
- Go 1.2 has my bug. Nothing changed with regard to deadlines.
- Go 1.3 has my bug. Nothing changed with regard to deadlines.
There's already precedent for changing this behavior to less buggy and more expected
behavior (go1.1 changed it.) Why not this time as well?
Is the implementation more important than the documentation? This is undocumented at the
moment (in both ways, it neither defines the bug to exist or not to exist.) Changing the
docs to match the implementation is an acceptable way to do things (see Perl5), but
personally I'd rather see Go accumulate as little API cruft as possible. People will
still likely write bugs expecting it to be more sane if this remains, even if documented.
That said, I don't know of a way to "go fix" this in user code, so it would need a line
of errata in the release notes.

[griesemer]

Comment 3:

Labels changed: added repo-main.

[lucabrunox]

I got hit by this bug as well. Have to see how I can implement a working request timeout by myself.

[rsc]

I agree with Brad - let's document this.

[gopherbot]

CL https://golang.org/cl/11411 mentions this issue.