SERVER-44018 Fix MR agg output mode 'reduce' and move JsExecution to decorator on op context

16 files changed, 78 insertions, 57 deletions

diff --git a/buildscripts/resmokeconfig/suites/core_map_reduce_agg.yaml b/buildscripts/resmokeconfig/suites/core_map_reduce_agg.yaml
index 067dcfe5a96..aa403c4b48d 100644
--- a/buildscripts/resmokeconfig/suites/core_map_reduce_agg.yaml
+++ b/buildscripts/resmokeconfig/suites/core_map_reduce_agg.yaml
@@ -10,6 +10,8 @@ selector:
   - jstests/core/mr_index2.js
   - jstests/core/mr_merge2.js
   - jstests/core/mr_mutable_properties.js
+  - jstests/core/mr_outreduce.js
+  - jstests/core/mr_outreduce2.js
   - jstests/core/mr_sort.js
   - jstests/core/mr_tolerates_js_exception.js
   - jstests/core/mr_undef.js
diff --git a/jstests/core/mr_outreduce.js b/jstests/core/mr_outreduce.js
index 33b338e5a86..ff9e297a5d2 100644
--- a/jstests/core/mr_outreduce.js
+++ b/jstests/core/mr_outreduce.js
@@ -57,8 +57,7 @@ assert.eq(tos(expected), tos(out.convertToSingleObject("value")), "B");
 
 t.insert({_id: 5, a: [5, 6]});
 out.insert({_id: 20, value: "10"});  // this is a sentinal to make sure it wasn't killed
-assert.commandWorked(
-    t.mapReduce(m, r, {out: {reduce: outName, nonAtomic: true}, query: {_id: {$gt: 4}}}));
+assert.commandWorked(t.mapReduce(m, r, {out: {reduce: outName}, query: {_id: {$gt: 4}}}));
 
 expected["5"]++;
 expected["6"] = 1;
diff --git a/jstests/core/mr_outreduce2.js b/jstests/core/mr_outreduce2.js
index 6e5468f5220..a309ad91af8 100644
--- a/jstests/core/mr_outreduce2.js
+++ b/jstests/core/mr_outreduce2.js
@@ -33,7 +33,7 @@ assert.eq(2, db[out].findOne({_id: 1}).value, "A1");
 assert.eq(1, db[out].findOne({_id: 2}).value, "A2");
 
 t.insert({_id: 4, x: 2});
-res = t.mapReduce(m, r, {out: {reduce: out}, query: {_id: {$gt: 3}}, finalize: null});
+res = t.mapReduce(m, r, {out: {reduce: out}, query: {_id: {$gt: 3}}});
 
 assert.eq(2, db[out].findOne({_id: 1}).value, "B1");
 assert.eq(2, db[out].findOne({_id: 2}).value, "B2");
