SERVER-39960 Simplify opCtx::markKilled

Operation context currently relies on an elaborate dance between the
client lock, _waitMutex, _waitCV and _numKillers to allow markKilled to
tap the condvar an opctx is waiting on.

After the introduction of batons on all opctx's, this is no longer
necessary (as batons have their own support for being woken, while
waiting on a condvar). Removing the special killing code will simplify
opctx, and remove a lot of extra book keeping.

7 files changed, 48 insertions, 125 deletions

diff --git a/src/mongo/db/default_baton.cpp b/src/mongo/db/default_baton.cpp
index 8aeef1bec64..1154d377ccb 100644
--- a/src/mongo/db/default_baton.cpp
+++ b/src/mongo/db/default_baton.cpp
@@ -57,11 +57,8 @@ void DefaultBaton::detachImpl() noexcept {
     {
         stdx::lock_guard<stdx::mutex> lk(_mutex);
 
-        {
-            stdx::lock_guard<Client> lk(*_opCtx->getClient());
-            invariant(_opCtx->getBaton().get() == this);
-            _opCtx->setBaton(nullptr);
-        }
+        invariant(_opCtx->getBaton().get() == this);
+        _opCtx->setBaton(nullptr);
 
         _opCtx = nullptr;
         _hasIngressSocket = false;
diff --git a/src/mongo/db/operation_context.cpp b/src/mongo/db/operation_context.cpp
index 271ea547fa2..2e9e4c0a00e 100644
--- a/src/mongo/db/operation_context.cpp
+++ b/src/mongo/db/operation_context.cpp
@@ -246,49 +246,26 @@ Status OperationContext::checkForInterruptNoAssert() noexcept {
     return Status::OK();
 }
 
-// Theory of operation for waitForConditionOrInterruptNoAssertUntil and markKilled:
-//
-// An operation indicates to potential killers that it is waiting on a condition variable by setting
-// _waitMutex and _waitCV, while holding the lock on its parent Client. It then unlocks its Client,
-// unblocking any killers, which are required to have locked the Client before calling markKilled.
-//
-// When _waitMutex and _waitCV are set, killers must lock _waitMutex before setting the _killCode,
-// and must signal _waitCV before releasing _waitMutex. Unfortunately, they must lock _waitMutex
-// without holding a lock on Client to avoid a deadlock with callers of
-// waitForConditionOrInterruptNoAssertUntil(). So, in the event that _waitMutex is set, the killer
-// increments _numKillers, drops the Client lock, acquires _waitMutex and then re-acquires the
-// Client lock. We know that the Client, its OperationContext and _waitMutex will remain valid
-// during this period because the caller of waitForConditionOrInterruptNoAssertUntil will not return
-// while _numKillers > 0 and will not return until it has itself reacquired _waitMutex. Instead,
-// that caller will keep waiting on _waitCV until _numKillers drops to 0.
+
+// waitForConditionOrInterruptNoAssertUntil returns when:
 //
-// In essence, when _waitMutex is set, _killCode is guarded by _waitMutex and _waitCV, but when
-// _waitMutex is not set, it is guarded by the Client spinlock. Changing _waitMutex is itself
-// guarded by the Client spinlock and _numKillers.
+// Normal condvar wait criteria:
+// - cv is notified
+// - deadline is passed
 //
-// When _numKillers does drop to 0, the waiter will null out _waitMutex and _waitCV.
+// OperationContext kill criteria:
+// - _deadline is passed (artificial deadline or maxTimeMS)
+// - markKilled is called (killOp)
 //
-// This implementation adds a minimum of two spinlock acquire-release pairs to every condition
-// variable wait.
+// Baton criteria:
+// - _baton is notified (someone's queuing work for the baton)
+// - _baton::run returns (timeout fired / networking is ready / socket disconnected)
 StatusWith<stdx::cv_status> OperationContext::waitForConditionOrInterruptNoAssertUntil(
     stdx::condition_variable& cv, stdx::unique_lock<stdx::mutex>& m, Date_t deadline) noexcept {
     invariant(getClient());
-    {
-        stdx::lock_guard<Client> clientLock(*getClient());
-        invariant(!_waitMutex);
-        invariant(!_waitCV);
-        invariant(_waitThread == kNoWaiterThread);
-        invariant(0 == _numKillers);
-
-        // This interrupt check must be done while holding the client lock, so as not to race with a
-        // concurrent caller of markKilled.
-        auto status = checkForInterruptNoAssert();
-        if (!status.isOK()) {
-            return status;
-        }
-        _waitMutex = m.mutex();
-        _waitCV = &cv;
-        _waitThread = stdx::this_thread::get_id();
+
+    if (auto status = checkForInterruptNoAssert(); !status.isOK()) {
+        return status;
     }
 
     // If the maxTimeNeverTimeOut failpoint is set, behave as though the operation's deadline does
@@ -314,22 +291,10 @@ StatusWith<stdx::cv_status> OperationContext::waitForConditionOrInterruptNoAsser
             cv, m, deadline, _baton.get());
     }();
 
-    // Continue waiting on cv until no other thread is attempting to kill this one.
-    Waitable::wait(_baton.get(), getServiceContext()->getPreciseClockSource(), cv, m, [this] {
-        stdx::lock_guard<Client> clientLock(*getClient());
-        if (0 == _numKillers) {
-            _waitMutex = nullptr;
-            _waitCV = nullptr;
-            _waitThread = kNoWaiterThread;
-            return true;
-        }
-        return false;
-    });
-
-    auto status = checkForInterruptNoAssert();
-    if (!status.isOK()) {
+    if (auto status = checkForInterruptNoAssert(); !status.isOK()) {
         return status;
     }
+
     if (opHasDeadline && waitStatus == stdx::cv_status::timeout && deadline == getDeadline()) {
         // It's possible that the system clock used in stdx::condition_variable::wait_until
         // is slightly ahead of the FastClock used in checkForInterrupt. In this case,
@@ -352,33 +317,8 @@ void OperationContext::markKilled(ErrorCodes::Error killCode) {
         log() << "operation was interrupted because a client disconnected";
     }
 
-    stdx::unique_lock<stdx::mutex> lkWaitMutex;
-
-    // If we have a _waitMutex, it means this opCtx is currently blocked in
-    // waitForConditionOrInterrupt.
-    //
-    // From there, we also know which thread is actually doing that waiting (it's recorded in
-    // _waitThread).  If that thread isn't our thread, it's necessary to do the regular numKillers
-    // song and dance mentioned in waitForConditionOrInterrupt.
-    //
-    // If it is our thread, we know that we're currently inside that call to
-    // waitForConditionOrInterrupt and are being invoked by a callback run from Baton->run.  And
-    // that means we don't need to deal with the waitMutex and waitCV (because we're running
-    // callbacks, which means run is returning, which means we'll be checking _killCode in the near
-    // future).
-    if (_waitMutex && stdx::this_thread::get_id() != _waitThread) {
-        invariant(++_numKillers > 0);
-        getClient()->unlock();
-        ON_BLOCK_EXIT([this] {
-            getClient()->lock();
-            invariant(--_numKillers >= 0);
-        });
-        lkWaitMutex = stdx::unique_lock<stdx::mutex>{*_waitMutex};
-    }
-    _killCode.compareAndSwap(ErrorCodes::OK, killCode);
-    if (lkWaitMutex && _numKillers == 0) {
-        invariant(_waitCV);
-        _waitCV->notify_all();
+    if (_killCode.compareAndSwap(ErrorCodes::OK, killCode) == ErrorCodes::OK) {
+        _baton->notify();
     }
 }
 
diff --git a/src/mongo/db/operation_context.h b/src/mongo/db/operation_context.h
index 56a363aab3e..ffa9678d7a0 100644
--- a/src/mongo/db/operation_context.h
+++ b/src/mongo/db/operation_context.h
@@ -446,27 +446,8 @@ private:
     // once from OK to some kill code.
     AtomicWord<ErrorCodes::Error> _killCode{ErrorCodes::OK};
 
-    // A transport Baton associated with the operation. The presence of this object implies that a
-    // client thread is doing it's own async networking by blocking on it's own thread.
     BatonHandle _baton;
 
-    // If non-null, _waitMutex and _waitCV are the (mutex, condition variable) pair that the
-    // operation is currently waiting on inside a call to waitForConditionOrInterrupt...().
-    //
-    // _waitThread is the calling thread's thread id.
-    //
-    // All access guarded by the Client's lock.
-    stdx::mutex* _waitMutex = nullptr;
-    stdx::condition_variable* _waitCV = nullptr;
-    stdx::thread::id _waitThread;
-
-    // If _waitMutex and _waitCV are non-null, this is the number of threads in a call to markKilled
-    // actively attempting to kill the operation. If this value is non-zero, the operation is inside
-    // waitForConditionOrInterrupt...() and must stay there until _numKillers reaches 0.
-    //
-    // All access guarded by the Client's lock.
-    int _numKillers = 0;
-
     WriteConcernOptions _writeConcern;
 
     // The timepoint at which this operation exceeds its time limit.
diff --git a/src/mongo/db/service_context.cpp b/src/mongo/db/service_context.cpp
index ee26756a642..ac8937dbc6c 100644
--- a/src/mongo/db/service_context.cpp
+++ b/src/mongo/db/service_context.cpp
@@ -248,21 +248,20 @@ ServiceContext::UniqueOperationContext ServiceContext::makeOperationContext(Clie
         opCtx->setRecoveryUnit(std::make_unique<RecoveryUnitNoop>(),
                                WriteUnitOfWork::RecoveryUnitState::kNotInUnitOfWork);
     }
-    {
-        stdx::lock_guard<Client> lk(*client);
-        client->setOperationContext(opCtx.get());
-    }
+    // The baton must be attached before attaching to a client
     if (_transportLayer) {
         _transportLayer->makeBaton(opCtx.get());
     } else {
         makeBaton(opCtx.get());
     }
+    {
+        stdx::lock_guard<Client> lk(*client);
+        client->setOperationContext(opCtx.get());
+    }
     return UniqueOperationContext(opCtx.release());
 };
 
 void ServiceContext::OperationContextDeleter::operator()(OperationContext* opCtx) const {
-    opCtx->getBaton()->detach();
-
     auto client = opCtx->getClient();
     if (client->session()) {
         _numCurrentOps.subtractAndFetch(1);
@@ -272,6 +271,8 @@ void ServiceContext::OperationContextDeleter::operator()(OperationContext* opCtx
         stdx::lock_guard<Client> lk(*client);
         client->resetOperationContext();
     }
+    opCtx->getBaton()->detach();
+
     onDestroy(opCtx, service->_clientObservers);
     delete opCtx;
 }
@@ -413,13 +414,10 @@ void ServiceContext::ServiceContextDeleter::operator()(ServiceContext* service)
 }
 
 BatonHandle ServiceContext::makeBaton(OperationContext* opCtx) const {
-    auto baton = std::make_shared<DefaultBaton>(opCtx);
+    invariant(!opCtx->getBaton());
 
-    {
-        stdx::lock_guard<Client> lk(*opCtx->getClient());
-        invariant(!opCtx->getBaton());
-        opCtx->setBaton(baton);
-    }
+    auto baton = std::make_shared<DefaultBaton>(opCtx);
+    opCtx->setBaton(baton);
 
     return baton;
 }
diff --git a/src/mongo/stdx/condition_variable.h b/src/mongo/stdx/condition_variable.h
index 33e42f927fa..031d5005484 100644
--- a/src/mongo/stdx/condition_variable.h
+++ b/src/mongo/stdx/condition_variable.h
@@ -47,6 +47,19 @@ namespace mongo {
  */
 class Notifyable {
 public:
+    // !!! PAY ATTENTION, THERE IS DANGER HERE !!!
+    //
+    // Implementers of the notifyable api must be level triggered by notify, rather than edge
+    // triggered.
+    //
+    // I.e. a call to notify() must either unblock the notifyable immediately, if it is currently
+    // blocked, or unblock it the next time it would wait, if it is not currently blocked.
+    //
+    // In addition to unblocking, the notifyable should also atomically consume that notification
+    // state as a result of waking.  I.e. any number of calls to notify before or during a wait must
+    // unblock exactly one wait.
+    //
+    // Notifyable::notify is not like condition_variable::notify_X()
     virtual void notify() noexcept = 0;
 
 protected:
diff --git a/src/mongo/transport/baton_asio_linux.h b/src/mongo/transport/baton_asio_linux.h
index 1b6eb98db4f..c07ce5b32f4 100644
--- a/src/mongo/transport/baton_asio_linux.h
+++ b/src/mongo/transport/baton_asio_linux.h
@@ -394,11 +394,8 @@ private:
         {
             stdx::lock_guard<stdx::mutex> lk(_mutex);
 
-            {
-                stdx::lock_guard<Client> lk(*_opCtx->getClient());
-                invariant(_opCtx->getBaton().get() == this);
-                _opCtx->setBaton(nullptr);
-            }
+            invariant(_opCtx->getBaton().get() == this);
+            _opCtx->setBaton(nullptr);
 
             _opCtx = nullptr;
 
diff --git a/src/mongo/transport/transport_layer_asio.cpp b/src/mongo/transport/transport_layer_asio.cpp
index 8843478a4ee..dc0475b801d 100644
--- a/src/mongo/transport/transport_layer_asio.cpp
+++ b/src/mongo/transport/transport_layer_asio.cpp
@@ -879,13 +879,10 @@ SSLParams::SSLModes TransportLayerASIO::_sslMode() const {
 
 #ifdef __linux__
 BatonHandle TransportLayerASIO::makeBaton(OperationContext* opCtx) const {
-    auto baton = std::make_shared<BatonASIO>(opCtx);
+    invariant(!opCtx->getBaton());
 
-    {
-        stdx::lock_guard<Client> lk(*opCtx->getClient());
-        invariant(!opCtx->getBaton());
-        opCtx->setBaton(baton);
-    }
+    auto baton = std::make_shared<BatonASIO>(opCtx);
+    opCtx->setBaton(baton);
 
     return baton;
 }