fix: Improve coder server shutdown procedure
#3246
Conversation
This commit improves the `coder server` shutdown procedure so that all triggers for shutdown do so in a graceful way without skipping any steps. We also improve cancellation and shutdown of services by ensuring resources are cleaned up at the end. Notable changes: - We wrap `cmd.Context()` to allow us to control cancellation better - We attempt graceful shutdown of the http server (`server.Shutdown`) because it's less abrupt (compared to `shutdownConns`) - All exit paths share the same shutdown procedure (except for early exit) - `provisionerd`s are now shutdown concurrently instead of one at a time, the also now get a new context for shutdown because `cmd.Context()` may be cancelled - Resources created by `newProvisionerDaemon` are cleaned up - Lifecycle `Executor` exits its goroutine on context cancellation Fixes #3245
| // Clean up idle connections at the end, e.g. | ||
| // embedded-postgres can leave an idle connection | ||
| // which is caught by goleaks. | ||
| defer http.DefaultClient.CloseIdleConnections() |
| devTunnelErrChan = make(<-chan error, 1) | ||
| ctxTunnel, closeTunnel = context.WithCancel(ctx) | ||
| devTunnel *devtunnel.Tunnel | ||
| devTunnelErr <-chan error |
There was a problem hiding this comment.
Why are we moving to an un-buffered channel here?
There was a problem hiding this comment.
This is just so that the channel is accessible outside the if, the actual channel is returned by devtunnel.New().
I thought it was weird that we're allocating a channel here since the nil channel blocks forever which is ideal, and informs the reader of our intent.
Otherwise it's the callers responsibility.
| // Trigger context cancellation for any remaining services. | ||
| cancel() | ||
|
|
||
| return exitErr |
There was a problem hiding this comment.
We'll always be returning an error now. There's really no way to cleanly shut down the server (e.g. click shut down in WebUI), so this makes sense. And interrupt for instance usually returns non-zero exit code.
The only downside is that we will print context cancelled and see --help at the end. I kind of want us to have a cobra post-exec for server that does os.Exit(n) is the context is cancelled. Thoughts?
This commit improves the
coder servershutdown procedure so that alltriggers for shutdown do so in a graceful way without skipping any
steps.
We also improve cancellation and shutdown of services by ensuring
resources are cleaned up at the end.
Notable changes:
cmd.Context()to allow us to control cancellation betterserver.Shutdown)because it's less abrupt (compared to
shutdownConns)exit)
provisionerds are now shutdown concurrently instead of one at atime, the also now get a new context for shutdown because
cmd.Context()may be cancellednewProvisionerDaemonare cleaned upExecutorexits its goroutine on context cancellationFixes #3245
Combined with hashicorp/hc-install#68, this PR
allows
goleakstest to pass forcliTestServertests.