cmd: ensure http servers shut down gracefully by mccutchen · Pull Request #262 · buzzfeed/sso
Problem
Both sso-proxy and sso-auth abort all in-flight requests on shutdown. Instead, they should stop accepting new requests and allow in-flight requests to complete before exiting.
Addressing this will make deploys of the sso services less disruptive to end users.
Solution
Because graceful shutdown is just tricky enough to get right, here we add a new internal httpserver package that exposes a single function, Run(), which will run an http server and arrange for graceful shutdown when receiving SIGTERM or SIGINT. A shutdown timeout must also be provided, after which the server will abort any outstanding requests.
Notes
Reproducing
The easiest way I know to reproduce the behavior uses httpbin via the quickstart:
- In the quickstart dir,
docker-compose up -dto bring up the demo services - In a browser, visit http://httpbin.sso.localtest.me/delay/9 to start a request that will take 9 seconds to complete
- Before those 9 seconds elapse, stop the sso-proxy container (note the
-t 30to tell docker to wait up to 30 seconds before forcibly killing it):
docker stop -t 30 $(docker ps | grep /bin/sso-proxy | awk '{print $1}')
On latest master, you should immediately get an HTTP 502 error.
With these changes, you should instead get a successful response after the 9 seconds have elapsed, and then sso-proxy will be shut down.
Unit tests
I'm not entirely sure how to test this, given that
- it relies on the process receiving either
SIGTERMorSIGINT, and - the simple
httpserver.Run()interface manages the whole lifecycle of the server, leaving no straightforward way to control the listener or figure out what port it is listening on (assuming we'd want it to choose a random available port)
If anybody has any good ideas for tackling (1) I'm all ears; the issues in (2) are under our control, so we could conceivably make the interface more flexible and use a trick like this to choose a random port in advance (though this would be inherently race-y).
I'll also leave a few notes inline below.