Skip to content

Properly shutdown the server#244

Merged
byo merged 7 commits into
mainfrom
proper-server-shutdown
May 20, 2026
Merged

Properly shutdown the server#244
byo merged 7 commits into
mainfrom
proper-server-shutdown

Conversation

@byo
Copy link
Copy Markdown
Contributor

@byo byo commented May 8, 2026

When starting the shutdown connection use a new
context with specific timeout since the one used
in the server has already been canceled.

When starting the shutdown connection use a new
context with specific timeout since the one used
in the server has already been canceled.
@byo byo requested review from gammazero, nymd and willscott May 8, 2026 16:14
Comment thread server.go
Comment on lines 335 to 350
<-s.Context.Done()
err := serv.Shutdown(s.Context)

shutdownCtx, cancel := context.WithTimeout(context.Background(), config.Server.ShutdownTimeout)
defer cancel()

err := serv.Shutdown(shutdownCtx)
if err != nil {
log.Warnw("failed shutdown", "err", err)
log.Warnw("failed to shutdown", "err", err)
ec <- err
}

err = metricsServ.Shutdown(shutdownCtx)
if err != nil {
log.Warnw("failed to shutdown metrics server", "err", err)
ec <- err
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If both shutdowns fail the second send blocks forever and defer close(ec) never runs.

Cleanest fix is collect both errors and send once, which keeps the deal with the consumer as "at most one value, then close":

Suggested change
<-s.Context.Done()
err := serv.Shutdown(s.Context)
shutdownCtx, cancel := context.WithTimeout(context.Background(), config.Server.ShutdownTimeout)
defer cancel()
err := serv.Shutdown(shutdownCtx)
if err != nil {
log.Warnw("failed shutdown", "err", err)
log.Warnw("failed to shutdown", "err", err)
ec <- err
}
err = metricsServ.Shutdown(shutdownCtx)
if err != nil {
log.Warnw("failed to shutdown metrics server", "err", err)
ec <- err
}
<-s.Context.Done()
shutdownCtx, cancel := context.WithTimeout(context.Background(), config.Server.ShutdownTimeout)
defer cancel()
var errs []error
if err := serv.Shutdown(shutdownCtx); err != nil {
log.Warnw("failed to shutdown finder server", "err", err)
errs = append(errs, fmt.Errorf("finder server: %w", err))
}
if err := metricsServ.Shutdown(shutdownCtx); err != nil {
log.Warnw("failed to shutdown metrics server", "err", err)
errs = append(errs, fmt.Errorf("metrics server: %w", err))
}
if err := errors.Join(errs...); err != nil {
ec <- err
}

Needs errors added to the import block.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I analyzed the code further and found out that there could be more issues with the approach.

Updated the code to properly handle a case where there are multiple errors sent through the channel with extra unit tests to ensure out application / signal interface layer handles such case properly.

@nymd nymd assigned byo May 8, 2026
byo added 6 commits May 18, 2026 12:48
* Use non-blocking send when config time change is detected
  avoiding rare deadlock
* Use same channel for signal detection for config file time
  change detection
This function is responsible for integration
between the running server and external signals
that could affect the server: termination and
config reload.
@byo byo force-pushed the proper-server-shutdown branch from 5479660 to e2951ba Compare May 19, 2026 10:12
@byo byo requested a review from nymd May 19, 2026 10:12
Copy link
Copy Markdown

@nymd nymd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elegant fix to do it consumer-side. Thanks.

@byo byo merged commit 1ee7f08 into main May 20, 2026
6 checks passed
@byo byo deleted the proper-server-shutdown branch May 20, 2026 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants