Consider shutdown signal during entire signer loop #936
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
roeierez
approved these changes
Apr 16, 2024
dangeross
approved these changes
Apr 16, 2024
|
As discussed, I separated the scheduler loop from the signer loop, such that calls to PR is ready for final review. |
ok300
added a commit
that referenced
this pull request
Apr 17, 2024
* Consider shutdown signal during entire signer loop * Cargo fmt * Add comments to run_forever_inner() * Separate scheduler and signer loop
roeierez
added a commit
that referenced
this pull request
May 7, 2024
* commit 'b08215dc4fa98cadb50616a3702bc8f83afaf636': Fix indentation on CHANGELOG.md Update sdk-flutter plugin changelogs Update version to 0.4.0 Change the redeem swap failure notification to prompt to open the app Update version to 0.4.0-rc5 Move notification source files and change podspecs/package Consider shutdown signal during entire signer loop (#936) Update version to 0.4.0-rc4 Merge Swift package sources into one BreezSDK target Update version to 0.4.0-rc3 upgrade version to 0.4.0-rc2 Add a service timeout to the android notification plugin Pre-check for a valid notification manager before starting the foreground service temporary skip c# tests Fix csharp test connect request fix test to use production environment update versions to 0.4.0
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.
Before this PR, the signer loop consisted of
signer.run_onceis not running, which may take 10 minutes to return. In other words, in the worst case the shutdown signal is handled (e.g. signer loop exits) 10 minutes after it's sent.This PR wraps both loops above in a single
run_forever_innerthread. In parallel to it, it listens to the shutdown signal and exits the thread as soon as the signal is received.Possibly overrides #934.
Note to reviewers / Open question
I'm not sure if the signer upgrade loop (
scheduler.maybe_upgrade) should be interrupted in the case of an upgrade. WIth this PR, it would be interrupted if we get a shutdown signal.On the other hand, as described in #934, if we disregard the shutdown signal while we're in the signer upgrade loop, we may end up in an endless loop in the case when the GL scheduler is not reachable.
What do you think? How to best handle the upgrade loop?