Fix deadlock between ReschedulableTask::destroy() and concurrent task - #24
Conversation
| m_state->async_task->destroy(); | ||
| m_state->pending_invocations = -1; | ||
| std::shared_ptr<AsyncTask> async_task = m_state->async_task; | ||
| m_state->pending_invocations = -1; |
There was a problem hiding this comment.
I haven't tried to break it, but I feel like assigning this before calling async_task->destroy() could result in the task re-incrementing the counter to 0 when trying to run, which would probably break the contract and lead to some form of a corrupted state. This would be a very rare case (probably the same category of case that would previously have deadlocked the code?). The important part of this method is that we need to have a strong guarantee that the function that was passed into the ReschedulableTask can never be invoked again after destroy() returns.
There was a problem hiding this comment.
Hmm I think you're right. If schedule_now/schedule_after/cancel are called concurrently with destroy this might be an issue. Let me think about the right way to fix this...
There was a problem hiding this comment.
I built a small deterministic reproducer and I could see pending_invocations rise -1 -> 0 -> 1 but I could never trigger the function running after destroy. It seems to be safe by an ordering accident of the strand + future. I think the right fix for this is to set and check a destroyed bool instead of the -1 value. What do you think?
There was a problem hiding this comment.
Seems reasonable, recycling the pending invocation count was a bit of a hack, this is more explicit.
destroy() held InternalState::mutex while blocking on AsyncTask::destroy(), which waits for the asio strand to become free. The strand running the callback starts by acquiring the same mutex. So if destroy() coincides with a callback, it becomes deadlocked. This might happen if a reconnection or confirm timeout task fires at the same time that something calls stop(). destroy() now releases the mutex before calling AsyncTask::destroy(), keeping a local reference to the AsyncTask so a concurrent destroy() call can't have it torn down from under the first caller. AsyncTask::destroy() is idempotent, so both callers redoing it is OK. They converge on waiting for the same current_invocation afterward, matching what holding the mutex throughout used to guarantee. We also set a destroyed bool on the task which takes the place of setting and checking that pending_invocations is -1. This avoids races where pending_invocations is incremented which ruin the indicator that the task is destroyed. This change adds a test case to a new suite reschedulable_task.cpp which thrashes - it keeps the task firing from one thread and repeatedly destroys and recreates it from another. This reliably reproduces the deadlock before the fix after a few iterations.
b2ba5ed to
38dcab8
Compare
2 participants
destroy() held InternalState::mutex while blocking on AsyncTask::destroy(), which waits for the asio strand to become free. The strand running the callback starts by acquiring the same mutex. So if destroy() coincides with a callback, it becomes deadlocked. This might happen if a reconnection or confirm timeout task fires at the same time that something calls stop().
destroy() now releases the mutex before calling AsyncTask::destroy(), keeping a local reference to the AsyncTask so a concurrent destroy() call can't have it torn down from under the first caller. AsyncTask::destroy() is idempotent, so both callers redoing it is OK. They converge on waiting for the same current_invocation afterward, matching what holding the mutex throughout used to guarantee.
This change adds a test case to a new suite reschedulable_task.cpp which thrashes - it keeps the task firing from one thread and repeatedly destroys and recreates it from another. This reliably reproduces the deadlock before the fix after a few iterations.