Don't error on cancelled startup to make it not necessary to export ErrShutdown
#841
Conversation
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
325af9e to
f6470c0
Compare
…ErrShutdown` This change's driven by the proposal in #830. When client start up is cancelled by a stop, we'd return `ErrShutdown` as a special signal. The problem is that `ErrShutdown` was never exported, so this wasn't really usable by outside callers. In #830 it was proposed to export `ErrShutdown`. However, after some discussion, we suspect that it might be better to leave it unexported, and then have `Start` return no error on cancelled startup. This minimizes the number of startup conditions callers have to handle. Because `ErrShutdown` wasn't previously exported, we should be able to make this change safely without breaking anyone. @mastercactapus did bring up a fair point that the alternative of exporting `ErrShutdown` would allow a caller to definitively tell the difference between the client successfully starting and backgrounding itself versus one whose start was cancelled. I think we're okay not to require that differentiation though because if `ErrShutdown` would've be returned, then the caller would have called `Stop`, so presumably they'd know to expect an successful start. I figure that in case a strong case for wanting to differentiate the cases comes through, we could always resurrect the error. Also, rename the internal `ErrShutdown` to `ErrStop` to be more consistent with the naming of `Client.Stop` (which had previously been called `Shutdown`). This won't make any difference, but will be a little better in case we ever do export it and then forget to rename it at that time.
f6470c0 to
38c731e
Compare
bgentry
approved these changes
Apr 9, 2025
|
Thx. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This change's driven by the proposal in #830. When client start up is
cancelled by a stop, we'd return
ErrShutdownas a special signal. Theproblem is that
ErrShutdownwas never exported, so this wasn't reallyusable by outside callers.
In #830 it was proposed to export
ErrShutdown. However, after somediscussion, we suspect that it might be better to leave it unexported,
and then have
Startreturn no error on cancelled startup. Thisminimizes the number of startup conditions callers have to handle.
Because
ErrShutdownwasn't previously exported, we should be able tomake this change safely without breaking anyone.
@mastercactapus did bring up a fair point that the alternative of
exporting
ErrShutdownwould allow a caller to definitively tell thedifference between the client successfully starting and backgrounding
itself versus one whose start was cancelled. I think we're okay not to
require that differentiation though because if
ErrShutdownwould've bereturned, then the caller would have called
Stop, so presumably they'dknow to expect an successful start.
I figure that in case a strong case for wanting to differentiate the
cases comes through, we could always resurrect the error.
Also, rename the internal
ErrShutdowntoErrStopto be moreconsistent with the naming of
Client.Stop(which had previously beencalled
Shutdown). This won't make any difference, but will be a littlebetter in case we ever do export it and then forget to rename it at that
time.