fix: EventLoop::post() deadlocks when posted function throws#260
fix: EventLoop::post() deadlocks when posted function throws#260hulxv wants to merge 1 commit intobitcoin-core:masterfrom
EventLoop::post() deadlocks when posted function throws#260Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process. |
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
3 participants
Fix #259