go: net/http: Shutdown must make all future Serves return errors

This is a fundamental mistake in the Shutdown API. The idea that you can Shutdown a Server and then once it’s done call Serve again is wrong. We should make it be that once you Shutdown a server, it’s shut down. Future calls to Serve simply return errShutdown.

If this is too invasive for now it would be fine to punt to Go 1.11 but there’s really no other way it can work. Any other way, you have this race between concurrent Shutdown and Serve behaving unpredictably.

It’s only a few lines of code to make shutdown sticky, and there’s a decent argument for making this possibly-breaking change now instead of letting Shutdown last another release in the racy form (it was introduced in Go 1.8), just to head off 6 more months of people potentially thinking they can Serve again after Shutdown.

/cc @tombergan @bradfitz to decide about Go 1.10 vs Go 1.11

I stumbled across this issue, so I took a few hours taking a look. I investigated the interaction between Serve() and Shutdown()/Close() instead of ListenAndServe() as the original report discusses, as that’s where the main logic is.

First, I did see a somewhat surprising behavior when I tried to run code similar to https://github.com/golang/go/issues/20239#issuecomment-299245151. You would expect the server to stop and Serve() to bail out upon calling Close(), but Serve() just blocks on l.Accept(), waiting for connections.

The reason for this is that while Close() kills off all net.Listeners that were registered before the call to Close() is made, it does not necessarily “stop” the Serve() call. The order of event that leads to Serve() to block is as follows:

1. srv.Close() gets called, kills all previous connections
2. srv.Serve(l) gets called
3. srv.trackListeners(l) gets called, registers the listener l
4. At the same time as (3), srv.trackListeners(l) sees that the number of registered listeners == 0, so clears srv.doneChan, and basically resets the state of the Server as running.
5. the server merrily goes onto waiting for incoming connections, and blocks on l.Accept(). It will even go into the server process loop again, because the srv.doneChan says that we’re running.

The really tricky part is that srv.trackListeners() actually resets srv.doneChan.

From the perspective of a user who is creating a simple Web server, you would expect Close() to set a flag in the Server to not allow it to process anymore requests, so the fact that Serve() could block should be understandably confusing.

However, this reset is there to allow re-using the Server even after Close() was called. In pseudocode, this is allowed in the current http.Server:

var srv http.Server
l1, err := newListener(...)
go srv.Serve(l1)
srv.Close()

// ... later ...
l2, err := newListener(...)
go srv.Serve(l2)

It’s fairly easy to make Serve() not block when Close() is called before it, but a simple fix would actually make the Server not work when Serve() is called again.

We have a situation where we are sharing http.Server (global state) between multiple goroutines, and their behavior is timing dependent as to when you call Close() and Serve().

Given this fact and that we have a backwards compatible guarantee, I think we should be looking at introducing a context.Context aware API. This would free the user from having to know the timing when the server is ready as the OP posted alltogether

I expect to have some way of knowing that the server is now up and running such that Shutdown() will stop it, short of sending it http requests.

l1, ... := newListener(addr1)
l2, ... := newListener(addr2)
ctx, cancel := context.WithCancel()
go srv.ServeCtx(ctx, l1)
go srv.ServeCtx(ctx, l2)

// ... later ...
cancel() // kills both previous
// if you need to reuse srv, you can, by using a new context
go srv.ServeCtx(ctx2, l3)

Maybe inclusion of such API is planned, I don’t know.

But meanwhile, without a new API, I don’t think this an easy to fix problem. So I’d like to suggest documenting that it is the users’ responsibility to synchronize calling Close()/Shutdown() and Serve() (or equivalent), or make sure to call Close()/Shutdown() repeatedly until the call to Serve() returns (this is not an elegant solution, but it should work 99% of the times).

---

If people are up for (1) documentation and (2) context.Context aware API, I’m willing to work on either.

One way to work around this for now is to use a wait group in reverse. I think you can still have a context switch between isUp.Done() and srv.Serve so this shouldn’t be 100% safe but better than sprinkled context switches. At least code from the right go routine was executed.

srv := &http.Server{}
var isUp sync.WaitGroup
isUp.Add(1)
go func() {
    isUp.Done()
    if err := srv.Serve(l); err != nil {...}
}()
isUp.Wait()
// safe to call srv.Shutdown() from here on