serve: don't drop the server start error on the way out#889
Open
c-tonneslan wants to merge 1 commit into
Open
Conversation
The run loop selects between two channels: lch (the server's exit error from s.Start) and done (signal or close). When s.Start fails early (e.g. bind: permission denied on :22 under an unprivileged user, see charmbracelet#645), the goroutine writes to lch and then doneOnce() closes done. Both are then ready at the same time, and Go picks one at random. About half the time the select takes done first, falls through to the shutdown path, and the bind error never gets surfaced to the user, who just sees soft "start" successfully and then no listener on the configured port. When the done case fires with a nil sig (i.e. done was closed by doneOnce, not by a real signal), drain lch and return whatever error the server reported. Real signals (SIGINT/TERM with a non-nil sig) still take the graceful path. Closes charmbracelet#645 Signed-off-by: Charlie Tonneslan <cst0520@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #645.
The run loop selects between `lch` (the server's exit error from `s.Start`) and `done` (signal or close). When `s.Start` fails early (e.g. `bind: permission denied` on `:22` as an unprivileged user, which is the repro in #645), the goroutine writes to `lch` and then `doneOnce()` closes `done`. Both channels are then ready at the same time, and the select picks one at random — about half the time it takes `done` first, falls through to the shutdown path, and the bind error never makes it back to the caller. The user sees soft "start" successfully, but no listener on the configured port and nothing in the logs.
When the `done` case fires with a nil `sig` (which means `done` was closed by `doneOnce`, not raised by a real signal), drain `lch` and return whatever error the server reported. Real signal-driven shutdowns (non-nil `sig`) still take the graceful path.
@aymanbagabas — you pointed at this spot in the issue thread, hopefully this matches what you had in mind.