net/http: document Server.Shutdown ignores hijacked conns

[bradfitz]

Document that Server.Shutdown ignores hijacked conns (such as WebSockets).

[deankarn]

no problem @bradfitz I'll comment from the open issue, my apologies

The problem that I see is that when the http.Server is finally shutdown with the current implementation hijacked connections will be terminated when the application process terminates instead of being/able to gracefully shutdown. Just using WebSockets as en example, which are pretty common, would be terminated, possibly while still sending or receiving data.

I know that there is no way to tell how the Hijacked connection is/will be used, but the desired behaviour would be that there be some way for the Hijacked connection code to be notified that the server is being shutdown and for the http.Server's Shutdown() function to somehow wait for the hijacked connections to be closed or eventually Timeout.

my idea was a channel of some sort that the hijacked connections could optionally listen for close on.

[bradfitz]

The semantics of Hijack have always been that it's then disconnected from the net/http package. It's a little weird for us to keep tracking them, or even to assume they're WebSockets at all.

And Shutdown isn't supposed to forcefully break anything. That includes both TCP connections, and contexts. You could imagine Shutdown closing the base Server context.Context channel, which means all subcontexts would then be finished. But that will likely break people in the middle of real work.

So, yes, maybe there needs to be an opt-in way for long-lived requests (be they WebSockets or bidi communication over http2, etc) to listen for shutdown and fail gracefully.

Internally inside the http.Server we already have such a channel, and go out of our way to snyly get to it from http2 to use it there.

Maybe we should just export an accessor to get it.

/cc @tombergan

[deankarn]

@bradfitz exposing a channel would definitely help.

the only thing is if the connections aren't tracked somehow then it would be a race between graceful shutdown and the timeout.

eg.

conn := // hijacked connection

FOR:
for {
    select {
        case <-shutdown:
            conn.Close()
            break FOR
        case msg := <- messageToWrite:
            // if this write takes a while then ShutDown() Timeout could fire
            // before it even get's a chance to check if it should shutdown.
            conn.Write(msg)
    }
}

[bradfitz]

We could do a whole registration thing, letting websocket packages tell the http.Server about their conns coming and going (or not even their Conns, just that they're doing work), but I'm just not sure how much that matters. People could do that on their own, too. Yes, I agree we could put it in a common place for convenience, but maybe not for this release.

Worst case: if the Go process dies because we thought things were gracefully shut down but we end up killing some unprotected websocket connections, that's not the end of the world... most websocket connections are just left open to notify users of new stuff in real time. That is, they're semantically idempotent. The client JS will probably just reconnect and get a new connection.

Otherwise people who care can first call Shutdown, then notify their websocket clients (e.g. close some channel), and then wait for all their websockets to shut down, and then exit the process.

That is, documentation only is probably sufficient here for now.

[deankarn]

Thanks @bradfitz that is definitely a viable option :)

Last thing I'll say about this is that in the future perhaps keeping track of all connections, maybe via a WaitGroup, from within a/the net.Listener when they are accepted and closed could be impemented that way graceful shutdown could be implemented not only in net/http by anything that uses a net.Listener such as RPC etc. and the net/http graceful shutdown would just be a superset of that functionality.

In this way any connection, hijacked or not, can be waited for and if a channel is available indicating shutdown has been initiated then anything being done with the connection could be optionally programmed to close() gracefully.

Just food for thought :) perhaps I should make a more formal proposal about it?

Thanks again

[quentinmit]

My opinion is that the request context is the right place to propagate cancellation, but that shouldn't necessarily happen immediately when server.Shutdown is called. If we could pass in a context to Server, that would allow the user to control when it is cancelled...

[bradfitz]

We decided against letting people set a Server base context in #16220 because there are existing ways to do it already.

But I agree that the way to shut down unbounded handlers (like Websockets) is to have some "please stop soon" context (of some sort) that the caller of Shutdown cancels, before or after calling Shutdown.

In summary: this bug remains a documentation issue.

[gopherbot]

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