Uploaded image for project: 'Core Server'
  1. Core Server
  2. SERVER-5999

page fault exception in ClientCursor::aboutToDelete() can leave another Cursor / ClientCursor in an inconsistent state

    • Type: Icon: Bug Bug
    • Resolution: Done
    • Priority: Icon: Major - P3 Major - P3
    • 2.1.2
    • Affects Version/s: None
    • Component/s: Concurrency
    • None
    • ALL

      Before a document is deleted, all cursors pointing at it are advanced until they do not point at it:

                  c->recoverFromYield();
                  DiskLoc tmp1 = c->refLoc();
                  if ( tmp1 != dl ) {
                      // This might indicate a failure to call ClientCursor::prepareToYield() but it can
                      // also happen during correct operation, see SERVER-2009.
                      problem() << "warning: cursor loc " << tmp1 << " does not match byLoc position " << dl << " !" << endl;
                  }
                  else {
                      c->advance();
                  }
                  while (!c->eof() && c->refLoc() == dl) {
                      /* We don't delete at EOF because we want to return "no more results" rather than "no such cursor".
                       * The loop is to handle MultiKey indexes where the deleted record is pointed to by multiple adjacent keys.
                       * In that case we need to advance until we get to the next distinct record or EOF.
                       * SERVER-4154
                       * SERVER-5198
                       * But see SERVER-5725.
                       */
                      c->advance();
                  }
                  cc->updateLocation();
      

      Some of the functions above may generate page fault exceptions for certain types of Cursors. For example an inconsistency can arise if:

      • A cursor is advanced but an exception is thrown before cc->updateLocation() is called, causing the cursor's _lastLoc to be inconsistent with its actual location.
      • A page fault exception is thrown within advance(), leaving the cursor in an inconsistent state.

      The impact of these issues is somewhat mitigated by the fact that a non capped BasicCursor tends to perform few data accesses, and that a BtreeCursor probably does not need to be handled in aboutToDelete() anyway (see SERVER-5725). I have not checked all cases with these cursor types, and with the potential for page fault exceptions in this code there are a lot of cases to check. For BtreeCursor there at least seems to be a problematic case in noteLocation() where page fault exceptions are simulated in debug mode and keyAtKeyOfs is updated but locAtKeyOfs is not updated due to a page fault exception. This could cause the cursor to skip matching documents.

      I am attaching a test case for one issue where a MultiCusor is left in an inconsistent state due to a page fault exception in advance(). I believe there is also a case where a MultiCusor can be left pointing to a bad document, but it is difficult to write a test for that case.

      This test will fail sporadically on debug builds, where a page fault is simulated probabilistically:

      diff --git a/src/mongo/dbtests/cursortests.cpp b/src/mongo/dbtests/cursortests.cpp
      index 38d1197..738f62b 100644
      --- a/src/mongo/dbtests/cursortests.cpp
      +++ b/src/mongo/dbtests/cursortests.cpp
      @@ -298,7 +298,51 @@ namespace CursorTests {
                       client.dropCollection( ns() );
                   }
               };        
      -        
      +
      +        /**
      +         * A cursor is advanced when the document at its current iterate is removed.
      +         */
      +        class HandleDelete : public Base {
      +        public:
      +            void run() {
      +                for( int i = 0; i < 150; ++i ) {
      +                    client.insert( ns(), BSON( "_id" << i ) );
      +                }
      +
      +                boost::shared_ptr<Cursor> cursor;
      +                ClientCursor::Holder clientCursor;
      +                ClientCursor::YieldData yieldData;
      +
      +                {
      +                    Client::ReadContext ctx( ns() );
      +                    // The query will utilize the _id index for both the first and second clauses.
      +                    cursor = NamespaceDetailsTransient::getCursor
      +                            ( ns(), fromjson( "{$or:[{_id:{$gte:0,$lte:148}},{_id:149}]}" ) );
      +                    clientCursor.reset( new ClientCursor( QueryOption_NoCursorTimeout, cursor,
      +                                                          ns() ) );
      +                    // Advance to the last iterate of the first clause.
      +                    ASSERT( cursor->ok() );
      +                    while( cursor->current()[ "_id" ].number() != 148 ) {
      +                        ASSERT( cursor->advance() );
      +                    }
      +                    clientCursor->prepareToYield( yieldData );
      +                }
      +
      +                // Remove the document at the cursor's current position, which will cause the
      +                // cursor to be advanced.
      +                client.remove( ns(), BSON( "_id" << 148 ) );
      +
      +                {
      +                    Client::ReadContext ctx( ns() );
      +                    clientCursor->recoverFromYield( yieldData );
      +                    // Verify that the cursor has another iterate, _id:149, after it is advanced due
      +                    // to _id:148's removal.
      +                    ASSERT( cursor->ok() );
      +                    ASSERT_EQUALS( 149, cursor->current()[ "_id" ].number() );
      +                }
      +            }
      +        };
      +
               /**
                * ClientCursor::aboutToDelete() advances a ClientCursor with a refLoc() matching the
                * document to be deleted.
      @@ -470,6 +514,7 @@ namespace CursorTests {
                   add<BtreeCursor::RangeEq>();
                   add<BtreeCursor::RangeIn>();
                   add<BtreeCursor::AbortImplicitScan>();
      +            add<ClientCursor::HandleDelete>();
                   add<ClientCursor::AboutToDelete>();
                   add<ClientCursor::AboutToDeleteDuplicate>();
                   add<ClientCursor::AboutToDeleteDuplicateNextClause>();
      

            Assignee:
            eliot Eliot Horowitz (Inactive)
            Reporter:
            aaron Aaron Staple
            Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

              Created:
              Updated:
              Resolved: