SERVER-34790 ensure proper locks are held when doing reads or writes, via debug checks

(cherry picked from commit ec2b67ac05f7aaa05b990e18cd7c23109a2e6eb7)

8 files changed, 53 insertions, 13 deletions

diff --git a/src/mongo/db/concurrency/locker.h b/src/mongo/db/concurrency/locker.h
index 6f376599d55..816d35831f1 100644
--- a/src/mongo/db/concurrency/locker.h
+++ b/src/mongo/db/concurrency/locker.h
@@ -427,7 +427,7 @@ public:
      * for special purpose threads, such as FTDC.
      */
     void setShouldAcquireTicket(bool newValue) {
-        invariant(!isLocked());
+        invariant(!isLocked() || isNoop());
         _shouldAcquireTicket = newValue;
     }
     bool shouldAcquireTicket() const {
diff --git a/src/mongo/db/concurrency/locker_noop.h b/src/mongo/db/concurrency/locker_noop.h
index 590489f255f..2c3176aeb75 100644
--- a/src/mongo/db/concurrency/locker_noop.h
+++ b/src/mongo/db/concurrency/locker_noop.h
@@ -205,15 +205,17 @@ public:
     }
 
     virtual bool isLocked() const {
+        // This is necessary because replication makes decisions based on the answer to this, and
+        // we wrote unit tests to test the behavior specifically when this returns "false".
         return false;
     }
 
     virtual bool isWriteLocked() const {
-        return false;
+        return true;
     }
 
     virtual bool isReadLocked() const {
-        MONGO_UNREACHABLE;
+        return true;
     }
 
     virtual bool hasLockPending() const {
diff --git a/src/mongo/db/operation_context_noop.h b/src/mongo/db/operation_context_noop.h
index a28a875fb8e..1f4e51c40f5 100644
--- a/src/mongo/db/operation_context_noop.h
+++ b/src/mongo/db/operation_context_noop.h
@@ -27,14 +27,12 @@
  */
 #pragma once
 
-#include <boost/optional.hpp>
+#include <memory>
 
 #include "mongo/db/concurrency/locker_noop.h"
-#include "mongo/db/logical_session_id.h"
 #include "mongo/db/operation_context.h"
 #include "mongo/db/storage/recovery_unit_noop.h"
-#include "mongo/stdx/memory.h"
-#include "mongo/util/progress_meter.h"
+#include "mongo/db/storage/write_unit_of_work.h"
 
 namespace mongo {
 
@@ -57,7 +55,7 @@ public:
     OperationContextNoop(Client* client, unsigned int opId) : OperationContext(client, opId) {
         setRecoveryUnit(new RecoveryUnitNoop(),
                         WriteUnitOfWork::RecoveryUnitState::kNotInUnitOfWork);
-        setLockState(stdx::make_unique<LockerNoop>());
+        setLockState(std::make_unique<LockerNoop>());
     }
 };
 
diff --git a/src/mongo/db/repl/replication_coordinator_impl.cpp b/src/mongo/db/repl/replication_coordinator_impl.cpp
index a51399cca8c..ec47ed4baa9 100644
--- a/src/mongo/db/repl/replication_coordinator_impl.cpp
+++ b/src/mongo/db/repl/replication_coordinator_impl.cpp
@@ -3339,7 +3339,7 @@ Status ReplicationCoordinatorImpl::updateTerm(OperationContext* opCtx, long long
     }
 
     // Check we haven't acquired any lock, because potential stepdown needs global lock.
-    dassert(!opCtx->lockState()->isLocked());
+    dassert(!opCtx->lockState()->isLocked() || opCtx->lockState()->isNoop());
     TopologyCoordinator::UpdateTermResult updateTermResult;
     EventHandle finishEvh;
 
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_index.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_index.cpp
index 52c6b1b60e8..b90916bc823 100644
--- a/src/mongo/db/storage/wiredtiger/wiredtiger_index.cpp
+++ b/src/mongo/db/storage/wiredtiger/wiredtiger_index.cpp
@@ -316,6 +316,7 @@ Status WiredTigerIndex::insert(OperationContext* opCtx,
                                const BSONObj& key,
                                const RecordId& id,
                                bool dupsAllowed) {
+    dassert(opCtx->lockState()->isWriteLocked());
     invariant(id.isNormal());
     dassert(!hasFieldNames(key));
 
@@ -334,6 +335,7 @@ void WiredTigerIndex::unindex(OperationContext* opCtx,
                               const BSONObj& key,
                               const RecordId& id,
                               bool dupsAllowed) {
+    dassert(opCtx->lockState()->isWriteLocked());
     invariant(id.isNormal());
     dassert(!hasFieldNames(key));
 
@@ -348,6 +350,7 @@ void WiredTigerIndex::unindex(OperationContext* opCtx,
 void WiredTigerIndex::fullValidate(OperationContext* opCtx,
                                    long long* numKeysOut,
                                    ValidateResults* fullResults) const {
+    dassert(opCtx->lockState()->isReadLocked());
     if (fullResults && !WiredTigerRecoveryUnit::get(opCtx)->getSessionCache()->isEphemeral()) {
         int err = WiredTigerUtil::verifyTable(opCtx, _uri, &(fullResults->errors));
         if (err == EBUSY) {
@@ -387,6 +390,7 @@ void WiredTigerIndex::fullValidate(OperationContext* opCtx,
 bool WiredTigerIndex::appendCustomStats(OperationContext* opCtx,
                                         BSONObjBuilder* output,
                                         double scale) const {
+    dassert(opCtx->lockState()->isReadLocked());
     {
         BSONObjBuilder metadata(output->subobjStart("metadata"));
         Status status = WiredTigerUtil::getApplicationMetadata(opCtx, uri(), &metadata);
@@ -467,6 +471,7 @@ Status WiredTigerIndex::touch(OperationContext* opCtx) const {
 
 
 long long WiredTigerIndex::getSpaceUsedBytes(OperationContext* opCtx) const {
+    dassert(opCtx->lockState()->isReadLocked());
     auto ru = WiredTigerRecoveryUnit::get(opCtx);
     WiredTigerSession* session = ru->getSession();
 
@@ -509,6 +514,7 @@ bool WiredTigerIndex::isDup(OperationContext* opCtx,
                             WT_CURSOR* c,
                             const BSONObj& key,
                             const RecordId& id) {
+    dassert(opCtx->lockState()->isReadLocked());
     invariant(unique());
 
     // First check whether the key exists.
@@ -542,6 +548,7 @@ Status WiredTigerIndex::initAsEmpty(OperationContext* opCtx) {
 }
 
 Status WiredTigerIndex::compact(OperationContext* opCtx) {
+    dassert(opCtx->lockState()->isWriteLocked());
     WiredTigerSessionCache* cache = WiredTigerRecoveryUnit::get(opCtx)->getSessionCache();
     if (!cache->isEphemeral()) {
         WT_SESSION* s = WiredTigerRecoveryUnit::get(opCtx)->getSession()->getSession();
@@ -845,6 +852,7 @@ public:
     boost::optional<IndexKeyEntry> seek(const BSONObj& key,
                                         bool inclusive,
                                         RequestedInfo parts) override {
+        dassert(_opCtx->lockState()->isReadLocked());
         const BSONObj finalKey = stripFieldNames(key);
         const auto discriminator =
             _forward == inclusive ? KeyString::kExclusiveBefore : KeyString::kExclusiveAfter;
@@ -859,6 +867,7 @@ public:
 
     boost::optional<IndexKeyEntry> seek(const IndexSeekPoint& seekPoint,
                                         RequestedInfo parts) override {
+        dassert(_opCtx->lockState()->isReadLocked());
         // TODO: don't go to a bson obj then to a KeyString, go straight
         BSONObj key = IndexEntryComparison::makeQueryObject(seekPoint, _forward);
 
@@ -1177,6 +1186,7 @@ public:
     }
 
     boost::optional<IndexKeyEntry> seekExact(const BSONObj& key, RequestedInfo parts) override {
+        dassert(_opCtx->lockState()->isReadLocked());
         _query.resetToKey(stripFieldNames(key), _idx.ordering());
         const WiredTigerItem keyItem(_query.getBuffer(), _query.getSize());
 
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_index_test.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_index_test.cpp
index 7fa7ee6cabe..c2606acf5bf 100644
--- a/src/mongo/db/storage/wiredtiger/wiredtiger_index_test.cpp
+++ b/src/mongo/db/storage/wiredtiger/wiredtiger_index_test.cpp
@@ -37,7 +37,6 @@
 #include "mongo/db/catalog/index_catalog_entry.h"
 #include "mongo/db/index/index_descriptor.h"
 #include "mongo/db/json.h"
-#include "mongo/db/operation_context_noop.h"
 #include "mongo/db/storage/kv/kv_prefix.h"
 #include "mongo/db/storage/sorted_data_interface_test_harness.h"
 #include "mongo/db/storage/wiredtiger/wiredtiger_index.h"
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp
index 2b15932f89f..8453d0c140e 100644
--- a/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp
+++ b/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp
@@ -779,6 +779,8 @@ int64_t WiredTigerRecordStore::cappedMaxSize() const {
 int64_t WiredTigerRecordStore::storageSize(OperationContext* opCtx,
                                            BSONObjBuilder* extraInfo,
                                            int infoLevel) const {
+    dassert(opCtx->lockState()->isReadLocked());
+
     if (_isEphemeral) {
         return dataSize(opCtx);
     }
@@ -808,6 +810,8 @@ RecordData WiredTigerRecordStore::_getData(const WiredTigerCursor& cursor) const
 }
 
 RecordData WiredTigerRecordStore::dataFor(OperationContext* opCtx, const RecordId& id) const {
+    dassert(opCtx->lockState()->isReadLocked());
+
     // ownership passes to the shared_array created below
     WiredTigerCursor curwrap(_uri, _tableId, true, opCtx);
     WT_CURSOR* c = curwrap.get();
@@ -822,6 +826,8 @@ RecordData WiredTigerRecordStore::dataFor(OperationContext* opCtx, const RecordI
 bool WiredTigerRecordStore::findRecord(OperationContext* opCtx,
                                        const RecordId& id,
                                        RecordData* out) const {
+    dassert(opCtx->lockState()->isReadLocked());
+
     WiredTigerCursor curwrap(_uri, _tableId, true, opCtx);
     WT_CURSOR* c = curwrap.get();
     invariant(c);
@@ -836,6 +842,8 @@ bool WiredTigerRecordStore::findRecord(OperationContext* opCtx,
 }
 
 void WiredTigerRecordStore::deleteRecord(OperationContext* opCtx, const RecordId& id) {
+    dassert(opCtx->lockState()->isWriteLocked());
+
     // Deletes should never occur on a capped collection because truncation uses
     // WT_SESSION::truncate().
     invariant(!isCapped());
@@ -1156,6 +1164,8 @@ Status WiredTigerRecordStore::_insertRecords(OperationContext* opCtx,
                                              Record* records,
                                              const Timestamp* timestamps,
                                              size_t nRecords) {
+    dassert(opCtx->lockState()->isWriteLocked());
+
     // We are kind of cheating on capped collections since we write all of them at once ....
     // Simplest way out would be to just block vector writes for everything except oplog ?
     int64_t totalLength = 0;
@@ -1264,6 +1274,8 @@ Status WiredTigerRecordStore::insertRecordsWithDocWriter(OperationContext* opCtx
                                                          const Timestamp* timestamps,
                                                          size_t nDocs,
                                                          RecordId* idsOut) {
+    dassert(opCtx->lockState()->isReadLocked());
+
     std::unique_ptr<Record[]> records(new Record[nDocs]);
 
     // First get all the sizes so we can allocate a single buffer for all documents. Eventually it
@@ -1305,6 +1317,8 @@ Status WiredTigerRecordStore::updateRecord(OperationContext* opCtx,
                                            int len,
                                            bool enforceQuota,
                                            UpdateNotifier* notifier) {
+    dassert(opCtx->lockState()->isWriteLocked());
+
     WiredTigerCursor curwrap(_uri, _tableId, true, opCtx);
     curwrap.assertInActiveTxn();
     WT_CURSOR* c = curwrap.get();
@@ -1415,6 +1429,8 @@ Status WiredTigerRecordStore::compact(OperationContext* opCtx,
                                       RecordStoreCompactAdaptor* adaptor,
                                       const CompactOptions* options,
                                       CompactStats* stats) {
+    dassert(opCtx->lockState()->isWriteLocked());
+
     WiredTigerSessionCache* cache = WiredTigerRecoveryUnit::get(opCtx)->getSessionCache();
     if (!cache->isEphemeral()) {
         WT_SESSION* s = WiredTigerRecoveryUnit::get(opCtx)->getSession()->getSession();
@@ -1430,6 +1446,8 @@ Status WiredTigerRecordStore::validate(OperationContext* opCtx,
                                        ValidateAdaptor* adaptor,
                                        ValidateResults* results,
                                        BSONObjBuilder* output) {
+    dassert(opCtx->lockState()->isReadLocked());
+
     if (!_isEphemeral && level == kValidateFull) {
         int err = WiredTigerUtil::verifyTable(opCtx, _uri, &results->errors);
         if (err == EBUSY) {
@@ -1559,6 +1577,8 @@ void WiredTigerRecordStore::waitForAllEarlierOplogWritesToBeVisible(OperationCon
 
 boost::optional<RecordId> WiredTigerRecordStore::oplogStartHack(
     OperationContext* opCtx, const RecordId& startingPosition) const {
+    dassert(opCtx->lockState()->isReadLocked());
+
     if (!_isOplog)
         return boost::none;
 
@@ -1955,6 +1975,8 @@ void StandardWiredTigerRecordStore::setKey(WT_CURSOR* cursor, RecordId id) const
 
 std::unique_ptr<SeekableRecordCursor> StandardWiredTigerRecordStore::getCursor(
     OperationContext* opCtx, bool forward) const {
+    dassert(opCtx->lockState()->isReadLocked());
+
     if (_isOplog && forward) {
         WiredTigerRecoveryUnit* wru = WiredTigerRecoveryUnit::get(opCtx);
         // If we already have a snapshot we don't know what it can see, unless we know no one
@@ -2006,6 +2028,8 @@ PrefixedWiredTigerRecordStore::PrefixedWiredTigerRecordStore(WiredTigerKVEngine*
 
 std::unique_ptr<SeekableRecordCursor> PrefixedWiredTigerRecordStore::getCursor(
     OperationContext* opCtx, bool forward) const {
+    dassert(opCtx->lockState()->isReadLocked());
+
     if (_isOplog && forward) {
         WiredTigerRecoveryUnit* wru = WiredTigerRecoveryUnit::get(opCtx);
         // If we already have a snapshot we don't know what it can see, unless we know no one
diff --git a/src/mongo/dbtests/storage_timestamp_tests.cpp b/src/mongo/dbtests/storage_timestamp_tests.cpp
index ecef7cf5114..c558b7dd3d5 100644
--- a/src/mongo/dbtests/storage_timestamp_tests.cpp
+++ b/src/mongo/dbtests/storage_timestamp_tests.cpp
@@ -362,6 +362,8 @@ public:
             static_cast<KVStorageEngine*>(_opCtx->getServiceContext()->getStorageEngine())
                 ->getCatalog();
 
+        AutoGetCollection autoColl(_opCtx, nss, LockMode::MODE_IS, LockMode::MODE_IS);
+
         // getCollectionIdent() returns the ident for the given namespace in the KVCatalog.
         // getAllIdents() actually looks in the RecordStore for a list of all idents, and is thus
         // versioned by timestamp. These tests do not do any renames, so we can expect the
@@ -1703,6 +1705,8 @@ public:
         std::string sysProfileIndexIdent;
         for (auto& tuple : {std::tie(nss, collIdent, indexIdent),
                             std::tie(sysProfile, sysProfileIdent, sysProfileIndexIdent)}) {
+            AutoGetCollection autoColl(_opCtx, nss, LockMode::MODE_X, LockMode::MODE_X);
+
             // Save the pre-state idents so we can capture the specific idents related to collection
             // creation.
             std::vector<std::string> origIdents = kvCatalog->getAllIdents(_opCtx);
@@ -1718,8 +1722,6 @@ public:
                 reset(nss);
             }
 
-            AutoGetCollection autoColl(_opCtx, nss, LockMode::MODE_X, LockMode::MODE_X);
-
             // Bind the local values to the variables in the parent scope.
             auto& collIdent = std::get<1>(tuple);
             auto& indexIdent = std::get<2>(tuple);
@@ -2139,7 +2141,11 @@ public:
             auto kvStorageEngine =
                 dynamic_cast<KVStorageEngine*>(_opCtx->getServiceContext()->getStorageEngine());
             KVCatalog* kvCatalog = kvStorageEngine->getCatalog();
-            std::vector<std::string> origIdents = kvCatalog->getAllIdents(_opCtx);
+            std::vector<std::string> origIdents;
+            {
+                AutoGetCollection autoColl(_opCtx, nss, LockMode::MODE_IS, LockMode::MODE_IS);
+                origIdents = kvCatalog->getAllIdents(_opCtx);
+            }
 
             auto indexSpec = BSON("createIndexes" << nss.coll() << "ns" << nss.ns() << "v"
                                                   << static_cast<int>(kIndexVersion)
@@ -2162,6 +2168,7 @@ public:
 
             ASSERT_OK(doAtomicApplyOps(nss.db().toString(), {createIndexOp}));
 
+            AutoGetCollection autoColl(_opCtx, nss, LockMode::MODE_IS, LockMode::MODE_IS);
             const std::string indexIdent = getNewIndexIdent(kvCatalog, origIdents);
             assertIdentsMissingAtTimestamp(
                 kvCatalog, "", indexIdent, beforeBuildTime.asTimestamp());