SERVER-58403 Serialize concurrent accesses to OperationContext::_comment

10 files changed, 26 insertions, 5 deletions

diff --git a/src/mongo/db/commands/explain_cmd.cpp b/src/mongo/db/commands/explain_cmd.cpp
index 318029aef75..34f3965d454 100644
--- a/src/mongo/db/commands/explain_cmd.cpp
+++ b/src/mongo/db/commands/explain_cmd.cpp
@@ -165,6 +165,7 @@ std::unique_ptr<CommandInvocation> CmdExplain::parse(OperationContext* opCtx,
     // Extract 'comment' field from the 'explainedObj' only if there is no top-level comment.
     auto commentField = explainedObj["comment"];
     if (!opCtx->getComment() && commentField) {
+        stdx::lock_guard<Client> lk(*opCtx->getClient());
         opCtx->setComment(commentField.wrap());
     }
 
diff --git a/src/mongo/db/commands/getmore_cmd.cpp b/src/mongo/db/commands/getmore_cmd.cpp
index 2652cfbc7e9..901f62ef6d7 100644
--- a/src/mongo/db/commands/getmore_cmd.cpp
+++ b/src/mongo/db/commands/getmore_cmd.cpp
@@ -239,6 +239,7 @@ void setUpOperationContextStateForGetMore(OperationContext* opCtx,
     // that if the 'getMore' command itself has a 'comment' field, we give precedence to it.
     auto comment = cursor.getOriginatingCommandObj()["comment"];
     if (!opCtx->getComment() && comment) {
+        stdx::lock_guard<Client> lk(*opCtx->getClient());
         opCtx->setComment(comment.wrap());
     }
 }
diff --git a/src/mongo/db/curop_test.cpp b/src/mongo/db/curop_test.cpp
index 857ef674d52..332e7d9042e 100644
--- a/src/mongo/db/curop_test.cpp
+++ b/src/mongo/db/curop_test.cpp
@@ -232,7 +232,10 @@ TEST(CurOpTest, ShouldNotReportFailpointMsgIfNotSet) {
 
     // Test the reported state should _not_ contain 'failpointMsg'.
     BSONObjBuilder reportedStateWithoutFailpointMsg;
-    curop->reportState(opCtx.get(), &reportedStateWithoutFailpointMsg);
+    {
+        stdx::lock_guard<Client> lk(*opCtx->getClient());
+        curop->reportState(opCtx.get(), &reportedStateWithoutFailpointMsg);
+    }
     auto bsonObj = reportedStateWithoutFailpointMsg.done();
 
     // bsonObj should _not_ contain 'failpointMsg' if a fail point is not set.
diff --git a/src/mongo/db/operation_context.h b/src/mongo/db/operation_context.h
index bbb89ecb739..010ab14756c 100644
--- a/src/mongo/db/operation_context.h
+++ b/src/mongo/db/operation_context.h
@@ -518,10 +518,17 @@ public:
         _isStartingMultiDocumentTransaction = isStartingMultiDocumentTransaction;
     }
 
+    /**
+     * Sets '_comment'. The client lock must be acquired before calling this method.
+     */
     void setComment(const BSONObj& comment) {
         _comment = comment.getOwned();
     }
 
+    /**
+     * Gets '_comment'. The client lock must be acquired when calling from any thread that does
+     * not own the client associated with the operation.
+     */
     boost::optional<BSONElement> getComment() {
         // The '_comment' object, if present, will only ever have one field.
         return _comment ? boost::optional<BSONElement>(_comment->firstElement()) : boost::none;
diff --git a/src/mongo/db/operation_context_test.cpp b/src/mongo/db/operation_context_test.cpp
index e06c084101a..66fcda8632a 100644
--- a/src/mongo/db/operation_context_test.cpp
+++ b/src/mongo/db/operation_context_test.cpp
@@ -1151,6 +1151,8 @@ TEST(OperationContextTest, CurrentOpExcludesKilledOperations) {
             // of an `opCtx`. This is because `CurOp::reportCurrentOpForClient()` accepts an `opCtx`
             // as input and requires it to be present throughout its execution.
             stdx::thread thread([&]() mutable {
+                stdx::lock_guard<Client> lk(*opCtx->getClient());
+
                 auto threadClient = serviceCtx->makeClient("ThreadClient");
 
                 // Generate report in absence of any opCtx
diff --git a/src/mongo/db/s/forwardable_operation_metadata.cpp b/src/mongo/db/s/forwardable_operation_metadata.cpp
index 45371ee0b26..b49c73f9fc9 100644
--- a/src/mongo/db/s/forwardable_operation_metadata.cpp
+++ b/src/mongo/db/s/forwardable_operation_metadata.cpp
@@ -51,15 +51,17 @@ ForwardableOperationMetadata::ForwardableOperationMetadata(OperationContext* opC
 }
 
 void ForwardableOperationMetadata::setOn(OperationContext* opCtx) const {
+    Client* client = opCtx->getClient();
     if (const auto& comment = getComment()) {
+        stdx::lock_guard<Client> lk(*client);
         opCtx->setComment(comment.get());
     }
 
     if (const auto& optAuthMetadata = getImpersonatedUserMetadata()) {
         const auto& authMetadata = optAuthMetadata.get();
         if (!authMetadata.getUsers().empty() || !authMetadata.getRoles().empty()) {
-            AuthorizationSession::get(opCtx->getClient())
-                ->setImpersonatedUserData(authMetadata.getUsers(), authMetadata.getRoles());
+            AuthorizationSession::get(client)->setImpersonatedUserData(authMetadata.getUsers(),
+                                                                       authMetadata.getRoles());
         }
     }
 }
diff --git a/src/mongo/db/service_entry_point_common.cpp b/src/mongo/db/service_entry_point_common.cpp
index 0b7ec6d4ee3..28cca1ed035 100644
--- a/src/mongo/db/service_entry_point_common.cpp
+++ b/src/mongo/db/service_entry_point_common.cpp
@@ -1337,6 +1337,7 @@ void ExecCommandDatabase::_initiateCommand() {
         } else if (fieldName == CommandHelpers::kHelpFieldName) {
             helpField = element;
         } else if (fieldName == "comment") {
+            stdx::lock_guard<Client> lk(*client);
             opCtx->setComment(element.wrap());
         } else if (fieldName == query_request_helper::queryOptionMaxTimeMS) {
             uasserted(ErrorCodes::InvalidOptions,
diff --git a/src/mongo/s/commands/cluster_explain_cmd.cpp b/src/mongo/s/commands/cluster_explain_cmd.cpp
index 9ee65bdbc49..e7734ec43fb 100644
--- a/src/mongo/s/commands/cluster_explain_cmd.cpp
+++ b/src/mongo/s/commands/cluster_explain_cmd.cpp
@@ -180,6 +180,7 @@ std::unique_ptr<CommandInvocation> ClusterExplainCmd::parse(OperationContext* op
     // Extract 'comment' field from the 'explainedObj' only if there is no top-level comment.
     auto commentField = explainedObj["comment"];
     if (!opCtx->getComment() && commentField) {
+        stdx::lock_guard<Client> lk(*opCtx->getClient());
         opCtx->setComment(commentField.wrap());
     }
 
diff --git a/src/mongo/s/commands/strategy.cpp b/src/mongo/s/commands/strategy.cpp
index 5a80b17e933..db2d1c2ceda 100644
--- a/src/mongo/s/commands/strategy.cpp
+++ b/src/mongo/s/commands/strategy.cpp
@@ -500,7 +500,8 @@ void ParseAndRunCommand::_parseCommand() {
     _isHello.emplace(command->getName() == "hello"_sd || command->getName() == "isMaster"_sd);
 
     opCtx->setExhaust(OpMsg::isFlagSet(m, OpMsg::kExhaustSupported));
-    const auto session = opCtx->getClient()->session();
+    Client* client = opCtx->getClient();
+    const auto session = client->session();
     if (session) {
         if (!opCtx->isExhaust() || !_isHello.get()) {
             InExhaustHello::get(session.get())->setInExhaust(false, _commandName);
@@ -529,10 +530,10 @@ void ParseAndRunCommand::_parseCommand() {
 
     // If the command includes a 'comment' field, set it on the current OpCtx.
     if (auto commentField = request.body["comment"]) {
+        stdx::lock_guard<Client> lk(*client);
         opCtx->setComment(commentField.wrap());
     }
 
-    Client* client = opCtx->getClient();
     auto const apiParamsFromClient = initializeAPIParameters(request.body, command);
 
     {
diff --git a/src/mongo/s/query/cluster_find.cpp b/src/mongo/s/query/cluster_find.cpp
index 7ed9694090c..cca6cf1ee02 100644
--- a/src/mongo/s/query/cluster_find.cpp
+++ b/src/mongo/s/query/cluster_find.cpp
@@ -466,8 +466,10 @@ Status setUpOperationContextStateForGetMore(OperationContext* opCtx,
     // that if the 'getMore' command itself has a 'comment' field, we give precedence to it.
     auto comment = cursor->getOriginatingCommand()["comment"];
     if (!opCtx->getComment() && comment) {
+        stdx::lock_guard<Client> lk(*opCtx->getClient());
         opCtx->setComment(comment.wrap());
     }
+
     if (cursor->isTailableAndAwaitData()) {
         // For tailable + awaitData cursors, the request may have indicated a maximum amount of time
         // to wait for new data. If not, default it to 1 second.  We track the deadline instead via