Skip to content

Make shutdown procedure more robust #592

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 9, 2025

Conversation

tnull
Copy link
Collaborator

@tnull tnull commented Jul 7, 2025

These are a few improvements to our shutdown logic. Most notably, we now switch our 'shutdown' signal for the background event processor from using channels to just keeping the JoinHandle, which also gets rid of a potential race condition. Moreover, we attempt to clear the broadcast queue one last time before we shut down.

Previously, we had to configure enormous syncing timeouts as the BDK
wallet syncing would hold a central mutex that could lead to large parts
of event handling and syncing locking up. Here, we drop the configured
timeouts considerably across the board, since such huge values are
hopefully not required anymore.
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jul 7, 2025

I've assigned @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

src/lib.rs Outdated
@@ -670,6 +667,17 @@ impl Node {
// Disconnect all peers.
self.peer_manager.disconnect_all_peers();

// Ensure we process the broadcast queue one last time, as we may have generated
// force-closures during peer disconnection.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're force closing due to a disconnection, then we never broadcast the funding transaction to begin with, so we shouldn't need to broadcast a commitment transaction. I assume the behavior you were seeing will end up getting fixed by lightningdevkit/rust-lightning#3881.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay. I expected this but wasn't sure if we could lean on it. Will drop the commit then.

tnull added 2 commits July 9, 2025 12:38
Previously, we used to a channel to indicate that the background
processor task has been stopped. Here, we rather just await the task's
`JoinHandle` which is more robust in that it avoids a race condition.
.. we provide finer-grained logging after each step of `stop`.
@tnull tnull force-pushed the 2025-07-make-shutdown-more-robust branch from 7c773a1 to c3d6161 Compare July 9, 2025 10:39
@tnull tnull merged commit bc6137a into lightningdevkit:main Jul 9, 2025
12 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants