Fix race condition in cancellation setup/teardown#1609
Conversation
The code failed to handle three cases. 1. A cancellation is in progress when a cancellable awaitable begins. 2. A cancellation is in progress when a cancellable awaitable ends. 3. A cancellation is in progress when a cancel() request is made. The m_canceller member has one of these three values: * nullptr, meaning that there is nothing to cancel. It has this value when the coroutine is not awaiting, or if it is awaiting something that cannot be cancelled. * cancelling_ptr, meaning that another thread (not the coroutine thread) is in the middle of cancellation request. * function pointer, representing the function to call to cancel the await. In case 1, we should not overwrite the canceling_ptr with the function pointer, because only the code doing the cancel() can transition into/out of cancelling_ptr. In case 2, we intended to spin until the m_canceller is no longer cancelling_ptr, but we used m_canceller.exchange(nullptr) in a loop, which means that if m_canceller was cancelling_ptr, we overwrite it with nullptr. As a result, the "while" loop always exits after one iteration. We need to spin on the m_canceller without modifying it if it is cancelling_ptr. In case 3, cancel() function resets m_cancelling back to nullptr, even if the value was cancelling_ptr on entry, prematurely declaring that the existing cancel() has completed. If the original value was cancelling_ptr, we should leave it that way. There are still other cases not handled: * Coroutine already cancelled when a co_await starts. In this case, we never call the canceller, so the coroutine fails to propagate cancellation. This will require a broader fix, so I'm not going to fix it in this PR. This PR is primarily about fixing the crash caused by case 2. Cases 1 and 3 were fixed opportunistically.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Fixes race conditions in coroutine cancellation propagation by tightening the state machine around cancellable_promise::m_canceller so cancellation setup/teardown and overlapping Cancel() calls don’t corrupt the atomic state (addressing the crash described as “case 2” and opportunistically improving cases 1 and 3).
Changes:
set_cancellernow uses a CAS fromnullptrto avoid overwriting thecancelling_ptrsentinel during an in-flight cancellation.revoke_cancellernow waits forcancelling_ptrto clear without clobbering it, and then clears the canceller via CAS.cancelno longer resetsm_cancellerback tonullptrwhen it detects another cancellation is already in progress.
| auto existing = m_canceller.load(std::memory_order_relaxed); | ||
| do | ||
| { | ||
| std::this_thread::yield(); | ||
| while (existing == cancelling_ptr) | ||
| { |
|
Turns out 2 of the 3 fixes were independently discovered and fixes proposed in #1599. |
|
Doesn't this have the same bug as before? I also noticed that cancellation propagation fails when the couroutine was already cancelled. |
@Clyraz My version spins in the inner
Yes, this is the troublesome "case 4" mentioned in the PR description that will require a broader fix. |
Yeah I just realized upon further look, stupid me. |
The code failed to handle three cases.
The m_canceller member has one of these three values:
In case 1, we should not overwrite the canceling_ptr with the function pointer, because only the code doing the cancel() can transition into/out of cancelling_ptr.
In case 2, we intended to spin until the m_canceller is no longer cancelling_ptr, but we used m_canceller.exchange(nullptr) in a loop, which means that if m_canceller was cancelling_ptr, we overwrite it with nullptr. As a result, the "while" loop always exits after one iteration. We need to spin on the m_canceller without modifying it if it is cancelling_ptr.
In case 3, cancel() function resets m_cancelling back to nullptr, even if the value was cancelling_ptr on entry, prematurely declaring that the existing cancel() has completed. If the original value was cancelling_ptr, we should leave it that way.
There are still other cases not handled:
In this case, we never call the canceller, so the coroutine fails to propagate cancellation. This will require a broader fix, so I'm not going to fix it in this PR. This PR is primarily about fixing the crash caused by case 2. Cases 1 and 3 were fixed opportunistically.