Skip to content

Stop BackgroundProcessor's thread on drop #1007

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.

Sign up for GitHub

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 29, 2021
Merged

Stop BackgroundProcessor's thread on drop #1007

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9fa8f1
Correctly assert BackgroundProcessor error
jkczyz Jul 18, 2021
4f05db6
Stop BackgroundProcessor's thread on drop
jkczyz Jul 18, 2021
e260cfc
Add join method to BackgroundProcessor
jkczyz Jul 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 25 additions & 5 deletions lightning-background-processor/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ pub struct BackgroundProcessor {
stop_thread: Arc<AtomicBool>,
/// May be used to retrieve and handle the error if `BackgroundProcessor`'s thread
/// exits due to an error while persisting.
pub thread_handle: JoinHandle<Result<(), std::io::Error>>,
pub thread_handle: Option<JoinHandle<Result<(), std::io::Error>>>,
Copy link
Collaborator

@TheBlueMatt TheBlueMatt Jul 19, 2021

Choose a reason for hiding this comment

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

Maybe we should make this non-pub now? Users can always use stop or stop_and_join_thread.

Copy link
Contributor Author

@jkczyz jkczyz Jul 19, 2021

Choose a reason for hiding this comment

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

Yeah, that would be a good idea. This was pub to allow waiting on any errors (e.g. from persisting). We'd have to expose a join method to allow for that behavior still, which also hides the Option implementation detail.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Jul 19, 2021

Choose a reason for hiding this comment

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

Wait, how does that differ from stop?

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Jul 19, 2021

Choose a reason for hiding this comment

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

Oh, duh.

Copy link
Contributor Author

@jkczyz jkczyz Jul 19, 2021

Choose a reason for hiding this comment

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

stop will stop the thread (if it hasn't already because of an error) and allows you to examine any errors that may have occurred. join will wait for any errors to occur. The latter is only useful if your persisters can give an error. I updated the documentation to spell out these cases in terms of join and stop.

}

#[cfg(not(test))]
Expand Down Expand Up @@ -158,13 +158,27 @@ impl BackgroundProcessor {
}
}
});
Self { stop_thread: stop_thread_clone, thread_handle: handle }
Self { stop_thread: stop_thread_clone, thread_handle: Some(handle) }
}

/// Stop `BackgroundProcessor`'s thread.
pub fn stop(self) -> Result<(), std::io::Error> {
pub fn stop(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
self.stop_and_join_thread()
}

fn stop_and_join_thread(&mut self) -> Result<(), std::io::Error> {
self.stop_thread.store(true, Ordering::Release);
self.thread_handle.join().unwrap()
match self.thread_handle.take() {
Some(handle) => handle.join().unwrap(),
Copy link
Contributor

@valentinewallace valentinewallace Jul 20, 2021

Choose a reason for hiding this comment

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

It seems that if the background thread panics, join will return an error that will cause the unwrap to panic on this line. Might it'd be preferable to either return the panic as an error or at least document that this is the expected behavior?

Copy link
Contributor Author

@jkczyz jkczyz Jul 28, 2021

Choose a reason for hiding this comment

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

Added docs about panicking to join and stop.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Jul 28, 2021

Choose a reason for hiding this comment

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

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

Copy link
Contributor

@valentinewallace valentinewallace Jul 28, 2021

Choose a reason for hiding this comment

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

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

Copy link
Contributor Author

@jkczyz jkczyz Jul 28, 2021

Choose a reason for hiding this comment

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

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

It will return an Err if the persister returns an Err. The unwrap is to remove the error handling JoinHandle provides for panics, not for the Result returned by the thread's closure.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

My intention was to mirror the same behavior as stop, which would already panic in this case. The reasoning for panicking was because the panic would likely have originated from user code. So users should either write code to not panic in the case of event handling (some discussion here) or to return an error in case of persisting. If the panic is coming from RL, my assumption was we'd want to propagate it.

I'm not necessarily opposed to making it return the wrapped result, but I think we'd want stop to have the same behavior. There may be a good argument based on how a user configures Rust to handle panics. I vaguely remember @TheBlueMatt discussing it with us but I'm a bit foggy on the details.

None => Ok(()),
}
}
}

impl Drop for BackgroundProcessor {
fn drop(&mut self) {
self.stop_and_join_thread().unwrap();
}
}

Expand Down Expand Up @@ -416,7 +430,13 @@ mod tests {
let persister = |_: &_| Err(std::io::Error::new(std::io::ErrorKind::Other, "test"));
let event_handler = |_| {};
let bg_processor = BackgroundProcessor::start(persister, event_handler, nodes[0].chain_monitor.clone(), nodes[0].node.clone(), nodes[0].peer_manager.clone(), nodes[0].logger.clone());
let _ = bg_processor.thread_handle.join().unwrap().expect_err("Errored persisting manager: test");
match bg_processor.stop() {
Ok(_) => panic!("Expected error persisting manager"),
Err(e) => {
assert_eq!(e.kind(), std::io::ErrorKind::Other);
assert_eq!(e.get_ref().unwrap().to_string(), "test");
},
}
}

#[test]
Expand Down