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: impossible to use both CloseNotify and Hijack in the same Server #9763

Closed
bgentry opened this issue Feb 3, 2015 · 13 comments

Comments

Projects
None yet
8 participants
@bgentry
Copy link
Contributor

commented Feb 3, 2015

The existing implementation makes it impossible to run a Server with keep-alive that both gets notified about disconnecting clients and is capable of handling upgrade requests (such as to websocket protocol).

For example, consider the recent change to utilize CloseNotifier within the httputil.ReverseProxy: ececbe8

That's an important change for a reverse proxy because it allows upstream servers to stop doing work for clients that have gone away. Without that behavior, servers are much more vulnerable to resource exhaustion by misbehaving clients that disconnect immediately after making a request.

However, suppose that a Server not only wanted to defend against disconnecting clients, but also wanted to be able to handle HTTP Upgrade (or websocket) requests. That can only be implemented in http.Server by using the ResponseWriter's Hijacker interface to hijack the underlying net.Conn.

However, those two features are explicitly blocked from being used together. There's a good underlying reason for that: once CloseNotify() is called, there's an io.Copy reading from the underlying net.Conn that cannot be cleanly interrupted.

I think it's important to be able to use both of these features together in the same Server. One could imagine adding websocket or more general HTTP Upgrade support to the ReverseProxy, or simply supporting such features from a Server that also wants to know when clients go away on normal requests.

What version of Go are you using (go version)?

go1.4.1

What operating system and processor architecture are you using?

Darwin amd64

What did you do?

I attempted to Hijack() on a re-used server connection, one that had been setup for CloseNotify() on a previous request.

What did you expect to see?

I expected to be able to hijack the connection, even though in a previous request I had asked to be notified about disconnecting clients.

What did you see instead?

http: Hijack is incompatible with use of CloseNotifier

@bgentry

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2015

I made an attempt to solve this in https://go-review.googlesource.com/#/c/3821/

This change allows connections that have been used with CloseNotify to
also be used with Hijack. It does so by returning a wrapped net.Conn
type that reads from the closeNotify pipe reader, rather than directly
from the net.Conn.

@benburkert

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2015

CloseNotify checks the state of the underlying net.Conn of a conn, so another approach is to check if the underlying net.Conn is itself a CloseNotifier, and if so return the result of calling CloseNotify(). In this case it should be safe to hijack the conn after a CloseNotify because there was no io.Copy goroutine issuing reads to the net.Conn directly.

The necessary change should be quite small and overhead negligible: benburkert@3c2fd2f

@bgentry

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2015

I do like the idea of relying on a CloseNotifier on the net.Conn. Seems like that could be useful elsewhere.

To do that, you'd have to either add CloseNotify() to the built-in net.Conn types like net.TCPConn, or use a custom net.Listener that returns a wrapped net.Conn type which fulfills CloseNotifier.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 25, 2015

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

@pwaller

This comment has been minimized.

Copy link
Contributor

commented May 10, 2015

Just to note that this is causing problems for Docker, too, so it would be really great to get this fixed for 1.5. moby/moby#12845

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2015

I apologize, but I don't believe we'll be able to fix this in time for Go 1.5.

@bgentry

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2015

Any updates on whether there's an approach that would work for this? I would work on my CL further if it's actually an acceptable solution.

@bmizerany

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2015

Are you trying to hijack after closenotify? I.e.

  • do long running http land stuff, maybe hijack?
  • yes: hijack and start doing custom stuff
    On Wed, Oct 28, 2015 at 6:51 PM Blake Gentry notifications@github.com
    wrote:

Any updates on whether there's an approach that would work for this? I
would work on my CL further if it's actually an acceptable solution.


Reply to this email directly or view it on GitHub
#9763 (comment).

@bgentry

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2015

Are you trying to hijack after closenotify?

@bmizerany I think the situation is pretty well described in the original issue. You can't hijack a connection that previously had a CloseNotifier set up on it (even if it was set up on a previous request). Essentially this means that you can't reliably use both CloseNotify and Hijack in the same http.Server (sharing the same client connection pool).

CloseNotify is essential to a production server (protecting it from continuing to work when the client goes away). At the same time, Hijack is required for Websocket or just general HTTP Upgrade support. As a result, it's impossible to run an http.Server that has both production-ready support for regular HTTP requests, and also support for Websockets/HTTP upgrade.

Please read the original issue and let me know if anything is poorly explained ✌️

@bmizerany

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2015

I saw this in email and mistook it for a forum post, not Github, so I didn't get the back story. I was going to suggest building your own CloseNotifierConn, but it looks like you already did. Nevermind.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.6, Go1.6Early Dec 11, 2015

@gopherbot

This comment has been minimized.

Copy link

commented Dec 11, 2015

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 13, 2015

@bgentry, check out the latest version of https://golang.org/cl/17750 (patchset 2+), which has a fix for this bug. Unlike your change, it doesn't returned a custom net.Conn type, so users can still type-assert it to a *net.TCPConn or *tls.Conn, if that matters. Instead of a background io.Copy always running now, we only read a single byte in the background.

@gopherbot

This comment has been minimized.

Copy link

commented Oct 15, 2016

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

gopherbot pushed a commit that referenced this issue Oct 18, 2016

net/http: make Server Handler's Request.Context be done on conn errors
This CL changes how the http1 Server reads from the client.

The goal of this change is to make the Request.Context given to Server
Handlers become done when the TCP connection dies (has seen any read
or write error). I didn't finish that for Go 1.7 when Context was
added to http package.

We can't notice the peer disconnect unless we're blocked in a Read
call, though, and previously we were only doing read calls as needed,
when reading the body or the next request. One exception to that was
the old pre-context CloseNotifier mechanism.

The implementation of CloseNotifier has always been tricky. The past
few releases have contained the complexity and moved the
reading-from-TCP-conn logic into the "connReader" type. This CL
extends connReader to make sure that it's always blocked in a Read
call, at least once the request body has been fully consumed.

In the process, this deletes all the old CloseNotify code and unifies
it with the context cancelation code. The two notification mechanisms
are nearly identical, except the CloseNotify path always notifies on
the arrival of pipelined HTTP/1 requests. We might want to change that
in a subsequent commit. I left a TODO for that. For now there's no
change in behavior except that the context now cancels as it was
supposed to.

As a bonus that fell out for free, a Handler can now use CloseNotifier
and Hijack together in the same request now.

Fixes #15224 (make http1 Server always in a Read, like http2)
Fixes #15927 (cancel context when underlying connection closes)
Updates #9763 (CloseNotifier + Hijack)

Change-Id: I972cf6ecbab7f1230efe8cc971e89f8e6e56196b
Reviewed-on: https://go-review.googlesource.com/31173
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

@golang golang locked and limited conversation to collaborators Oct 15, 2017

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