diff --git a/src/mongo/db/commands/map_reduce_javascript_code.h b/src/mongo/db/commands/map_reduce_javascript_code.h
index 2fcf2e27b6b..fbe4cbae203 100644
--- a/src/mongo/db/commands/map_reduce_javascript_code.h
+++ b/src/mongo/db/commands/map_reduce_javascript_code.h
@@ -44,7 +44,8 @@ class MapReduceJavascriptCode {
 public:
     static MapReduceJavascriptCode parseFromBSON(const BSONElement& element) {
         uassert(ErrorCodes::BadValue,
-                "'scope' must be of string or code type",
+                str::stream() << "'" << element.fieldNameStringData()
+                              << "' must be of string or code type",
                 element.type() == String || element.type() == Code);
         return MapReduceJavascriptCode(element._asCode());
     }
diff --git a/src/mongo/db/commands/mr_common.cpp b/src/mongo/db/commands/mr_common.cpp
index eab02894609..e1595bea415 100644
--- a/src/mongo/db/commands/mr_common.cpp
+++ b/src/mongo/db/commands/mr_common.cpp
@@ -150,9 +150,10 @@ auto translateOutReduce(boost::intrusive_ptr<ExpressionContext> expCtx,
                         NamespaceString targetNss,
                         std::string code) {
     // Because of communication for sharding, $merge must hold on to a serializable BSON object
-    // at the moment so we reparse here.
-    auto reduceObj = BSON("args" << BSON_ARRAY("$value"
-                                               << "$$new.value")
+    // at the moment so we reparse here. Note that the reduce function signature expects 2
+    // arguments, the first being the key and the second being the array of values to reduce.
+    auto reduceObj = BSON("args" << BSON_ARRAY("$_id" << BSON_ARRAY("$value"
+                                                                    << "$$new.value"))
                                  << "eval" << code);
 
     auto finalProjectSpec =
diff --git a/src/mongo/db/pipeline/SConscript b/src/mongo/db/pipeline/SConscript
index 3737415ccf9..42afe075abb 100644
--- a/src/mongo/db/pipeline/SConscript
+++ b/src/mongo/db/pipeline/SConscript
@@ -49,12 +49,14 @@ env.Library(
     target='expression_context',
     source=[
         'expression_context.cpp',
+        'javascript_execution.cpp',
         'variables.cpp',
     ],
     LIBDEPS=[
         'aggregation_request',
         '$BUILD_DIR/mongo/db/query/collation/collator_factory_interface',
         '$BUILD_DIR/mongo/db/service_context',
+        '$BUILD_DIR/mongo/scripting/scripting',
         '$BUILD_DIR/mongo/util/intrusive_counter',
     ]
 )
diff --git a/src/mongo/db/pipeline/accumulator_js_reduce.cpp b/src/mongo/db/pipeline/accumulator_js_reduce.cpp
index 37a5103e684..16bba9c0a90 100644
--- a/src/mongo/db/pipeline/accumulator_js_reduce.cpp
+++ b/src/mongo/db/pipeline/accumulator_js_reduce.cpp
@@ -100,7 +100,8 @@ Value AccumulatorInternalJsReduce::getValue(bool toBeMerged) {
         }
 
         auto expCtx = getExpressionContext();
-        auto [jsExec, newlyCreated] = expCtx->getJsExecWithScope();
+        auto jsExec = expCtx->getJsExecWithScope();
+
         ScriptingFunction func = jsExec->getScope()->createFunction(_funcSource.c_str());
 
         uassert(31247, "The reduce function failed to parse in the javascript engine", func);
diff --git a/src/mongo/db/pipeline/expression_context.h b/src/mongo/db/pipeline/expression_context.h
index 7913ed2ddc0..7e129f08d28 100644
--- a/src/mongo/db/pipeline/expression_context.h
+++ b/src/mongo/db/pipeline/expression_context.h
@@ -42,6 +42,7 @@
 #include "mongo/db/namespace_string.h"
 #include "mongo/db/operation_context.h"
 #include "mongo/db/pipeline/aggregation_request.h"
+#include "mongo/db/pipeline/javascript_execution.h"
 #include "mongo/db/pipeline/mongo_process_interface.h"
 #include "mongo/db/pipeline/runtime_constants_gen.h"
 #include "mongo/db/pipeline/variables.h"
@@ -228,7 +229,7 @@ public:
     auto getJsExecWithScope() const {
         RuntimeConstants runtimeConstants = getRuntimeConstants();
         const boost::optional<mongo::BSONObj>& scope = runtimeConstants.getJsScope();
-        return mongoProcessInterface->getJsExec(scope.get_value_or(BSONObj()));
+        return JsExecution::get(opCtx, scope.get_value_or(BSONObj()));
     }
 
     // The explain verbosity requested by the user, or boost::none if no explain was requested.
diff --git a/src/mongo/db/pipeline/expression_javascript.cpp b/src/mongo/db/pipeline/expression_javascript.cpp
index 7d390b3e91a..be814efa34c 100644
--- a/src/mongo/db/pipeline/expression_javascript.cpp
+++ b/src/mongo/db/pipeline/expression_javascript.cpp
@@ -117,10 +117,7 @@ Value ExpressionInternalJsEmit::evaluate(const Document& root, Variables* variab
     // If the scope does not exist and is created by the following call, then make sure to
     // re-bind emit() and the given function to the new scope.
     auto expCtx = getExpressionContext();
-    auto [jsExec, newlyCreated] = expCtx->getJsExecWithScope();
-    if (newlyCreated) {
-        jsExec->getScope()->loadStored(expCtx->opCtx, true);
-    }
+    auto jsExec = expCtx->getJsExecWithScope();
 
     // Although inefficient to "create" a new function every time we evaluate, this will usually end
     // up being a simple cache lookup. This is needed because the JS Scope may have been recreated
@@ -204,10 +201,7 @@ Value ExpressionInternalJs::evaluate(const Document& root, Variables* variables)
                           << " can't be run on this process. Javascript is disabled.",
             getGlobalScriptEngine());
 
-    auto [jsExec, newlyCreated] = expCtx->getJsExecWithScope();
-    if (newlyCreated) {
-        jsExec->getScope()->loadStored(expCtx->opCtx, true);
-    }
+    auto jsExec = expCtx->getJsExecWithScope();
 
     ScriptingFunction func = jsExec->getScope()->createFunction(_funcSource.c_str());
     uassert(31265, "The eval function did not evaluate", func);
diff --git a/src/mongo/db/pipeline/javascript_execution.cpp b/src/mongo/db/pipeline/javascript_execution.cpp
new file mode 100644
index 00000000000..ac8e474fb9c
--- /dev/null
+++ b/src/mongo/db/pipeline/javascript_execution.cpp
@@ -0,0 +1,49 @@
+/**
+ *    Copyright (C) 2019-present MongoDB, Inc.
+ *
+ *    This program is free software: you can redistribute it and/or modify
+ *    it under the terms of the Server Side Public License, version 1,
+ *    as published by MongoDB, Inc.
+ *
+ *    This program is distributed in the hope that it will be useful,
+ *    but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *    Server Side Public License for more details.
+ *
+ *    You should have received a copy of the Server Side Public License
+ *    along with this program. If not, see
+ *    <http://www.mongodb.com/licensing/server-side-public-license>.
+ *
+ *    As a special exception, the copyright holders give permission to link the
+ *    code of portions of this program with the OpenSSL library under certain
+ *    conditions as described in each individual source file and distribute
+ *    linked combinations including the program with the OpenSSL library. You
+ *    must comply with the Server Side Public License in all respects for
+ *    all of the code used other than as permitted herein. If you modify file(s)
+ *    with this exception, you may extend this exception to your version of the
+ *    file(s), but you are not obligated to do so. If you do not wish to do so,
+ *    delete this exception statement from your version. If you delete this
+ *    exception statement from all source files in the program, then also delete
+ *    it in the license file.
+ */
+
+#include "mongo/platform/basic.h"
+
+#include "mongo/db/pipeline/javascript_execution.h"
+
+namespace mongo {
+
+namespace {
+const auto getExec = OperationContext::declareDecoration<std::unique_ptr<JsExecution>>();
+}  // namespace
+
+JsExecution* JsExecution::get(OperationContext* opCtx, const BSONObj& scope) {
+    auto& exec = getExec(opCtx);
+    if (!exec) {
+        exec = std::make_unique<JsExecution>(scope);
+        exec->getScope()->loadStored(opCtx, true);
+    }
+    return exec.get();
+}
+
+}  // namespace mongo
diff --git a/src/mongo/db/pipeline/javascript_execution.h b/src/mongo/db/pipeline/javascript_execution.h
index 4f63586c069..b62a2d2df12 100644
--- a/src/mongo/db/pipeline/javascript_execution.h
+++ b/src/mongo/db/pipeline/javascript_execution.h
@@ -31,9 +31,13 @@
 
 #include "mongo/db/client.h"
 #include "mongo/db/exec/document_value/value.h"
+#include "mongo/db/operation_context.h"
 #include "mongo/scripting/engine.h"
 
 namespace mongo {
+
+class OperationContext;
+
 /*
  * This class provides a more sensible interface with JavaScript Scope objects. It helps with
  * boilerplate related to calling JS functions from C++ code, and extracting BSON objects from the
@@ -42,9 +46,15 @@ namespace mongo {
 class JsExecution {
 public:
     /**
+     * Create or get a pointer to a JsExecution instance, capable of invoking Javascript functions
+     * and reading the return value.
+     */
+    static JsExecution* get(OperationContext* opCtx, const BSONObj& scope);
+
+    /**
      * Construct with a thread-local scope and initialize with the given scope variables.
      */
-    JsExecution(const BSONObj& scopeVars)
+    explicit JsExecution(const BSONObj& scopeVars)
         : _scope(getGlobalScriptEngine()->newScopeForCurrentThread()) {
         _scopeVars = scopeVars.getOwned();
         _scope->init(&_scopeVars);
diff --git a/src/mongo/db/pipeline/mongo_process_interface.h b/src/mongo/db/pipeline/mongo_process_interface.h
index 92f49708c30..754c6e6f37b 100644
--- a/src/mongo/db/pipeline/mongo_process_interface.h
+++ b/src/mongo/db/pipeline/mongo_process_interface.h
@@ -406,16 +406,6 @@ public:
                                            boost::optional<std::vector<std::string>> fields,
                                            boost::optional<ChunkVersion> targetCollectionVersion,
                                            const NamespaceString& outputNs) const = 0;
-
-    /**
-     * Create or get a pointer to a JsExecution instance, capable of invoking Javascript functions
-     * and reading the return value.
-     *
-     * Returns a pointer to a JsExecution and a boolean to indicate whether the JS Scope was newly
-     * created.
-     */
-    virtual std::pair<JsExecution*, bool> getJsExec(const BSONObj& scope) = 0;
-    virtual void releaseJsExec() = 0;
 };
 
 }  // namespace mongo
diff --git a/src/mongo/db/pipeline/mongos_process_interface.h b/src/mongo/db/pipeline/mongos_process_interface.h
index fc6457fcc4b..a3ad00d64a2 100644
--- a/src/mongo/db/pipeline/mongos_process_interface.h
+++ b/src/mongo/db/pipeline/mongos_process_interface.h
@@ -211,13 +211,6 @@ public:
                                            boost::optional<ChunkVersion> targetCollectionVersion,
                                            const NamespaceString& outputNs) const override;
 
-    std::pair<JsExecution*, bool> getJsExec(const BSONObj&) override {
-        // Javascript engine is not support on mongos.
-        MONGO_UNREACHABLE;
-    }
-
-    void releaseJsExec() override {}
-
 protected:
     BSONObj _reportCurrentOpForClient(OperationContext* opCtx,
                                       Client* client,
diff --git a/src/mongo/db/pipeline/pipeline.cpp b/src/mongo/db/pipeline/pipeline.cpp
index 7e9fa39c66d..c20fc4e82cb 100644
--- a/src/mongo/db/pipeline/pipeline.cpp
+++ b/src/mongo/db/pipeline/pipeline.cpp
@@ -320,7 +320,6 @@ bool Pipeline::aggHasWriteStage(const BSONObj& cmd) {
 void Pipeline::detachFromOperationContext() {
     pCtx->opCtx = nullptr;
     pCtx->mongoProcessInterface->setOperationContext(nullptr);
-    pCtx->mongoProcessInterface->releaseJsExec();
 
     for (auto&& source : _sources) {
         source->detachFromOperationContext();
diff --git a/src/mongo/db/pipeline/process_interface_standalone.h b/src/mongo/db/pipeline/process_interface_standalone.h
index ef502e8d3f2..7200cabd785 100644
--- a/src/mongo/db/pipeline/process_interface_standalone.h
+++ b/src/mongo/db/pipeline/process_interface_standalone.h
@@ -154,22 +154,6 @@ public:
                                            boost::optional<ChunkVersion> targetCollectionVersion,
                                            const NamespaceString& outputNs) const override;
 
-    /**
-     * If we are creating a new JsExecution (and therefore a new thread-local scope), make sure
-     * we pass that information back to the caller.
-     */
-    std::pair<JsExecution*, bool> getJsExec(const BSONObj& scope) override {
-        if (!_jsExec) {
-            _jsExec = std::make_unique<JsExecution>(scope);
-            return {_jsExec.get(), true};
-        }
-        return {_jsExec.get(), false};
-    }
-
-    void releaseJsExec() override {
-        _jsExec.reset();
-    }
-
 protected:
     BSONObj _reportCurrentOpForClient(OperationContext* opCtx,
                                       Client* client,
diff --git a/src/mongo/db/pipeline/stub_mongo_process_interface.h b/src/mongo/db/pipeline/stub_mongo_process_interface.h
index 1644d545366..147be45fc46 100644
--- a/src/mongo/db/pipeline/stub_mongo_process_interface.h
+++ b/src/mongo/db/pipeline/stub_mongo_process_interface.h
@@ -241,10 +241,5 @@ public:
         }
         return {fieldPaths, targetCollectionVersion};
     }
-
-    std::pair<JsExecution*, bool> getJsExec(const BSONObj&) {
-        MONGO_UNREACHABLE;
-    }
-    void releaseJsExec() {}
 };
 }  // namespace mongo