SERVER-18277 Clarify locking of Client when accessing its stored OperationContext.

As a side-effect, clean up operation killing in ServiceContextMongoD.

9 files changed, 78 insertions, 43 deletions

diff --git a/src/mongo/db/client.cpp b/src/mongo/db/client.cpp
index 4b220a308dd..a7d3de69b16 100644
--- a/src/mongo/db/client.cpp
+++ b/src/mongo/db/client.cpp
@@ -117,14 +117,11 @@ namespace mongo {
     void Client::setOperationContext(OperationContext* txn) {
         // We can only set the OperationContext once before resetting it.
         invariant(txn != NULL && _txn == NULL);
-
-        boost::unique_lock<SpinLock> uniqueLock(_lock);
         _txn = txn;
     }
 
     void Client::resetOperationContext() {
         invariant(_txn != NULL);
-        boost::unique_lock<SpinLock> uniqueLock(_lock);
         _txn = NULL;
     }
 
diff --git a/src/mongo/db/client.h b/src/mongo/db/client.h
index 63750b69523..597ed07dd70 100644
--- a/src/mongo/db/client.h
+++ b/src/mongo/db/client.h
@@ -86,11 +86,29 @@ namespace mongo {
         void lock() { _lock.lock(); }
         void unlock() { _lock.unlock(); }
 
-        // Changes the currently active operation context on this client. There can only be one
-        // active OperationContext at a time.
+        /**
+         * Sets the active operation context on this client to "txn", which must be non-NULL.
+         *
+         * It is an error to call this method if there is already an operation context on Client.
+         * It is an error to call this on an unlocked client.
+         */
         void setOperationContext(OperationContext* txn);
+
+        /**
+         * Clears the active operation context on this client.
+         *
+         * There must already be such a context set on this client.
+         * It is an error to call this on an unlocked client.
+         */
         void resetOperationContext();
-        const OperationContext* getOperationContext() const { return _txn; }
+
+        /**
+         * Gets the operation context active on this client, or nullptr if there is no such context.
+         *
+         * It is an error to call this method on an unlocked client, or to use the value returned
+         * by this method while the client is not locked.
+         */
+        OperationContext* getOperationContext() { return _txn; }
 
         // TODO(spencer): SERVER-10228 SERVER-14779 Remove this/move it fully into OperationContext.
         bool isInDirectClient() const { return _inDirectClient; }
diff --git a/src/mongo/db/clientlistplugin.cpp b/src/mongo/db/clientlistplugin.cpp
index 9bd2ca06f3f..89f0f5788f1 100644
--- a/src/mongo/db/clientlistplugin.cpp
+++ b/src/mongo/db/clientlistplugin.cpp
@@ -92,7 +92,7 @@ namespace {
                 invariant(client);
 
                 // Make the client stable
-                boost::unique_lock<Client> clientLock(*client);
+                stdx::lock_guard<Client> lk(*client);
                 const OperationContext* txn = client->getOperationContext();
                 if (!txn) continue;
 
@@ -205,7 +205,7 @@ namespace {
                 BSONObjBuilder b;
 
                 // Make the client stable
-                boost::unique_lock<Client> clientLock(*client);
+                stdx::lock_guard<Client> lk(*client);
 
                 client->reportState(b);
 
diff --git a/src/mongo/db/commands/current_op.cpp b/src/mongo/db/commands/current_op.cpp
index 7d3957ea42f..402558814df 100644
--- a/src/mongo/db/commands/current_op.cpp
+++ b/src/mongo/db/commands/current_op.cpp
@@ -106,7 +106,7 @@ namespace mongo {
 
                 invariant(client);
 
-                boost::unique_lock<Client> uniqueLock(*client);
+                stdx::lock_guard<Client> lk(*client);
                 const OperationContext* opCtx = client->getOperationContext();
 
                 if (!includeAll) {
@@ -132,7 +132,7 @@ namespace mongo {
 
                     // LockState
                     Locker::LockerInfo lockerInfo;
-                    client->getOperationContext()->lockState()->getLockerInfo(&lockerInfo);
+                    opCtx->lockState()->getLockerInfo(&lockerInfo);
                     fillLockerInfo(lockerInfo, infoBuilder);
                 }
                 else {
diff --git a/src/mongo/db/curop.cpp b/src/mongo/db/curop.cpp
index 57b3d780d11..640c82063e0 100644
--- a/src/mongo/db/curop.cpp
+++ b/src/mongo/db/curop.cpp
@@ -147,11 +147,7 @@ namespace mongo {
     CurOp* CurOp::get(const OperationContext* opCtx) { return get(*opCtx); }
 
     CurOp* CurOp::get(const OperationContext& opCtx) {
-        return getFromClient(opCtx.getClient());
-    }
-
-    CurOp* CurOp::getFromClient(const Client* client) {
-        return _curopStack(client).top();
+        return _curopStack(opCtx.getClient()).top();
     }
 
     CurOp::CurOp(Client* client) : CurOp(client, &_curopStack(client)) {}
diff --git a/src/mongo/db/curop.h b/src/mongo/db/curop.h
index b3f96d66d3a..66201f53fdd 100644
--- a/src/mongo/db/curop.h
+++ b/src/mongo/db/curop.h
@@ -38,12 +38,10 @@
 #include "mongo/db/server_options.h"
 #include "mongo/platform/atomic_word.h"
 #include "mongo/util/concurrency/spin_lock.h"
-#include "mongo/util/net/hostandport.h"
 #include "mongo/util/progress_meter.h"
 #include "mongo/util/thread_safe_string.h"
 #include "mongo/util/time_support.h"
 
-
 namespace mongo {
 
     class Client;
@@ -197,7 +195,6 @@ namespace mongo {
     class CurOp {
         MONGO_DISALLOW_COPYING(CurOp);
     public:
-        static CurOp* getFromClient(const Client* client);
         static CurOp* get(const OperationContext* opCtx);
         static CurOp* get(const OperationContext& opCtx);
 
diff --git a/src/mongo/db/operation_context_impl.cpp b/src/mongo/db/operation_context_impl.cpp
index 07fd8b2f6ad..b57090e04b9 100644
--- a/src/mongo/db/operation_context_impl.cpp
+++ b/src/mongo/db/operation_context_impl.cpp
@@ -83,11 +83,13 @@ namespace {
         StorageEngine* storageEngine = getGlobalServiceContext()->getGlobalStorageEngine();
         _recovery.reset(storageEngine->newRecoveryUnit());
 
+        stdx::lock_guard<Client> lk(*_client);
         _client->setOperationContext(this);
     }
 
     OperationContextImpl::~OperationContextImpl() {
         _locker->assertEmptyAndReset();
+        stdx::lock_guard<Client> lk(*_client);
         _client->resetOperationContext();
     }
 
diff --git a/src/mongo/db/service_context_d.cpp b/src/mongo/db/service_context_d.cpp
index eaef2f6f639..eb00449c625 100644
--- a/src/mongo/db/service_context_d.cpp
+++ b/src/mongo/db/service_context_d.cpp
@@ -205,11 +205,11 @@ namespace mongo {
     }
 
     void ServiceContextMongoD::setKillAllOperations() {
-        boost::lock_guard<boost::mutex> clientLock(_mutex);
+        stdx::lock_guard<stdx::mutex> clientLock(_mutex);
         _globalKill = true;
-        for (size_t i = 0; i < _killOpListeners.size(); i++) {
+        for (const auto listener : _killOpListeners) {
             try {
-                _killOpListeners[i]->interruptAll();
+                listener->interruptAll();
             }
             catch (...) {
                 std::terminate();
@@ -223,30 +223,38 @@ namespace mongo {
 
     bool ServiceContextMongoD::_killOperationsAssociatedWithClientAndOpId_inlock(
             Client* client, unsigned int opId) {
-        for( CurOp *k = CurOp::getFromClient(client); k; k = k->parent() ) {
+        OperationContext* opCtx = client->getOperationContext();
+        if (!opCtx) {
+            return false;
+        }
+        for( CurOp *k = CurOp::get(opCtx); k; k = k->parent() ) {
             if ( k->opNum() != opId )
                 continue;
 
-            k->kill();
-            for( CurOp *l = CurOp::getFromClient(client); l; l = l->parent() ) {
-                l->kill();
-            }
-
-            for (size_t i = 0; i < _killOpListeners.size(); i++) {
-                try {
-                    _killOpListeners[i]->interrupt(opId);
-                }
-                catch (...) {
-                    std::terminate();
-                }
-            }
+            _killOperation_inlock(opCtx);
             return true;
         }
         return false;
     }
 
+    void ServiceContextMongoD::_killOperation_inlock(OperationContext* opCtx) {
+        for(CurOp *l = CurOp::get(opCtx); l; l = l->parent()) {
+            l->kill();
+        }
+
+        for (const auto listener : _killOpListeners) {
+            try {
+                listener->interrupt(opCtx->getOpID());
+            }
+            catch (...) {
+                std::terminate();
+            }
+        }
+    }
+
     bool ServiceContextMongoD::killOperation(unsigned int opId) {
         for (LockedClientsCursor cursor(this); Client* client = cursor.next();) {
+            stdx::lock_guard<Client> lk(*client);
             bool found = _killOperationsAssociatedWithClientAndOpId_inlock(client, opId);
             if (found) {
                 return true;
@@ -263,17 +271,18 @@ namespace mongo {
                 continue;
             }
 
-            if (CurOp::getFromClient(client)->opNum() == txn->getOpID()) {
-                // Don't kill ourself.
+            stdx::lock_guard<Client> lk(*client);
+            OperationContext* toKill = client->getOperationContext();
+            if (!toKill) {
                 continue;
             }
 
-            bool found = _killOperationsAssociatedWithClientAndOpId_inlock(
-                    client, CurOp::getFromClient(client)->opNum());
-            if (!found) {
-                warning() << "Attempted to kill operation " << CurOp::getFromClient(client)->opNum()
-                          << " but the opId changed";
+            if (toKill->getOpID() == txn->getOpID()) {
+                // Don't kill ourself.
+                continue;
             }
+
+            _killOperation_inlock(toKill);
         }
     }
 
@@ -282,7 +291,7 @@ namespace mongo {
     }
 
     void ServiceContextMongoD::registerKillOpListener(KillOpListenerInterface* listener) {
-        boost::lock_guard<boost::mutex> clientLock(_mutex);
+        stdx::lock_guard<stdx::mutex> clientLock(_mutex);
         _killOpListeners.push_back(listener);
     }
 
diff --git a/src/mongo/db/service_context_d.h b/src/mongo/db/service_context_d.h
index 9437d37aa07..1d707f33d63 100644
--- a/src/mongo/db/service_context_d.h
+++ b/src/mongo/db/service_context_d.h
@@ -80,8 +80,24 @@ namespace mongo {
 
     private:
 
+        /**
+         * Kills the active operation on "client" if that operation is associated with operation id
+         * "opId".
+         *
+         * Returns true if an operation was killed.
+         *
+         * Must only be called by a thread owning both this service context's mutex and the
+         * client's.
+         */
         bool _killOperationsAssociatedWithClientAndOpId_inlock(Client* client, unsigned int opId);
 
+        /**
+         * Kills the given operation.
+         *
+         * Caller must own the service context's _mutex.
+         */
+        void _killOperation_inlock(OperationContext* opCtx);
+
         bool _globalKill;
 
         // protected by parent class's _mutex