Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
encryptio opened this issue Jun 28, 2014 · 6 comments
Closed

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

encryptio opened this issue Jun 28, 2014 · 6 comments
Labels
Documentation FrozenDueToAge
Milestone
Go1.5

Comments

@encryptio
Copy link
Contributor 

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

Comment 3:

Labels changed: added repo-main.

@lucabrunox
Copy link

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

@mikioh mikioh mentioned this issue Feb 8, 2015
Closed
@rsc
Copy link
Contributor

rsc commented Apr 10, 2015

I agree with Brad - let's document this.

@rsc rsc changed the title net/http: conn.Hijack does not remove deadlines set from the use of server.{Read,Write}Timeout net/http: document that conn.Hijack might have left-over read/write timeouts on net.Conn Apr 10, 2015
@rsc rsc added this to the Go1.5 milestone Apr 10, 2015
@rsc rsc removed accepted labels Apr 14, 2015
@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Jun 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge
Projects
None yet
Milestone
Go1.5
Development

No branches or pull requests
6 participants
@bradfitz @encryptio @rsc @lucabrunox @griesemer @gopherbot