Uh oh!
There was an error while loading. Please reload this page.
Stop BackgroundProcessor's thread on drop - #1007
Conversation
The specific error from the ChannelManager persister is not asserted for in test_persist_error. Rather, any error will do. Update the test to use BackgroundProcessor::stop and assert for the expected value.
Without stopping the thread when BackgroundProcessor is dropped, it will run free. In the context of language bindings, it is difficult to know how long references held by the thread should live. Implement Drop to stop the thread just as is done when explicitly calling stop().
Codecov Report
@@ Coverage Diff @@## main #1007 +/- ##
==========================================
- Coverage 90.76% 90.76% -0.01%
==========================================
Files 60 60 Lines 30909 30930 +21 ==========================================
+ Hits 28056 28075 +19 - Misses 2853 2855 +2
Continue to review full report at Codecov.
|
| /// 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>>>, |
There was a problem hiding this comment.
Maybe we should make this non-pub now? Users can always use stop or stop_and_join_thread.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Wait, how does that differ from stop?
There was a problem hiding this comment.
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.
c0e9673 to
038efadCompareUh oh!
There was an error while loading. Please reload this page.
valentinewallace
left a comment
There was a problem hiding this comment.
Just one last question, otherwise SGTM!
| fn join_thread(&mut self) -> Result<(), std::io::Error> { | ||
| match self.thread_handle.take() { | ||
| Some(handle) => handle.join().unwrap(), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Added docs about panicking to join and stop.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
It does feel a bit strange that we have a function that returns a
Result, but instead of ever returning anErrit 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'sjoin()is to return an error if the thread panics. Might it make more sense for ourjoinmethod 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.
The previous commit wraps the background thread's JoinHandle in an Option. Providing a dedicated method to join hides this implementation detail from users.
e8fbcf4 to
e260cfcCompare
Without stopping the thread when
BackgroundProcessoris dropped, it will run free. In the context of language bindings, it is difficult to know how long references held by the thread should live. ImplementDropto stop the thread just as is done when explicitly callingstop().Fixes#981.