This repository has been archived by the owner on Aug 31, 2023. It is now read-only.
feat: Restart failed background processor #572
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
Annotating the function with `#[tokio::main]` killed the runtime after the function completes. Thus we had to block the run function indefinitely for the long living threads to keep going. This change introduces a runtime managed outside of the function scope and thus allows the `run` function to return bringing the following advantages. - We don't block a whole frb worker thread just to run the lightning node, sync tasks, background processor, etc. - We are using a multi threaded runtime instead of the current thread - allowing to actually join the background processor without blocking all other tasks. - making better use of multiple cpu cores. - We are not creating a new runtime on every async bridge call.
Adds and handling in case the background processor stops, by simply trying to start the background processor again. Some more refactoring could be probably done there, but I tried to keep the changes to a minimum.
holzeis
force-pushed
the
feat/restart-failed-background-processor
branch
from
5285be0 to
0d50521
Compare
holzeis
force-pushed
the
chore/use-dedicated-runtime
branch
from
d895a5b to
4e54c8d
Compare
luckysori
approved these changes
Dec 15, 2022
| let mut background_processor = background_processor; | ||
| loop { | ||
| // background processor joins on a sync thread, meaning that join here will block a | ||
| // full thread, which is dis-encouraged to do in async code. |
There was a problem hiding this comment.
🔧 It should be "discouraged"!
| // background processor joins on a sync thread, meaning that join here will block a | ||
| // full thread, which is dis-encouraged to do in async code. | ||
| if let Err(err) = background_processor.join() { | ||
| tracing::warn!(?err, "Background processor stopped unexpected"); |
There was a problem hiding this comment.
🔧 It should be "unexpectedly"!
Comment on lines
+530
to
+536
| // Step 17: Initialize routing ProbabilisticScorer | ||
| let scorer_path = format!("{ldk_data_dir}/scorer"); | ||
| let scorer = Arc::new(Mutex::new(disk::read_scorer( | ||
| Path::new(&scorer_path), | ||
| Arc::clone(&network_graph), | ||
| Arc::clone(&logger), | ||
| ))); |
There was a problem hiding this comment.
❓ Why did we have to move this?
There was a problem hiding this comment.
to be able to start the background processor from the properties of the lightning system
Comment on lines
-596
to
+602
| ); | ||
| let background_processor = system.start_background_processor(); |
There was a problem hiding this comment.
🔧 I'm not a huge fan of calling this here (inside of run_ldk) and also on the outside when restarting the background processor.
Actually, this function appears to be doing only 2 things:
- Setting the
INVOICE_PAYERstatic variable, something that should only happen once during the lifetime of the app AFAIK. - Starting the background processor via the
LightningSystem.
As such, I think we should get rid of the run_ldk function and just do those two things separately when necessary.
There was a problem hiding this comment.
agreed, but
Some more refactoring could be probably done there, but I tried to keep the changes to a minimum.
🙂
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Adds and handling in case the background processor stops, by simply trying to start the background processor again.
Some more refactoring could be probably done there, but I tried to keep the changes to a minimum.