SERVER-42737 Make secondary reads in ShardServerCatalogCacheLoader block behind the PBW lock

4 files changed, 97 insertions, 0 deletions

diff --git a/jstests/sharding/secondary_cache_reload_no_hang.js b/jstests/sharding/secondary_cache_reload_no_hang.js
new file mode 100644
index 00000000000..e6b54e04ba9
--- /dev/null
+++ b/jstests/sharding/secondary_cache_reload_no_hang.js
@@ -0,0 +1,54 @@
+/**
+ * TODO: SERVER-44105 maybe remove this test.
+ * This test is a simplified version that tries to simulate the condition described in
+ * SERVER-42737. It is very hard to replicate the exact condition because of:
+ *
+ * https://github.com/mongodb/mongo/blob/r4.3.0/src/mongo/db/s/shard_server_catalog_cache_loader.cpp#L293
+ *
+ * This means that secondary should be replicating a newer refresh after that line
+ * above and hit the condition described in the ticket to hit the bug.
+ */
+(function() {
+let rsOptions = {nodes: 2};
+let st = new ShardingTest({shards: {rs0: rsOptions, rs1: rsOptions}});
+
+assert.commandWorked(st.s.adminCommand({enableSharding: 'test'}));
+st.ensurePrimaryShard('test', st.shard0.shardName);
+assert.commandWorked(st.s.adminCommand({shardCollection: 'test.user', key: {x: 1}}));
+assert.commandWorked(st.s.adminCommand({split: 'test.user', middle: {x: 0}}));
+
+let coll = st.s.getDB('test').user;
+
+assert.commandWorked(coll.insert({x: -1}));
+assert.commandWorked(coll.insert({x: 1}));
+
+assert.commandWorked(
+    st.s.adminCommand({moveChunk: 'test.user', find: {x: 0}, to: st.shard1.shardName}));
+
+// Manually set refreshing flag to true so secondary cache refresh will block.
+
+st.rs0.getPrimary().getDB('config').cache.collections.update(
+    {_id: 'test.user', fake: {'$exists': false}}, {$set: {refreshing: true}});
+
+// Add a delay (sleep) to make sure that secondary will have the {refreshing: true}
+// in the lastApplied snapshot and is blocked waiting for refreshing to become false
+// before sending update to primary.
+
+let joinUpdate = startParallelShell(
+    'sleep(1000);' +
+        'db.getSiblingDB("config").cache.collections.update(' +
+        '{_id: "test.user", fake: {"$exists": false}},' +
+        '{$set: {refreshing: false, lastRefreshedCollectionVersion: Timestamp(5, 0)}});',
+    st.rs0.getPrimary().port);
+
+// This secondary read should not cause a hang.
+st.s.setReadPref('secondary');
+let res = assert.commandWorked(coll.getDB('test').runReadCommand(
+    {find: 'user', filter: {dummy: {'$exists': false}}, readConcern: {level: 'local'}}));
+
+assert.eq(2, res.cursor.firstBatch.length, tojson(res));
+
+joinUpdate();
+
+st.stop();
+})();
diff --git a/src/mongo/db/db_raii.cpp b/src/mongo/db/db_raii.cpp
index ede268a31f3..4e2fc2c8e45 100644
--- a/src/mongo/db/db_raii.cpp
+++ b/src/mongo/db/db_raii.cpp
@@ -46,6 +46,12 @@ namespace {
 
 const boost::optional<int> kDoNotChangeProfilingLevel = boost::none;
 
+// TODO: SERVER-44105 remove
+// If set to false, secondary reads should wait behind the PBW lock.
+// Does nothing if gAllowSecondaryReadsDuringBatchApplication setting is false.
+const auto allowSecondaryReadsDuringBatchApplication_DONT_USE =
+    OperationContext::declareDecoration<boost::optional<bool>>();
+
 }  // namespace
 
 AutoStatsTracker::AutoStatsTracker(OperationContext* opCtx,
@@ -92,6 +98,7 @@ AutoGetCollectionForRead::AutoGetCollectionForRead(OperationContext* opCtx,
     // Don't take the ParallelBatchWriterMode lock when the server parameter is set and our
     // storage engine supports snapshot reads.
     if (gAllowSecondaryReadsDuringBatchApplication.load() &&
+        allowSecondaryReadsDuringBatchApplication_DONT_USE(opCtx).value_or(true) &&
         opCtx->getServiceContext()->getStorageEngine()->supportsReadConcernSnapshot()) {
         _shouldNotConflictWithSecondaryBatchApplicationBlock.emplace(opCtx->lockState());
     }
@@ -388,4 +395,18 @@ LockMode getLockModeForQuery(OperationContext* opCtx, const boost::optional<Name
     return MODE_IS;
 }
 
+BlockSecondaryReadsDuringBatchApplication_DONT_USE::
+    BlockSecondaryReadsDuringBatchApplication_DONT_USE(OperationContext* opCtx)
+    : _opCtx(opCtx) {
+    auto allowSecondaryReads = &allowSecondaryReadsDuringBatchApplication_DONT_USE(opCtx);
+    allowSecondaryReads->swap(_originalSettings);
+    *allowSecondaryReads = false;
+}
+
+BlockSecondaryReadsDuringBatchApplication_DONT_USE::
+    ~BlockSecondaryReadsDuringBatchApplication_DONT_USE() {
+    auto allowSecondaryReads = &allowSecondaryReadsDuringBatchApplication_DONT_USE(_opCtx);
+    allowSecondaryReads->swap(_originalSettings);
+}
+
 }  // namespace mongo
diff --git a/src/mongo/db/db_raii.h b/src/mongo/db/db_raii.h
index f5497c6f3a1..940e13c1f6f 100644
--- a/src/mongo/db/db_raii.h
+++ b/src/mongo/db/db_raii.h
@@ -259,4 +259,19 @@ private:
     PrepareConflictBehavior _originalValue;
 };
 
