SERVER-48013 Use atomics to reduce contention in hasSeenKeys()

This commit simplifies the concurrency control in KeysCollectionManager and removes the
unnecessary invocation of shouldGossipLogicalTime() in appendRequiredFieldsToResponse().

4 files changed, 60 insertions, 40 deletions

diff --git a/src/mongo/db/keys_collection_manager.cpp b/src/mongo/db/keys_collection_manager.cpp
index 87f02b68e64..8b40033705f 100644
--- a/src/mongo/db/keys_collection_manager.cpp
+++ b/src/mongo/db/keys_collection_manager.cpp
@@ -223,6 +223,8 @@ void KeysCollectionManager::PeriodicRunner::_doPeriodicRefresh(ServiceContext* s
                                                                Milliseconds refreshInterval) {
     ThreadClient tc(threadName, service);
 
+    ON_BLOCK_EXIT([this]() mutable { _hasSeenKeys.store(false); });
+
     while (true) {
         unsigned errorCount = 0;
 
@@ -251,10 +253,7 @@ void KeysCollectionManager::PeriodicRunner::_doPeriodicRefresh(ServiceContext* s
                 const auto& latestKey = latestKeyStatusWith.getValue();
                 auto currentTime = LogicalClock::get(service)->getClusterTime();
 
-                {
-                    stdx::unique_lock<Latch> lock(_mutex);
-                    _hasSeenKeys = true;
-                }
+                _hasSeenKeys.store(true);
 
                 nextWakeup =
                     howMuchSleepNeedFor(currentTime, latestKey.getExpiresAt(), refreshInterval);
@@ -345,16 +344,14 @@ void KeysCollectionManager::PeriodicRunner::stop() {
         }
 
         _inShutdown = true;
-        _hasSeenKeys = false;
         _refreshNeededCV.notify_all();
     }
 
     _backgroundThread.join();
 }
 
-bool KeysCollectionManager::PeriodicRunner::hasSeenKeys() {
-    stdx::lock_guard<Latch> lock(_mutex);
-    return _hasSeenKeys;
+bool KeysCollectionManager::PeriodicRunner::hasSeenKeys() const noexcept {
+    return _hasSeenKeys.load();
 }
 
 }  // namespace mongo
diff --git a/src/mongo/db/keys_collection_manager.h b/src/mongo/db/keys_collection_manager.h
index 1131b7c3612..751de346ef0 100644
--- a/src/mongo/db/keys_collection_manager.h
+++ b/src/mongo/db/keys_collection_manager.h
@@ -37,6 +37,7 @@
 #include "mongo/db/keys_collection_cache.h"
 #include "mongo/db/keys_collection_document.h"
 #include "mongo/db/keys_collection_manager_gen.h"
+#include "mongo/platform/atomic_word.h"
 #include "mongo/platform/mutex.h"
 #include "mongo/stdx/thread.h"
 #include "mongo/util/concurrency/notification.h"
@@ -162,13 +163,15 @@ private:
         /**
          * Returns true if keys have ever successfully been returned from the config server.
          */
-        bool hasSeenKeys();
+        bool hasSeenKeys() const noexcept;
 
     private:
         void _doPeriodicRefresh(ServiceContext* service,
                                 std::string threadName,
                                 Milliseconds refreshInterval);
 
+        AtomicWord<bool> _hasSeenKeys{false};
+
         // protects all the member variables below.
         Mutex _mutex = MONGO_MAKE_LATCH("PeriodicRunner::_mutex");
         std::shared_ptr<Notification<void>> _refreshRequest;
@@ -177,7 +180,6 @@ private:
         stdx::thread _backgroundThread;
         std::shared_ptr<RefreshFunc> _doRefresh;
 
-        bool _hasSeenKeys = false;
         bool _inShutdown = false;
     };
 
