diff options
author | Jason Carey <jcarey@argv.me> | 2019-07-24 10:16:38 -0400 |
---|---|---|
committer | Jason Carey <jcarey@argv.me> | 2019-07-24 13:31:00 -0400 |
commit | aa5fef386e734d42ad8df1d0d37021daf38fc56c (patch) | |
tree | efcddaed6ef57db06cd59562e48d3b46836aefda | |
parent | e324999f4444483c01fee1a7233bcfd31df2aed0 (diff) | |
download | mongo-aa5fef386e734d42ad8df1d0d37021daf38fc56c.tar.gz |
SERVER-42370 Fix SharedStateImpl::addChild()
There's an unfortunate race between addChild and transitionToFinished,
where the caller of addChild can observe a kInit shared state, as can
transitionToFinished, and then the caller of addChild can add a child
that is never fulfilled by the caller of transitionToFinished.
It looks like:
addChild() | transitionToFinished()
==============================================
loadState, see init |
acquire mutex |
loadState, see init |
| transition to finished, observed init
attempt to cas, fail |
| return, doing nothing
add to children |
strand the child |
The fix is to check to see if our compare_exchange failed, in which case
we need to fulfill ourselves
(cherry picked from commit e82c7b495f4c4cebf12f4f1a19c2922b1d1d2683)
-rw-r--r-- | src/mongo/util/future_impl.h | 24 |
1 files changed, 15 insertions, 9 deletions
diff --git a/src/mongo/util/future_impl.h b/src/mongo/util/future_impl.h index 1deacc38733..1a37254a52b 100644 --- a/src/mongo/util/future_impl.h +++ b/src/mongo/util/future_impl.h @@ -479,23 +479,29 @@ struct SharedStateImpl final : SharedStateBase { } auto lk = stdx::unique_lock(mx); + auto oldState = state.load(std::memory_order_acquire); + if (oldState == SSBState::kInit) { + // On the success path, our reads and writes to children are protected by the mutex + // + // On the failure path, we raced with transitionToFinished() and lost, so we need to + // synchronize with it via acquire before accessing the results since it wouldn't have + // taken the mutex. + state.compare_exchange_strong(oldState, + SSBState::kWaitingOrHaveChildren, + std::memory_order_relaxed, + std::memory_order_acquire); + } if (oldState == SSBState::kFinished) { lk.unlock(); out->fillFromConst(*this); return out; } - - if (oldState == SSBState::kInit) { - // memory ordering can be relaxed because that will be handled by the mutex. - state.compare_exchange_strong( - oldState, SSBState::kWaitingOrHaveChildren, std::memory_order_relaxed); - } dassert(oldState != SSBState::kHaveCallback); - // If oldState became kFinished after we checked (and therefore after we acquired the lock), - // the returned continuation will be completed by the promise side once it acquires the lock - // since we are adding ourself to the chain here. + // If oldState became kFinished after we checked (or successfully stored + // kWaitingOrHaveChildren), the returned continuation will be completed by the promise side + // once it acquires the lock since we are adding ourself to the chain here. children.emplace_front(out.get(), /*add ref*/ false); out->threadUnsafeIncRefCountTo(2); |