+/**
+ * TODO: SERVER-44105 remove
+ * RAII type for letting secondary reads to block behind the PBW lock.
+ * Note: Do not add additional usage. This is only temporary for ease of backport.
+ */
+struct BlockSecondaryReadsDuringBatchApplication_DONT_USE {
+public:
+    BlockSecondaryReadsDuringBatchApplication_DONT_USE(OperationContext* opCtx);
+    ~BlockSecondaryReadsDuringBatchApplication_DONT_USE();
+
+private:
+    OperationContext* _opCtx{nullptr};
+    boost::optional<bool> _originalSettings;
+};
+
 }  // namespace mongo
diff --git a/src/mongo/db/s/shard_server_catalog_cache_loader.cpp b/src/mongo/db/s/shard_server_catalog_cache_loader.cpp
index ee2230b6333..37a59006a68 100644
--- a/src/mongo/db/s/shard_server_catalog_cache_loader.cpp
+++ b/src/mongo/db/s/shard_server_catalog_cache_loader.cpp
@@ -36,6 +36,7 @@
 #include <memory>
 
 #include "mongo/db/client.h"
+#include "mongo/db/db_raii.h"
 #include "mongo/db/operation_context.h"
 #include "mongo/db/operation_context_group.h"
 #include "mongo/db/read_concern.h"
@@ -619,6 +620,12 @@ void ShardServerCatalogCacheLoader::_runSecondaryGetChunksSince(
     forcePrimaryCollectionRefreshAndWaitForReplication(opCtx, nss);
 
     // Read the local metadata.
+
+    // Disallow reading on an older snapshot because this relies on being able to read the
+    // side effects of writes during secondary replication after being signalled from the
+    // CollectionVersionLogOpHandler.
+    BlockSecondaryReadsDuringBatchApplication_DONT_USE secondaryReadsBlockBehindReplication(opCtx);
+
     auto swCollAndChunks =
         _getCompletePersistedMetadataForSecondarySinceVersion(opCtx, nss, catalogCacheSinceVersion);
     callbackFn(opCtx, std::move(swCollAndChunks));