SERVER-25694 Eliminate race in PlanExecutor cleanup.

Ensures that a collection lock is held in at least MODE_IS while
deregistering a PlanExecutor from the cursor manager. Introduces new
PlanExecutor::dispose() and ClientCursor::dispose() methods that must be
called before destruction of those classes, and ensures they are called
before destruction. These calls will thread an OperationContext all the
way through to DocumentSource::dispose() for each stage in a Pipeline,
which will give DocumentSourceCursor a chance to acquire locks and
deregister its PlanExecutor if necessary.

1 files changed, 36 insertions, 14 deletions

diff --git a/src/mongo/db/clientcursor.cpp b/src/mongo/db/clientcursor.cpp
index 9bf48a6d379..48730e12b8c 100644
--- a/src/mongo/db/clientcursor.cpp
+++ b/src/mongo/db/clientcursor.cpp
@@ -117,7 +117,7 @@ void ClientCursor::init() {
 ClientCursor::~ClientCursor() {
     // Cursors must be unpinned and deregistered from their cursor manager before being deleted.
     invariant(!_isPinned);
-    invariant(!_cursorManager);
+    invariant(_disposed);
 
     cursorStatsOpen.decrement();
     if (isNoTimeout()) {
@@ -125,11 +125,23 @@ ClientCursor::~ClientCursor() {
     }
 }
 
-void ClientCursor::kill() {
-    if (_exec.get())
-        _exec->kill("cursor killed");
+void ClientCursor::markAsKilled(const std::string& reason) {
+    if (_exec) {
+        _exec->markAsKilled(reason);
+    }
+    _killed = true;
+}
+
+void ClientCursor::dispose(OperationContext* opCtx) {
+    if (_disposed) {
+        return;
+    }
+
+    if (_exec) {
+        _exec->dispose(opCtx, _cursorManager);
+    }
 
-    _cursorManager = nullptr;
+    _disposed = true;
 }
 
 //
@@ -167,10 +179,12 @@ void ClientCursor::updateSlaveLocation(OperationContext* opCtx) {
 // Pin methods
 //
 
-ClientCursorPin::ClientCursorPin(ClientCursor* cursor) : _cursor(cursor) {
+ClientCursorPin::ClientCursorPin(OperationContext* opCtx, ClientCursor* cursor)
+    : _opCtx(opCtx), _cursor(cursor) {
     invariant(_cursor);
     invariant(_cursor->_isPinned);
     invariant(_cursor->_cursorManager);
+    invariant(!_cursor->_disposed);
 
     // We keep track of the number of cursors currently pinned. The cursor can become unpinned
     // either by being released back to the cursor manager or by being deleted. A cursor may be
@@ -179,15 +193,16 @@ ClientCursorPin::ClientCursorPin(ClientCursor* cursor) : _cursor(cursor) {
     cursorStatsOpenPinned.increment();
 }
 
-ClientCursorPin::ClientCursorPin(ClientCursorPin&& other) : _cursor(other._cursor) {
+ClientCursorPin::ClientCursorPin(ClientCursorPin&& other)
+    : _opCtx(other._opCtx), _cursor(other._cursor) {
     // The pinned cursor is being transferred to us from another pin. The 'other' pin must have a
     // pinned cursor.
     invariant(other._cursor);
     invariant(other._cursor->_isPinned);
 
     // Be sure to set the 'other' pin's cursor to null in order to transfer ownership to ourself.
-    _cursor = other._cursor;
     other._cursor = nullptr;
+    other._opCtx = nullptr;
 }
 
 ClientCursorPin& ClientCursorPin::operator=(ClientCursorPin&& other) {
@@ -206,6 +221,10 @@ ClientCursorPin& ClientCursorPin::operator=(ClientCursorPin&& other) {
     // Be sure to set the 'other' pin's cursor to null in order to transfer ownership to ourself.
     _cursor = other._cursor;
     other._cursor = nullptr;
+
+    _opCtx = other._opCtx;
+    other._opCtx = nullptr;
+
     return *this;
 }
 
@@ -219,16 +238,16 @@ void ClientCursorPin::release() {
 
     invariant(_cursor->_isPinned);
 
-    if (!_cursor->_cursorManager) {
+    if (_cursor->_killed) {
         // The ClientCursor was killed while we had it.  Therefore, it is our responsibility to
-        // delete it.
+        // call dispose() and delete it.
         deleteUnderlying();
     } else {
         // Unpin the cursor under the collection cursor manager lock.
         _cursor->_cursorManager->unpin(_cursor);
+        cursorStatsOpenPinned.decrement();
     }
 
-    cursorStatsOpenPinned.decrement();
     _cursor = nullptr;
 }
 
@@ -236,16 +255,19 @@ void ClientCursorPin::deleteUnderlying() {
     invariant(_cursor);
     invariant(_cursor->_isPinned);
     // Note the following subtleties of this method's implementation:
-    // - We must unpin the cursor before destruction, since it is an error to destroy a pinned
+    // - We must unpin the cursor before destruction, since it is an error to delete a pinned
     //   cursor.
     // - In addition, we must deregister the cursor before unpinning, since it is an
     //   error to unpin a registered cursor without holding the cursor manager lock (note that
     //   we can't simply unpin with the cursor manager lock here, since we need to guarantee
     //   exclusive ownership of the cursor when we are deleting it).
-    if (_cursor->_cursorManager) {
+
+    if (!_cursor->_killed) {
         _cursor->_cursorManager->deregisterCursor(_cursor);
-        _cursor->kill();
     }
+
+    // Make sure the cursor is disposed and unpinned before being destroyed.
+    _cursor->dispose(_opCtx);
     _cursor->_isPinned = false;
     delete _cursor;