Skip to content
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

Detect if the background processor exits early and shutdown if so #109

Merged

Conversation

valentinewallace
Copy link
Contributor

A user ran into an issue where the BackgroundProcessor silently stopped, leading to ChannelManager persistence falling behind + channel force closures. Now we'll detect if the bg processor exits early and log the result.

Still need to do a bit more reading up on tokio docs to vet the patch but feedback welcome.

@valentinewallace valentinewallace marked this pull request as ready for review August 3, 2023 21:41
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated
// Exit if CLI polling exits or the background processor exits (which shouldn't happen unless we
// fail to write to the filesystem).
tokio::select! {
_ = cli::poll_for_user_input(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if it is strictly required, but you may want to spawn this instead.

https://docs.rs/tokio/latest/tokio/macro.select.html#runtime-characteristics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on those docs, I can't tell if with the way I had it written previously we would have gotten stuck on the CLI select branch. LMK if you know, for my understanding.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably not stuck given the background processor task was already spawned and would have been running that future already.

https://docs.rs/tokio/latest/tokio/task/fn.spawn.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I meant whether we would ever hit the other select branch to check for the bg processor exiting. Looks like it does work: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=0c2f28caa7c80e47d969465ba17e9986

@valentinewallace valentinewallace force-pushed the 2023-07-detect-bgproc-exit branch 2 times, most recently from 94dfd54 to dabe1c3 Compare August 3, 2023 23:43
src/main.rs Show resolved Hide resolved
src/main.rs Outdated
// Exit if CLI polling exits or the background processor exits (which shouldn't happen unless we
// fail to write to the filesystem).
tokio::select! {
_ = cli::poll_for_user_input(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably not stuck given the background processor task was already spawned and would have been running that future already.

https://docs.rs/tokio/latest/tokio/task/fn.spawn.html

@jkczyz jkczyz merged commit 34a748c into lightningdevkit:main Aug 7, 2023
4 checks passed
_ = cli_poll => {},
bg_res = &mut background_processor => {
stop_listen_connect.store(true, Ordering::Release);
peer_manager.disconnect_all_peers();
Copy link
Contributor

Choose a reason for hiding this comment

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

We should do a last-ditch ChannelManager persist attempt before panicking.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

4 participants