SERVER-5101 Make ClientCursor::Pin exception safe and remove its manual release mechanism for failed yield recovery.

4 files changed, 103 insertions, 25 deletions

diff --git a/src/mongo/db/clientcursor.cpp b/src/mongo/db/clientcursor.cpp
index 209279873eb..fb16f295077 100644
--- a/src/mongo/db/clientcursor.cpp
+++ b/src/mongo/db/clientcursor.cpp
@@ -347,6 +347,7 @@ namespace mongo {
             // defensive:
             _cursorid = INVALID_CURSOR_ID;
             _pos = -2;
+            _pinValue = 0;
         }
     }
 
diff --git a/src/mongo/db/clientcursor.h b/src/mongo/db/clientcursor.h
index 0b7d5c405e4..f6ee59bc637 100644
--- a/src/mongo/db/clientcursor.h
+++ b/src/mongo/db/clientcursor.h
@@ -87,38 +87,32 @@ namespace mongo {
            had a bug, it could (or perhaps some sort of attack situation).
         */
         class Pin : boost::noncopyable {
-            ClientCursor *_c;
+            CursorId _cursorid;
         public:
-            ClientCursor * c() { return _c; }
+            ClientCursor *c() const { return ClientCursor::find( _cursorid ); }
             void release() {
-                if( _c ) {
-                    verify( _c->_pinValue >= 100 );
-                    _c->_pinValue -= 100;
-                    _c = 0;
+                ClientCursor *cursor = c();
+                _cursorid = INVALID_CURSOR_ID;
+                if ( cursor ) {
+                    verify( cursor->_pinValue >= 100 );
+                    cursor->_pinValue -= 100;
                 }
             }
-            /**
-             * call this if during a yield, the cursor got deleted
-             * if so, we don't want to use the point address
-             */
-            void deleted() {
-                _c = 0;
-            }
-            ~Pin() { release(); }
-            Pin(long long cursorid) {
-                recursive_scoped_lock lock(ccmutex);
-                _c = ClientCursor::find_inlock(cursorid, true);
-                if( _c ) {
-                    if( _c->_pinValue >= 100 ) {
-                        _c = 0;
-                        uasserted(12051, "clientcursor already in use? driver problem?");
-                    }
-                    _c->_pinValue += 100;
+            ~Pin() { DESTRUCTOR_GUARD( release(); ) }
+            Pin( long long cursorid ) :
+                _cursorid( INVALID_CURSOR_ID ) {
+                recursive_scoped_lock lock( ccmutex );
+                ClientCursor *cursor = ClientCursor::find_inlock( cursorid, true );
+                if ( cursor ) {
+                    uassert( 12051, "clientcursor already in use? driver problem?",
+                            cursor->_pinValue < 100 );
+                    cursor->_pinValue += 100;
+                    _cursorid = cursorid;
                 }
             }
         };
 
-        // This object assures safe and reliable cleanup of a ClientCursor.
+        /** Assures safe and reliable cleanup of a ClientCursor. */
         class Holder : boost::noncopyable {
         public:
             Holder() : _c( 0 ), _id( INVALID_CURSOR_ID ) {}
@@ -143,6 +137,7 @@ namespace mongo {
             }
             operator bool() { return _c; }
             ClientCursor * operator-> () { return _c; }
+            const ClientCursor * operator-> () const { return _c; }
             /** Release ownership of the ClientCursor. */
             void release() {
                 _c = 0;
diff --git a/src/mongo/db/ops/query.cpp b/src/mongo/db/ops/query.cpp
index 4314e989935..17112559043 100644
--- a/src/mongo/db/ops/query.cpp
+++ b/src/mongo/db/ops/query.cpp
@@ -170,7 +170,6 @@ namespace mongo {
                     ClientCursor::erase(cursorid);
                     cursorid = 0;
                     cc = 0;
-                    p.deleted();
                     break;
                 }
             }
diff --git a/src/mongo/dbtests/cursortests.cpp b/src/mongo/dbtests/cursortests.cpp
index 465f382da3e..addf82dc747 100644
--- a/src/mongo/dbtests/cursortests.cpp
+++ b/src/mongo/dbtests/cursortests.cpp
@@ -287,6 +287,86 @@ namespace CursorTests {
         };
 
     } // namespace BtreeCursor
+    
+    namespace ClientCursor {
+
+        using mongo::ClientCursor;
+        
+        static const char * const ns() { return "unittests.cursortests.clientcursor"; }
+
+        namespace Pin {
+
+            class Base {
+            public:
+                Base() :
+                    _ctx( ns() ),
+                    _cursor( theDataFileMgr.findAll( ns() ) ) {
+                        ASSERT( _cursor );
+                        _clientCursor.reset( new ClientCursor( 0, _cursor, ns() ) );
+                }
+            protected:
+                CursorId cursorid() const { return _clientCursor->cursorid(); }
+            private:
+                Lock::GlobalWrite _lk;
+                Client::Context _ctx;
+                shared_ptr<Cursor> _cursor;
+                ClientCursor::Holder _clientCursor;
+            };
+            
+            /** Pin pins a ClientCursor over its lifetime. */
+            class PinCursor : public Base {
+            public:
+                void run() {
+                    assertNotPinned();
+                    {
+                        ClientCursor::Pin pin( cursorid() );
+                        assertPinned();
+                        ASSERT_THROWS( erase(), AssertionException );
+                    }
+                    assertNotPinned();
+                    ASSERT( erase() );
+                }
+            private:
+                void assertPinned() const {
+                    ASSERT( ClientCursor::find( cursorid() ) );
+                }
+                void assertNotPinned() const {
+                    ASSERT_THROWS( ClientCursor::find( cursorid() ), AssertionException );
+                }
+                bool erase() const {
+                    return ClientCursor::erase( cursorid() );
+                }
+            };
+            
+            /** A ClientCursor cannot be pinned twice. */
+            class PinTwice : public Base {
+            public:
+                void run() {
+                    ClientCursor::Pin pin( cursorid() );
+                    ASSERT_THROWS( pinCursor(), AssertionException );
+                }
+            private:
+                void pinCursor() const {
+                    ClientCursor::Pin pin( cursorid() );
+                }
+            };
+            
+            /** Pin behaves properly if its ClientCursor is destroyed early. */
+            class CursorDeleted : public Base {
+            public:
+                void run() {
+                    ClientCursor::Pin pin( cursorid() );
+                    ASSERT( pin.c() );
+                    // Delete the pinned cursor.
+                    ClientCursor::invalidate( ns() );
+                    ASSERT( !pin.c() );
+                    // pin is destroyed safely, even though its ClientCursor was already destroyed.
+                }
+            };
+            
+        } // namespace Pin
+        
+    } // namespace ClientCursor
 
     class All : public Suite {
     public:
@@ -302,6 +382,9 @@ namespace CursorTests {
             add<BtreeCursor::RangeEq>();
             add<BtreeCursor::RangeIn>();
             add<BtreeCursor::AbortImplicitScan>();
+            add<ClientCursor::Pin::PinCursor>();
+            add<ClientCursor::Pin::PinTwice>();
+            add<ClientCursor::Pin::CursorDeleted>();
         }
     } myall;
 } // namespace CursorTests