diff --git a/src/mongo/s/commands/cluster_command_test_fixture.cpp b/src/mongo/s/commands/cluster_command_test_fixture.cpp
index f3fe69feda8..60575f966c9 100644
--- a/src/mongo/s/commands/cluster_command_test_fixture.cpp
+++ b/src/mongo/s/commands/cluster_command_test_fixture.cpp
@@ -106,6 +106,10 @@ void ClusterCommandTestFixture::expectReturnsError(ErrorCodes::Error code) {
 }
 
 DbResponse ClusterCommandTestFixture::runCommand(BSONObj cmd) {
+    // TODO SERVER-48142 should remove the following fail-point usage.
+    // Skip appending required fields in unit-tests
+    FailPointEnableBlock skipAppendingReqFields("allowSkippingAppendRequiredFieldsToResponse");
+
     // Create a new client/operation context per command
     auto client = getServiceContext()->makeClient("ClusterCmdClient");
     auto opCtx = client->makeOperationContext();
diff --git a/src/mongo/s/commands/strategy.cpp b/src/mongo/s/commands/strategy.cpp
index d53bec20d32..2334232a604 100644
--- a/src/mongo/s/commands/strategy.cpp
+++ b/src/mongo/s/commands/strategy.cpp
@@ -134,43 +134,60 @@ Status processCommandMetadata(OperationContext* opCtx, const BSONObj& cmdObj) {
 }
 
 /**
+ * Invoking `shouldGossipLogicalTime()` is expected to always return "true" during normal execution.
+ * SERVER-48013 uses this property to avoid the cost of calling this function during normal
+ * execution. However, it might be desired to do the validation for test purposes (e.g.,
+ * unit-tests). This fail-point allows going through a code path that does the check and quick
+ * returns from `appendRequiredFieldsToResponse()` if `shouldGossipLogicalTime()` returns "false".
+ * TODO SERVER-48142 should remove the following fail-point.
+ */
+MONGO_FAIL_POINT_DEFINE(allowSkippingAppendRequiredFieldsToResponse);
+
+/**
  * Append required fields to command response.
  */
 void appendRequiredFieldsToResponse(OperationContext* opCtx, BSONObjBuilder* responseBuilder) {
     VectorClock::get(opCtx)->gossipOut(responseBuilder, opCtx->getClient()->getSessionTags());
-    auto validator = LogicalTimeValidator::get(opCtx);
-    if (validator->shouldGossipLogicalTime()) {
-        auto now = LogicalClock::get(opCtx)->getClusterTime();
-
-        // Add operationTime.
-        auto operationTime = OperationTimeTracker::get(opCtx)->getMaxOperationTime();
-        if (operationTime != LogicalTime::kUninitialized) {
-            LOGV2_DEBUG(22764,
-                        5,
-                        "Appending operationTime: {operationTime}",
-                        "Appending operationTime",
-                        "operationTime"_attr = operationTime.asTimestamp());
-            responseBuilder->append(kOperationTime, operationTime.asTimestamp());
-        } else if (now != LogicalTime::kUninitialized) {
-            // If we don't know the actual operation time, use the cluster time instead. This is
-            // safe but not optimal because we can always return a later operation time than actual.
-            LOGV2_DEBUG(22765,
-                        5,
-                        "Appending clusterTime as operationTime {clusterTime}",
-                        "Appending clusterTime as operationTime",
-                        "clusterTime"_attr = now.asTimestamp());
-            responseBuilder->append(kOperationTime, now.asTimestamp());
-        }
 
-        // Add $clusterTime.
-        if (LogicalTimeValidator::isAuthorizedToAdvanceClock(opCtx)) {
-            SignedLogicalTime dummySignedTime(now, TimeProofService::TimeProof(), 0);
-            rpc::LogicalTimeMetadata(dummySignedTime).writeToMetadata(responseBuilder);
-        } else {
-            auto currentTime = validator->signLogicalTime(opCtx, now);
-            rpc::LogicalTimeMetadata(currentTime).writeToMetadata(responseBuilder);
+    // TODO SERVER-48142 should remove the following block.
+    if (MONGO_unlikely(allowSkippingAppendRequiredFieldsToResponse.shouldFail())) {
+        auto validator = LogicalTimeValidator::get(opCtx);
+        if (!validator->shouldGossipLogicalTime()) {
+            LOGV2_DEBUG(4801301, 3, "Skipped gossiping logical time");
+            return;
         }
     }
+
+    auto now = LogicalClock::get(opCtx)->getClusterTime();
+
+    // Add operationTime.
+    auto operationTime = OperationTimeTracker::get(opCtx)->getMaxOperationTime();
+    if (operationTime != LogicalTime::kUninitialized) {
+        LOGV2_DEBUG(22764,
+                    5,
+                    "Appending operationTime: {operationTime}",
+                    "Appending operationTime",
+                    "operationTime"_attr = operationTime.asTimestamp());
+        responseBuilder->append(kOperationTime, operationTime.asTimestamp());
+    } else if (now != LogicalTime::kUninitialized) {
+        // If we don't know the actual operation time, use the cluster time instead. This is
+        // safe but not optimal because we can always return a later operation time than actual.
+        LOGV2_DEBUG(22765,
+                    5,
+                    "Appending clusterTime as operationTime {clusterTime}",
+                    "Appending clusterTime as operationTime",
+                    "clusterTime"_attr = now.asTimestamp());
+        responseBuilder->append(kOperationTime, now.asTimestamp());
+    }
+
+    // Add $clusterTime.
+    if (LogicalTimeValidator::isAuthorizedToAdvanceClock(opCtx)) {
+        SignedLogicalTime dummySignedTime(now, TimeProofService::TimeProof(), 0);
+        rpc::LogicalTimeMetadata(dummySignedTime).writeToMetadata(responseBuilder);
+    } else {
+        auto currentTime = LogicalTimeValidator::get(opCtx)->signLogicalTime(opCtx, now);
+        rpc::LogicalTimeMetadata(currentTime).writeToMetadata(responseBuilder);
+    }
 }
 
 /**