event loop: tolerate unexpected exceptions in post() callbacks#260
event loop: tolerate unexpected exceptions in post() callbacks#260hulxv wants to merge 1 commit intobitcoin-core:masterfrom
post() callbacks#260Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste |
src/mp/proxy.cpp
Outdated
| int post_fd{m_post_fd}; | ||
| KJ_DEFER({ | ||
| Lock lock(m_mutex); | ||
| m_cv.wait(lock.m_lock, [this]() MP_REQUIRES(m_mutex) { return m_num_clients == 0; }); |
There was a problem hiding this comment.
In commit "fix: EventLoop::post() deadlocks when posted function throws" (c6ad605)
It seems like waiting for clients to be released here might just replace one deadlock with another deadlock in the general case m_post_fn throws and there is not just a single client.
I tend to think if m_post_fn is going to throws, reasonable things to do might be:
- Abort so this is detected as a bug
- Or keep executing the event loop, but log an error
- Or let the exception immediately propagate to the caller like happens currently.
Trying to propagate the exception, but waiting for resources to be freed and possibly hanging seems like it adds more complexity without providing a reliable benefit
There was a problem hiding this comment.
oh ok, I didn't notice that. thanks for catching it.
I tend to think if m_post_fn is going to throws, reasonable things to do might be:
* Abort so this is detected as a bug * Or keep executing the event loop, but log an error * Or let the exception immediately propagate to the caller like happens currently.
I think the best and simplest solution here is keeping the executing of the event loop and just log an error. what do you think?
There was a problem hiding this comment.
re: #260 (comment)
I think the best and simplest solution here is keeping the executing of the event loop and just log an error. what do you think?
Yes I think logging the exception here would be a simple improvement that should help debugging. Would recommend kj::runCatchingExceptions for that and kj::str() to turn the exception into a string.
Using catch (...) { eptr = std::current_exception(); } to catch the exception in the event loop thread and std::rethrow_exception(eptr); to rethrow it EventLoop::post() could be a different improvement that would help debugging, but I don't know if it would add too much complexity.
Keeping the event loop running after the exception is thrown should also be an improvement.
hulxv
force-pushed
the
fix/deadlock-on-exception
branch
from
c6ad605 to
5cf3278
Compare
There was a problem hiding this comment.
Thanks for the logging improvement!
Forgot when I wrote this that this change isn't only a logging improvement, but it also changes execution because EventLoop::post will now return on failure instead of hanging, and other I/O events will now continue to be processed. Would be good to mention these things in the pr/commit description and maybe change title to something like "event loop: tolerate unexpected exceptions in post() callbacks"
hulxv
changed the title
EventLoop::post() deadlocks when posted function throwspost() callbacks
hulxv
force-pushed
the
fix/deadlock-on-exception
branch
from
5cf3278 to
7b26641
Compare
|
is there anything else that should be added? |
3 participants
If a function posted via
EventLoop::post()threw an exception, the event loop would exit without resettingm_post_fnor notifying the condition variable, permanently deadlocking the calling thread inpost().This change catches the exception instead, logs it, and keeps the event loop running so the caller is unblocked and other I/O events continue to be processed.
Fix #259