SERVER-20596 do less work inside the ClusterCursorManager lock

5 files changed, 90 insertions, 40 deletions

diff --git a/src/mongo/s/query/cluster_client_cursor_mock.cpp b/src/mongo/s/query/cluster_client_cursor_mock.cpp
index 402f2fd7ef8..97e999c6939 100644
--- a/src/mongo/s/query/cluster_client_cursor_mock.cpp
+++ b/src/mongo/s/query/cluster_client_cursor_mock.cpp
@@ -80,7 +80,11 @@ void ClusterClientCursorMock::queueResult(const BSONObj& obj) {
 }
 
 bool ClusterClientCursorMock::remotesExhausted() {
-    MONGO_UNREACHABLE;
+    return _remotesExhausted;
+}
+
+void ClusterClientCursorMock::markRemotesNotExhausted() {
+    _remotesExhausted = false;
 }
 
 void ClusterClientCursorMock::queueError(Status status) {
diff --git a/src/mongo/s/query/cluster_client_cursor_mock.h b/src/mongo/s/query/cluster_client_cursor_mock.h
index d538e482773..a8515ba61ce 100644
--- a/src/mongo/s/query/cluster_client_cursor_mock.h
+++ b/src/mongo/s/query/cluster_client_cursor_mock.h
@@ -54,10 +54,13 @@ public:
     void queueResult(const BSONObj& obj) final;
 
     /**
-     * Not used.
+     * Returns true unless marked as having non-exhausted remote cursors via
+     * markRemotesNotExhausted().
      */
     bool remotesExhausted() final;
 
+    void markRemotesNotExhausted();
+
     /**
      * Queues an error response.
      */
@@ -71,6 +74,8 @@ private:
 
     // Number of returned documents.
     long long _numReturnedSoFar = 0;
+
+    bool _remotesExhausted = true;
 };
 
 }  // namespace mongo
diff --git a/src/mongo/s/query/cluster_cursor_manager.cpp b/src/mongo/s/query/cluster_cursor_manager.cpp
index 7f09137a7ac..ff04edd9f36 100644
--- a/src/mongo/s/query/cluster_cursor_manager.cpp
+++ b/src/mongo/s/query/cluster_cursor_manager.cpp
@@ -163,8 +163,8 @@ void ClusterCursorManager::PinnedCursor::returnAndKillCursor() {
 }
 
 ClusterCursorManager::ClusterCursorManager(ClockSource* clockSource)
