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

net/http: Closing Hijacked Conn doesn't cancel Request Context #22347

Closed
anacrolix opened this issue Oct 20, 2017 · 10 comments

Comments

Projects
None yet
5 participants
@anacrolix
Copy link
Contributor

commented Oct 20, 2017

After a server Handler Hijacks and returns the underlying net.Conn, I'd expect the Handler's Request.Context to be Done when that Conn is closed. It appears that the Context is Done when the original handler routine returns, but closing the Conn after a Hijack no longer triggers it too.

Here's a test demonstrating it. It gets stuck at the linked line. https://github.com/anacrolix/wat/blob/master/http_test.go#L63

$ go version
go version devel +2da8a16cbc Thu Oct 19 23:34:02 2017 +0000 darwin/amd64

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 20, 2017

After a server Handler Hijacks and returns the underlying net.Conn, I'd expect the Handler's Request.Context to be Done when that Conn is closed.

We've never documented that guarantee. In fact, I believe we document something close to the opposite: that once you call Hijack, the net/http package is no longer involved at all, which includes doing magic context cancellations behind your back.

Let us know if there's missing or inconsistent documentation, but I don't think we can change behavior here.

/cc @tombergan

@tombergan

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

After a server Handler Hijacks and returns the underlying net.Conn, I'd expect the Handler's Request.Context to be Done when that Conn is closed.

We've never documented that guarantee.

I think we have, actually:

https://golang.org/pkg/net/http/#Request.Context

For incoming server requests, the context is canceled when the client's connection closes, the request is canceled (with HTTP/2), or when the ServeHTTP method returns.

There's no stated exception for hijacked connections so it's reasonable to assume that the above guarantee applies to hijacked connections as well. This would be simple to implement. In conn.hijackLocked, we'd wrap rwc to call cancelCtx() from Close or on error from Read/Write.

The problem is there are users who call Hijack and then typecast the returned net.Conn to *net.TCPConn or *tls.Conn. We haven't documented that this typecast works, but it has worked since the beginning (most likely) and users are relying on it. Wrapping rwc would break those typecasts. Looking through internal google source code, I found at least one major user of Hijack that relies on being able to cast to *net.TCPConn. Outside google code, I notice this. I haven't done a very thorough search ... I'm sure there are other casts like these.

So, we either wrap rwc and break users, or we keep the code as-is and update the documentation. Given the potential breakage, I favor updating the documentation.

If a user wants to cancel req.Context when the connection closes, it's not hard to wrap the returned net.Conn as I described above. Something like:

ctx, cancelCtx := context.WithCancel(req.Context())
req = req.WithContex(ctx)
conn, buff, err := rw.Hijack()
conn = connCancelOnClose{conn, cancelCtx}

type connCancelOnClose struct {  // implements wrappers for Read, Write, Close
  net.Conn
  cancelCtx func()
}
@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 20, 2017

In conn.hijackLocked, we'd wrap rwc to call cancelCtx() from Close or on error from Read/Write.

We can't change the concrete type of the net.Conn returned by Hijack, if that's what you're proposing.

@tombergan

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

We can't change the concrete type of the net.Conn returned by Hijack, if that's what you're proposing.

I proposed that as a straw man, then argued that it's a bad idea. I don't think we can do anything except clarify the documentation for Request.Context().

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 20, 2017

Sorry, once again I failed at reading. Yeah, documentation SGTM.

@tombergan tombergan self-assigned this Oct 24, 2017

@anacrolix

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2017

I appreciate clarification that closing the hijacked connection does not cancel the request context. It's a shame this can't be made the case, I think it's another example of the interface type assertion issue that exists elsewhere. It also seems presumptuous of existing users to assume that the net.Conn interface guarantees a specific type, that's why it's an interface in the first place. Clearly here we want to give the hijacker access to the true connection type with everything it implies (for instance TCPConn), but we need to modify a bunch of the methods. I also think we're unable to widen the conn interface passed to the Hijack method because it will cause compile issues. That wouldn't be a problem if functions automatically allowed interface narrowing (so that existing hijackers would just check that the interface methods they require are present and pass type checking).

@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 29, 2018

@bradfitz bradfitz self-assigned this May 29, 2018

@gopherbot

This comment has been minimized.

Copy link

commented May 29, 2018

Change https://golang.org/cl/115039 mentions this issue: net/http: document how Hijack and Request.Context interact

@gopherbot gopherbot closed this in 3988863 May 29, 2018

@anacrolix

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

I don't believe the Request Context is canceled when Hijack is called. The new documentation seems to suggest this.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 15, 2018

@anacrolix, yeah, thanks for catching that. The docs are wrong. I'll try again.

@bradfitz bradfitz reopened this Jun 15, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Jun 15, 2018

Change https://golang.org/cl/119235 mentions this issue: net/http: document how Hijack and Request.Context interact, take two

@gopherbot gopherbot closed this in c34381a Jun 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.