Preinitialize workCancel function in case StopAndCancel called before start
#557
Conversation
This file contains 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
|
@brandur oh, probably deserves a changelog entry too |
ccoVeille
reviewed
Aug 23, 2024
| @@ -491,6 +491,7 @@ func NewClient[TTx any](driver riverdriver.Driver[TTx], config *Config) (*Client | |||
| uniqueInserter: baseservice.Init(archetype, &dbunique.UniqueInserter{ | |||
| AdvisoryLockPrefix: config.AdvisoryLockPrefix, | |||
| }), | |||
| workCancel: func(cause error) {}, // replaced on start, but here in case StopAndCancel is called before start up | |||
There was a problem hiding this comment.
for such usecase I would define a defautWorkCanceler := func(error) {}
Then use it here, it helps to document separately, and using
Suggested change
| workCancel: func(cause error) {}, // replaced on start, but here in case StopAndCancel is called before start up | |
| workCancel: defautWorkCanceler // replaced on start |
There was a problem hiding this comment.
Hm, not sure I agree on that one :) You're never going to reuse this function, and I think adding a named variable for it introduces a layer of indirection that in the end just serves to make it less clear what the code's doing.
…fore start This one's related to #549. Although the most proximate problem in that repro code is that there was no error check when calling `Start`, it did reveal a legitimate problem in that the River client will panic in case `StopAndCancel` is called before `Start` because `workCancel` was never set. Here, initialize `workCancel` in the client's constructor. During normal operation this will be overwritten almost immediately on `Start` as the client starts up, but in case `Start` was never called or didn't run successfully, it provides a function for `StopAndCancel` to call so that it doesn't panic. Fixes #549.
brandur
force-pushed
the
brandur-preinit-work-cancel
branch
from
4886b73
to
35623ec
Compare
Added. |
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 one's related to #549. Although the most proximate problem in that
repro code is that there was no error check when calling
Start, it didreveal a legitimate problem in that the River client will panic in case
StopAndCancelis called beforeStartbecauseworkCancelwas neverset.
Here, initialize
workCancelin the client's constructor. During normaloperation this will be overwritten almost immediately on
Startas theclient starts up, but in case
Startwas never called or didn't runsuccessfully, it provides a function for
StopAndCancelto call so thatit doesn't panic.
Fixes #549.