-    : _pseudoRandom(std::unique_ptr<SecureRandom>(SecureRandom::create())->nextInt64()),
-      _clockSource(clockSource) {
+    : _clockSource(clockSource),
+      _pseudoRandom(std::unique_ptr<SecureRandom>(SecureRandom::create())->nextInt64()) {
     invariant(_clockSource);
 }
 
@@ -178,6 +178,9 @@ ClusterCursorManager::PinnedCursor ClusterCursorManager::registerCursor(
     const NamespaceString& nss,
     CursorType cursorType,
     CursorLifetime cursorLifetime) {
+    // Read the clock out of the lock.
+    const auto now = _clockSource->now();
+
     stdx::lock_guard<stdx::mutex> lk(_mutex);
 
     invariant(cursor);
@@ -214,8 +217,8 @@ ClusterCursorManager::PinnedCursor ClusterCursorManager::registerCursor(
     } while (cursorId == 0 || entryMap.count(cursorId) > 0);
 
     // Create a new CursorEntry and register it in the CursorEntryContainer's map.
-    auto emplaceResult = entryMap.emplace(
-        cursorId, CursorEntry(std::move(cursor), cursorType, cursorLifetime, _clockSource->now()));
+    auto emplaceResult =
+        entryMap.emplace(cursorId, CursorEntry(std::move(cursor), cursorType, cursorLifetime, now));
     invariant(emplaceResult.second);
 
     // Pin and return the cursor.  Note that pinning a cursor transfers ownership of the underlying
@@ -227,6 +230,9 @@ ClusterCursorManager::PinnedCursor ClusterCursorManager::registerCursor(
 
 StatusWith<ClusterCursorManager::PinnedCursor> ClusterCursorManager::checkOutCursor(
     const NamespaceString& nss, CursorId cursorId) {
+    // Read the clock out of the lock.
+    const auto now = _clockSource->now();
+
     stdx::lock_guard<stdx::mutex> lk(_mutex);
 
     CursorEntry* entry = getEntry_inlock(nss, cursorId);
@@ -241,7 +247,7 @@ StatusWith<ClusterCursorManager::PinnedCursor> ClusterCursorManager::checkOutCur
         return cursorInUseStatus(nss, cursorId);
     }
 
-    entry->setLastActive(_clockSource->now());
+    entry->setLastActive(now);
 
     // Note that pinning a cursor transfers ownership of the underlying ClusterClientCursor object
     // to the pin; the CursorEntry is left with a null ClusterClientCursor.
@@ -252,10 +258,12 @@ void ClusterCursorManager::checkInCursor(std::unique_ptr<ClusterClientCursor> cu
                                          const NamespaceString& nss,
                                          CursorId cursorId,
                                          CursorState cursorState) {
-    stdx::lock_guard<stdx::mutex> lk(_mutex);
+    stdx::unique_lock<stdx::mutex> lk(_mutex);
 
     invariant(cursor);
 
+    const bool remotesExhausted = cursor->remotesExhausted();
+
     CursorEntry* entry = getEntry_inlock(nss, cursorId);
     invariant(entry);
 
@@ -265,9 +273,21 @@ void ClusterCursorManager::checkInCursor(std::unique_ptr<ClusterClientCursor> cu
         return;
     }
 
-    // The cursor is exhausted, and the cursor doesn't already have a pending kill. Schedule for
-    // deletion by setting the kill pending flag.
-    entry->setKillPending();
+    if (!remotesExhausted) {
+        // The cursor still has open remote cursors that need to be cleaned up. Schedule for
+        // deletion by the reaper thread by setting the kill pending flag.
+        entry->setKillPending();
+        return;
+    }
+
+    // The cursor is exhausted, is not already scheduled for deletion, and does not have any
+    // remote cursor state left to clean up. We can delete the cursor right away.
+    auto detachedCursor = detachCursor_inlock(nss, cursorId);
+    invariantOK(detachedCursor.getStatus());
+
+    // Deletion of the cursor can happen out of the lock.
+    lk.unlock();
+    detachedCursor.getValue().reset();
 }
 
 Status ClusterCursorManager::killCursor(const NamespaceString& nss, CursorId cursorId) {
@@ -336,8 +356,8 @@ void ClusterCursorManager::reapZombieCursors() {
 
         lk.unlock();
         zombieCursor.getValue()->kill();
+        zombieCursor.getValue().reset();
         lk.lock();
-        // Cursor deleted as it goes out of scope.
     }
 }
 
diff --git a/src/mongo/s/query/cluster_cursor_manager.h b/src/mongo/s/query/cluster_cursor_manager.h
index 089507ec338..d10f58d5f9c 100644
--- a/src/mongo/s/query/cluster_cursor_manager.h
+++ b/src/mongo/s/query/cluster_cursor_manager.h
@@ -327,9 +327,14 @@ private:
 
     /**
      * Transfers ownership of the given pinned cursor back to the manager, and moves the cursor to
-     * the 'idle' state.  If 'cursorState' is 'Exhausted' and the cursor is not marked as 'kill
-     * pending', destroys the cursor (if the cursor is marked as 'kill pending', destruction of the
-     * cursor is delayed until it reaped).  Thread-safe.
+     * the 'idle' state.
+     *
+     * If 'cursorState' is 'Exhausted', the cursor will be destroyed.  However, destruction will be
+     * delayed until reapZombieCursors() is called under the following circumstances:
+     *   - The cursor is already marked as 'kill pending'.
+     *   - The cursor is managing open remote cursors which still need to be cleaned up.
+     *
+     * Thread-safe.
      *
      * Intentionally private.  Clients should use public methods on PinnedCursor to check a cursor
      * back in.
@@ -480,15 +485,16 @@ private:
         CursorEntryMap entryMap;
     };
 
-    // Synchronizes access to all private state.
+    // Clock source.  Used when the 'last active' time for a cursor needs to be set/updated.  May be
+    // concurrently accessed by multiple threads.
+    ClockSource* _clockSource;
+
+    // Synchronizes access to all private state variables below.
     mutable stdx::mutex _mutex;
 
     // Randomness source.  Used for cursor id generation.
     PseudoRandom _pseudoRandom;
 
-    // Clock source.  Used when the 'last active' time for a cursor needs to be set/updated.
-    ClockSource* _clockSource;
-
     // Map from cursor id prefix to associated namespace.  Exists only to provide namespace lookup
     // for (deprecated) getNamespaceForCursorId() method.
     //
diff --git a/src/mongo/s/query/cluster_cursor_manager_test.cpp b/src/mongo/s/query/cluster_cursor_manager_test.cpp
index d9da5a5957d..26cebbb0261 100644
--- a/src/mongo/s/query/cluster_cursor_manager_test.cpp
+++ b/src/mongo/s/query/cluster_cursor_manager_test.cpp
@@ -664,8 +664,8 @@ TEST_F(ClusterCursorManagerTest, PinnedCursorReturnCursorNotExhausted) {
     ASSERT_OK(checkedOutCursor.getStatus());
 }
 
-// Test that returning a pinned cursor with 'Exhausted' correctly de-registers the cursor, and
-// leaves the pin owning no cursor.
+// Test that returning a pinned cursor with 'Exhausted' correctly de-registers and destroys the
+// cursor, and leaves the pin owning no cursor.
 TEST_F(ClusterCursorManagerTest, PinnedCursorReturnCursorExhausted) {
     auto registeredCursor =
         getManager()->registerCursor(allocateMockCursor(),
@@ -677,7 +677,41 @@ TEST_F(ClusterCursorManagerTest, PinnedCursorReturnCursorExhausted) {
     ASSERT_OK(registeredCursor.next().getStatus());
     registeredCursor.returnCursor(ClusterCursorManager::CursorState::Exhausted);
     ASSERT_EQ(0, registeredCursor.getCursorId());
+
+    // Cursor should have been destroyed without ever being killed. To be sure that the cursor has
+    // not been marked kill pending but not yet destroyed (i.e. that the cursor is not a zombie), we
+    // reapZombieCursors() and check that the cursor still has not been killed.
     ASSERT_NOT_OK(getManager()->checkOutCursor(nss, cursorId).getStatus());
+    ASSERT(!isMockCursorKilled(0));
+    getManager()->reapZombieCursors();
+    ASSERT(!isMockCursorKilled(0));
+}
+
+// Test that when a cursor is returned as exhausted but is still managing non-exhausted remote
+// cursors, the cursor is not destroyed immediately. Instead, it should be marked kill pending, and
+// should be killed and destroyed by reapZombieCursors().
+TEST_F(ClusterCursorManagerTest, PinnedCursorReturnCursorExhaustedWithNonExhaustedRemotes) {
+    auto mockCursor = allocateMockCursor();
+
+    // The mock should indicate that is has open remote cursors.
+    mockCursor->markRemotesNotExhausted();
+
+    auto registeredCursor =
+        getManager()->registerCursor(std::move(mockCursor),
+                                     nss,
+                                     ClusterCursorManager::CursorType::NamespaceNotSharded,
+                                     ClusterCursorManager::CursorLifetime::Mortal);
+    CursorId cursorId = registeredCursor.getCursorId();
+    ASSERT_NE(0, cursorId);
+    ASSERT_OK(registeredCursor.next().getStatus());
+    registeredCursor.returnCursor(ClusterCursorManager::CursorState::Exhausted);
+    ASSERT_EQ(0, registeredCursor.getCursorId());
+
+    // Cursor should be kill pending, so it will be killed during reaping.
+    ASSERT_NOT_OK(getManager()->checkOutCursor(nss, cursorId).getStatus());
+    ASSERT(!isMockCursorKilled(0));
+    getManager()->reapZombieCursors();
+    ASSERT(isMockCursorKilled(0));
 }
 
 // Test that the PinnedCursor move assignment operator correctly kills the cursor if it has not yet
@@ -708,25 +742,6 @@ TEST_F(ClusterCursorManagerTest, PinnedCursorDestructorKill) {
     ASSERT(isMockCursorKilled(0));
 }
 
-// Test that cursors checked back in as exhausted are killed. Even exhausted cursors may have open
-// cursors on the remote shards, so they need to be killed in order to properly clean up these
-// remote cursors.
-TEST_F(ClusterCursorManagerTest, ExhaustedCursorKill) {
-    auto pinnedCursor =
-        getManager()->registerCursor(allocateMockCursor(),
-                                     nss,
-                                     ClusterCursorManager::CursorType::NamespaceNotSharded,
-                                     ClusterCursorManager::CursorLifetime::Mortal);
-    ASSERT(!isMockCursorKilled(0));
-    pinnedCursor.returnCursor(ClusterCursorManager::CursorState::Exhausted);
-
-    // Despite being returned as exhausted, the cursor should be alive until zombie cursors are
-    // reaped.
-    ASSERT(!isMockCursorKilled(0));
-    getManager()->reapZombieCursors();
-    ASSERT(isMockCursorKilled(0));
-}
-
 }  // namespace
 
 }  // namespace mongo