SERVER-15778 Clean up ClientCursor

- Removes ClientCursor::_pinValue and corresponding public methods.
- Adds ClientCursor::_isPinned and ClientCursor::_isNoTimeout and
  corresponding public methods.
- "isAggCursor" is now an immutable property of a ClientCursor.
- It is now an error to destroy a pinned ClientCursor before unpinning
  (and, visibility of ~ClientCursor() changed to private to help
  enforce this).

5 files changed, 89 insertions, 58 deletions

diff --git a/src/mongo/db/catalog/collection_cursor_cache.cpp b/src/mongo/db/catalog/collection_cursor_cache.cpp
index cd5535f1c97..affb12c58be 100644
--- a/src/mongo/db/catalog/collection_cursor_cache.cpp
+++ b/src/mongo/db/catalog/collection_cursor_cache.cpp
@@ -300,13 +300,13 @@ namespace mongo {
 
                 invariant( cc->getExecutor() == NULL || cc->getExecutor()->collection() == NULL );
 
-                // If there is a pinValue >= 100, somebody is actively using the CC and we do
-                // not delete it.  Instead we notify the holder that we killed it.  The holder
-                // will then delete the CC.
-                // pinvalue is <100, so there is nobody actively holding the CC.  We can
-                // safely delete it as nobody is holding the CC.
-
-                if (cc->pinValue() < 100) {
+                // If the CC is pinned, somebody is actively using it and we do not delete it.
+                // Instead we notify the holder that we killed it.  The holder will then delete the
+                // CC.
+                //
+                // If the CC is not pinned, there is nobody actively holding it.  We can safely
+                // delete it.
+                if (!cc->isPinned()) {
                     delete cc;
                 }
             }
@@ -326,7 +326,7 @@ namespace mongo {
                     continue;
                 }
 
-                if (cc->pinValue() >= 100 || cc->isAggCursor) {
+                if (cc->isPinned() || cc->isAggCursor()) {
                     // Pinned cursors need to stay alive, so we leave them around.  Aggregation
                     // cursors also can stay alive (since they don't have their lifetime bound to
                     // the underlying collection).  However, if they have an associated executor, we
@@ -416,8 +416,8 @@ namespace mongo {
         if ( pin ) {
             uassert( 12051,
                      "clientcursor already in use? driver problem?",
-                     cursor->_pinValue < 100 );
-            cursor->_pinValue += 100;
+                     !cursor->isPinned() );
+            cursor->setPinned();
         }
 
         return cursor;
@@ -426,8 +426,8 @@ namespace mongo {
     void CollectionCursorCache::unpin( ClientCursor* cursor ) {
         SimpleMutex::scoped_lock lk( _mutex );
 
-        invariant( cursor->_pinValue >= 100 );
-        cursor->_pinValue -= 100;
+        invariant( cursor->isPinned() );
+        cursor->unsetPinned();
     }
 
     void CollectionCursorCache::getCursorIds( std::set<CursorId>* openCursors ) {
@@ -490,7 +490,7 @@ namespace mongo {
 
         massert( 16089,
                  str::stream() << "Cannot kill active cursor " << id,
-                 cursor->pinValue() < 100 );
+                 !cursor->isPinned() );
 
         cursor->kill();
         _deregisterCursor_inlock( cursor );
diff --git a/src/mongo/db/clientcursor.cpp b/src/mongo/db/clientcursor.cpp
index a516c332fec..c8a5c8c03ab 100644
--- a/src/mongo/db/clientcursor.cpp
+++ b/src/mongo/db/clientcursor.cpp
@@ -69,18 +69,22 @@ namespace mongo {
         return cursorStatsOpen.get();
     }
 
-    ClientCursor::ClientCursor(const Collection* collection, PlanExecutor* exec,
-                               int qopts, const BSONObj query)
-        : _collection( collection ),
-          _countedYet( false ),
+    ClientCursor::ClientCursor(const Collection* collection,
+                               PlanExecutor* exec,
+                               int qopts,
+                               const BSONObj query,
+                               bool isAggCursor)
+        : _collection(collection),
+          _countedYet(false),
+          _isAggCursor(isAggCursor),
           _unownedRU(NULL) {
 
         _exec.reset(exec);
         _ns = exec->ns();
         _query = query;
         _queryOptions = qopts;
-        if ( exec->collection() ) {
-            invariant( collection == exec->collection() );
+        if (exec->collection()) {
+            invariant(collection == exec->collection());
         }
         init();
     }
@@ -88,8 +92,9 @@ namespace mongo {
     ClientCursor::ClientCursor(const Collection* collection)
         : _ns(collection->ns().ns()),
           _collection(collection),
-          _countedYet( false ),
+          _countedYet(false),
           _queryOptions(QueryOption_NoCursorTimeout),
+          _isAggCursor(false),
           _unownedRU(NULL) {
         init();
     }
@@ -97,17 +102,17 @@ namespace mongo {
     void ClientCursor::init() {
         invariant( _collection );
 
-        isAggCursor = false;
+        _isPinned = false;
+        _isNoTimeout = false;
 
         _idleAgeMillis = 0;
         _leftoverMaxTimeMicros = 0;
-        _pinValue = 0;
         _pos = 0;
 
         if (_queryOptions & QueryOption_NoCursorTimeout) {
             // cursors normally timeout after an inactivity period to prevent excess memory use
             // setting this prevents timeout of the cursor in question.
-            ++_pinValue;
+            _isNoTimeout = true;
             cursorStatsOpenNoTimeout.increment();
         }
 
@@ -124,10 +129,12 @@ namespace mongo {
             return;
         }
 
+        invariant( !_isPinned ); // Must call unsetPinned() before invoking destructor.
+
         if ( _countedYet ) {
             _countedYet = false;
             cursorStatsOpen.decrement();
-            if ( _pinValue == 1 || _pinValue == 101 )
+            if ( _isNoTimeout )
                 cursorStatsOpenNoTimeout.decrement();
         }
 
@@ -140,7 +147,7 @@ namespace mongo {
         _collection = NULL;
         _cursorid = INVALID_CURSOR_ID;
         _pos = -2;
-        _pinValue = 0;
+        _isNoTimeout = false;
     }
 
     void ClientCursor::kill() {
@@ -156,7 +163,10 @@ namespace mongo {
 
     bool ClientCursor::shouldTimeout(int millis) {
         _idleAgeMillis += millis;
-        return _idleAgeMillis > 600000 && _pinValue == 0;
+        if (_isNoTimeout || _isPinned) {
+            return false;
+        }
+        return _idleAgeMillis > 600000;
     }
 
     void ClientCursor::setIdleTime( int millis ) {
@@ -223,20 +233,22 @@ namespace mongo {
         if ( !_cursor )
             return;
 
-        invariant( _cursor->pinValue() >= 100 );
+        invariant( _cursor->isPinned() );
 
         if ( _cursor->collection() == NULL ) {
-            // the ClientCursor was killed while we had it
-            // therefore its our responsibility to kill it
-            delete _cursor;
-            _cursor = NULL; // defensive
+            // The ClientCursor was killed while we had it.  Therefore, it is our responsibility to
+            // kill it.
+            deleteUnderlying();
         }
         else {
+            // Unpin the cursor under the collection cursor cache lock.
             _cursor->collection()->cursorCache()->unpin( _cursor );
         }
     }
 
     void ClientCursorPin::deleteUnderlying() {
+        invariant( _cursor->isPinned() );
+        _cursor->unsetPinned(); // ClientCursor objects must be unpinned before destruction.
         delete _cursor;
         _cursor = NULL;
     }
diff --git a/src/mongo/db/clientcursor.h b/src/mongo/db/clientcursor.h
index 10309886402..1c8e694e063 100644
--- a/src/mongo/db/clientcursor.h
+++ b/src/mongo/db/clientcursor.h
@@ -64,15 +64,14 @@ namespace mongo {
         ClientCursor(const Collection* collection,
                      PlanExecutor* exec,
                      int qopts = 0,
-                     const BSONObj query = BSONObj());
+                     const BSONObj query = BSONObj(),
+                     bool isAggCursor = false);
 
         /**
          * This ClientCursor is used to track sharding state.
          */
         ClientCursor(const Collection* collection);
 
-        ~ClientCursor();
-
         //
         // Basic accessors
         //
@@ -80,6 +79,24 @@ namespace mongo {
         CursorId cursorid() const { return _cursorid; }
         std::string ns() const { return _ns; }
         const Collection* collection() const { return _collection; }
+        bool isAggCursor() const { return _isAggCursor; }
+
+        //
+        // Pinning functionality.
+        //
+
+        /**
+         * Marks this ClientCursor as in use.  unsetPinned() must be called before the destructor of
+         * this ClientCursor is invoked.
+         */
+        void setPinned() { _isPinned = true; }
+
+        /**
+         * Marks this ClientCursor as no longer in use.
+         */
+        void unsetPinned() { _isPinned = false; }
+
+        bool isPinned() const { return _isPinned; }
 
         /**
          * This is called when someone is dropping a collection or something else that
@@ -135,18 +152,6 @@ namespace mongo {
         void incPos(int n) { _pos += n; }
         void setPos(int n) { _pos = n; }
 
-        /**
-         * Is this ClientCursor backed by an aggregation pipeline. Defaults to false.
-         *
-         * Agg executors differ from others in that they manage their own locking internally and
-         * should not be killed or destroyed when the underlying collection is deleted.
-         *
-         * Note: This should *not* be set for the internal cursor used as input to an aggregation.
-         */
-        bool isAggCursor;
-
-        unsigned pinValue() const { return _pinValue; }
-
         static long long totalOpen();
 
         //
@@ -191,9 +196,13 @@ namespace mongo {
         RecoveryUnit* releaseOwnedRecoveryUnit();
 
     private:
-        friend class ClientCursorMonitor;
-        friend class CmdCursorInfo;
         friend class CollectionCursorCache;
+        friend class ClientCursorPin;
+
+        /**
+         * Only friends are allowed to destroy ClientCursor objects.
+         */
+        ~ClientCursor();
 
         /**
          * Initialization common between both constructors for the ClientCursor. The database must
@@ -208,12 +217,6 @@ namespace mongo {
         // The ID of the ClientCursor.
         CursorId _cursorid;
 
-        // A variable indicating the state of the ClientCursor.  Possible values:
-        //   0: Normal behavior.  May time out.
-        //   1: No timing out of this ClientCursor.
-        // 100: Currently in use (via ClientCursorPin).
-        unsigned _pinValue;
-
         // The namespace we're operating on.
         std::string _ns;
 
@@ -231,6 +234,21 @@ namespace mongo {
         // See the QueryOptions enum in dbclient.h
         int _queryOptions;
 
+        // Is this ClientCursor backed by an aggregation pipeline?  Defaults to false.
+        //
+        // Agg executors differ from others in that they manage their own locking internally and
+        // should not be killed or destroyed when the underlying collection is deleted.
+        //
+        // Note: This should *not* be set for the internal cursor used as input to an aggregation.
+        bool _isAggCursor;
+
+        // Is this cursor in use?  Defaults to false.
+        bool _isPinned;
+
+        // Is the "no timeout" flag set on this cursor?  If false, this cursor may be targeted for
+        // deletion after an interval of inactivity.  Defaults to false.
+        bool _isNoTimeout;
+
         // TODO: document better.
         OpTime _slaveReadTill;
 
diff --git a/src/mongo/db/commands/pipeline_command.cpp b/src/mongo/db/commands/pipeline_command.cpp
index a813987f351..6449f1f88d0 100644
--- a/src/mongo/db/commands/pipeline_command.cpp
+++ b/src/mongo/db/commands/pipeline_command.cpp
@@ -90,7 +90,7 @@ namespace mongo {
         if (pin) {
             invariant(cursor);
             invariant(cursor->getExecutor() == exec);
-            invariant(cursor->isAggCursor);
+            invariant(cursor->isAggCursor());
         }
 
         BSONElement batchSizeElem = cmdObj.getFieldDotted("cursor.batchSize");
@@ -277,8 +277,9 @@ namespace mongo {
 
                 if (collection) {
                     // XXX
-                    ClientCursor* cursor = new ClientCursor(collection, execHolder.release());
-                    cursor->isAggCursor = true; // enable special locking behavior
+                    const bool isAggCursor = true; // enable special locking behavior
+                    ClientCursor* cursor = new ClientCursor(collection, execHolder.release(), 0,
+                                                            BSONObj(), isAggCursor);
                     pin.reset(new ClientCursorPin(collection, cursor->cursorid()));
                     // Don't add any code between here and the start of the try block.
                 }
diff --git a/src/mongo/db/query/find.cpp b/src/mongo/db/query/find.cpp
index 3d15d9b4ab2..4227e8f229f 100644
--- a/src/mongo/db/query/find.cpp
+++ b/src/mongo/db/query/find.cpp
@@ -275,7 +275,7 @@ namespace mongo {
                 cc->updateSlaveLocation(txn, curop); 
             }
 
-            if (cc->isAggCursor) {
+            if (cc->isAggCursor()) {
                 // Agg cursors handle their own locking internally.
                 ctx.reset(); // unlocks
             }