Cancel the context of worker function when miner is stopped #3180
Merged
lukasz-zimnoch merged 5 commits intomainfrom Aug 18, 2022
Merged
Cancel the context of worker function when miner is stopped #3180lukasz-zimnoch merged 5 commits intomainfrom
lukasz-zimnoch merged 5 commits intomainfrom
Conversation
This change allows stopping the worker function immediately when the miner receives the stop signal. This is important for long-running worker functions when a single iteration can take a couple of minutes. Thanks to the change from tss-lib (bnb-chain/tss-lib#191) we can now stop the pre-param generation immediately with this mechanism. I have also added a mutex to `mockPersistence` in `pool_test.go`. There is a single goroutine writing to the map, but it's a separate goroutine from the test one reading from a map so having a mutex feels safer, especially since there might be some changes in miner or some other tests can be added to `pool_test.go`.
Improved the handling of errors to not log on the ERROR level in case the parent context was cancelled by the miner. tss-lib returns generic errors saying "timeout or error while ...". There are three possibilities: 1. Pool canceled the parent `ctx`. This is normal and we should not log anything in this case. 2. `timingOutCtx` timed out. It means the machine is not fast enough or that it was just unlucky. We should log a warning. 3. There is some error from tss-lib generator. We log it as a warning because we'll re-attempt to generate parameters again.
This way is much cleaner - the miner is creating and maintaining this array and we can use a concrete type there.
The select is much more elegant and ensures the goroutine does not leak after the context is done.
lukasz-zimnoch
previously approved these changes
Aug 18, 2022
The test failed on CI but keeps passing locally. There was a potential problem when the pool generated enough parameters to reach the capacity and although all of them were persisted, the last one was not written to the buffered channel given it's blocking operation. In this case pool.CurrentSize() is off-by-one to persistence.parameterCount().
lukasz-zimnoch
approved these changes
Aug 18, 2022
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
2 participants
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.
Refs #3161
Depends on #3177
This change allows stopping the worker function immediately when the miner receives the stop signal. This is important for long-running worker functions when a single iteration can take a couple of minutes. Thanks to the change from tss-lib (bnb-chain/tss-lib#191) we can now stop the pre-param generation immediately with this mechanism.
I have also added a mutex to
mockPersistenceinpool_test.go. There is a single goroutine writing to the map, but it's a separate goroutine from the test one reading from a map so having a mutex feels safer, especially since there might be some changes in miner or some other tests can be added topool_test.go.