From 7a711e0faac5dcdc1eafe779f81c481596b05d6f Mon Sep 17 00:00:00 2001 From: Jimmy Thomson Date: Fri, 18 May 2018 12:04:57 -0700 Subject: [PATCH] deps: fixing microtask behavior with multiple contexts Initially we were handling microtasks per-context since our promise interface relies on per-library callbacks, but the v8 API actually deals with microtasks on a per-isolate level. This meant that we weren't properly firing microtasks / promise resolutions from contexts other than the currently active one. Now we also handle the microtask queue at the isolate level. PR-URL: https://github.com/nodejs/node-chakracore/pull/539 Reviewed-By: Kyle Farnung Reviewed-By: Hitesh Kanwathirtha --- deps/chakrashim/lib/chakra_shim.js | 11 ------- .../src/jsrtcachedpropertyidref.inc | 2 -- deps/chakrashim/src/jsrtcontextshim.cc | 21 ------------ deps/chakrashim/src/jsrtcontextshim.h | 3 -- deps/chakrashim/src/jsrtisolateshim.cc | 23 ++++++++++++- deps/chakrashim/src/jsrtisolateshim.h | 25 +++++++++++++++ deps/chakrashim/src/jsrtpromise.cc | 6 +--- deps/chakrashim/src/v8isolate.cc | 2 +- test/parallel/parallel.status | 4 +++ test/parallel/test-promise-context.js | 32 +++++++++++++++++++ 10 files changed, 85 insertions(+), 44 deletions(-) create mode 100755 test/parallel/test-promise-context.js diff --git a/deps/chakrashim/lib/chakra_shim.js b/deps/chakrashim/lib/chakra_shim.js index a7528028d2e..fddf9be901a 100644 --- a/deps/chakrashim/lib/chakra_shim.js +++ b/deps/chakrashim/lib/chakra_shim.js @@ -409,9 +409,6 @@ }; } - // Simulate v8 micro tasks queue - const microTasks = []; - function patchUtils(utils) { const isUintRegex = /^(0|[1-9]\d*)$/; @@ -629,14 +626,6 @@ utils.ensureDebug = ensureDebug; - utils.enqueueMicrotask = function(task) { - microTasks.push(task); - }; - - utils.dequeueMicrotask = function(task) { - return microTasks.shift(); - }; - utils.getPropertyAttributes = function(object, value) { const descriptor = Object_getOwnPropertyDescriptor(object, value); if (descriptor === undefined) { diff --git a/deps/chakrashim/src/jsrtcachedpropertyidref.inc b/deps/chakrashim/src/jsrtcachedpropertyidref.inc index a021fc3e0a0..ce8f73c6063 100644 --- a/deps/chakrashim/src/jsrtcachedpropertyidref.inc +++ b/deps/chakrashim/src/jsrtcachedpropertyidref.inc @@ -70,8 +70,6 @@ DEF(getStackTrace) DEF(getSymbolKeyFor) DEF(getSymbolFor) DEF(ensureDebug) -DEF(enqueueMicrotask) -DEF(dequeueMicrotask) DEF(saveInHandleScope) DEF(getPropertyAttributes) DEF(getOwnPropertyNames) diff --git a/deps/chakrashim/src/jsrtcontextshim.cc b/deps/chakrashim/src/jsrtcontextshim.cc index 69feed079ab..6559dc18ef8 100644 --- a/deps/chakrashim/src/jsrtcontextshim.cc +++ b/deps/chakrashim/src/jsrtcontextshim.cc @@ -95,8 +95,6 @@ ContextShim::ContextShim(IsolateShim * isolateShim, getSymbolKeyForFunction(JS_INVALID_REFERENCE), getSymbolForFunction(JS_INVALID_REFERENCE), ensureDebugFunction(JS_INVALID_REFERENCE), - enqueueMicrotaskFunction(JS_INVALID_REFERENCE), - dequeueMicrotaskFunction(JS_INVALID_REFERENCE), getPropertyAttributesFunction(JS_INVALID_REFERENCE), getOwnPropertyNamesFunction(JS_INVALID_REFERENCE), jsonParseFunction(JS_INVALID_REFERENCE), @@ -484,23 +482,6 @@ void ContextShim::SetAlignedPointerInEmbedderData(int index, void * value) { } } -void ContextShim::RunMicrotasks() { - JsValueRef dequeueMicrotaskFunction = GetdequeueMicrotaskFunction(); - - for (;;) { - JsValueRef task; - if (jsrt::CallFunction(dequeueMicrotaskFunction, &task) != JsNoError || - reinterpret_cast(task)->IsUndefined()) { - break; - } - - JsValueRef notUsed; - if (jsrt::CallFunction(task, ¬Used) != JsNoError) { - JsGetAndClearException(¬Used); // swallow any exception from task - } - } -} - // check initialization state first instead of calling // InitializeCurrentContextShim to save a function call for cases where // contextshim is already initialized @@ -615,8 +596,6 @@ CHAKRASHIM_FUNCTION_GETTER(getStackTrace) CHAKRASHIM_FUNCTION_GETTER(getSymbolKeyFor) CHAKRASHIM_FUNCTION_GETTER(getSymbolFor) CHAKRASHIM_FUNCTION_GETTER(ensureDebug) -CHAKRASHIM_FUNCTION_GETTER(enqueueMicrotask); -CHAKRASHIM_FUNCTION_GETTER(dequeueMicrotask); CHAKRASHIM_FUNCTION_GETTER(getPropertyAttributes); CHAKRASHIM_FUNCTION_GETTER(getOwnPropertyNames); CHAKRASHIM_FUNCTION_GETTER(jsonParse); diff --git a/deps/chakrashim/src/jsrtcontextshim.h b/deps/chakrashim/src/jsrtcontextshim.h index d79093943cf..173c82a4932 100644 --- a/deps/chakrashim/src/jsrtcontextshim.h +++ b/deps/chakrashim/src/jsrtcontextshim.h @@ -97,7 +97,6 @@ class ContextShim { void * GetAlignedPointerFromEmbedderData(int index); void SetAlignedPointerInEmbedderData(int index, void * value); - void RunMicrotasks(); static ContextShim * GetCurrent(); @@ -162,8 +161,6 @@ class ContextShim { DECLARE_CHAKRASHIM_FUNCTION_GETTER(getSymbolKeyFor); DECLARE_CHAKRASHIM_FUNCTION_GETTER(getSymbolFor); DECLARE_CHAKRASHIM_FUNCTION_GETTER(ensureDebug); - DECLARE_CHAKRASHIM_FUNCTION_GETTER(enqueueMicrotask); - DECLARE_CHAKRASHIM_FUNCTION_GETTER(dequeueMicrotask); DECLARE_CHAKRASHIM_FUNCTION_GETTER(getPropertyAttributes); DECLARE_CHAKRASHIM_FUNCTION_GETTER(getOwnPropertyNames); DECLARE_CHAKRASHIM_FUNCTION_GETTER(jsonParse); diff --git a/deps/chakrashim/src/jsrtisolateshim.cc b/deps/chakrashim/src/jsrtisolateshim.cc index 5b71016538b..13ceaabfb1d 100644 --- a/deps/chakrashim/src/jsrtisolateshim.cc +++ b/deps/chakrashim/src/jsrtisolateshim.cc @@ -229,7 +229,8 @@ IsolateShim::IsolateShim(JsRuntimeHandle runtime) isDisposing(false), contextScopeStack(nullptr), tryCatchStackTop(nullptr), - embeddedData() { + embeddedData(), + microtaskQueue() { // CHAKRA-TODO: multithread locking for s_isolateList? this->prevnext = &s_isolateList; this->next = s_isolateList; @@ -664,6 +665,26 @@ void IsolateShim::SetPromiseRejectCallback(v8::PromiseRejectCallback callback) { reinterpret_cast(callback)); } +void IsolateShim::RunMicrotasks() { + // Loop until we've handled all the tasks, including the ones + // added by these tasks + while (!this->microtaskQueue.empty()) { + std::vector batch; + std::swap(batch, this->microtaskQueue); + + for (const MicroTask& mt : batch) { + JsValueRef notUsed = JS_INVALID_REFERENCE; + if (jsrt::CallFunction(mt.task, ¬Used) != JsNoError) { + JsGetAndClearException(¬Used); // swallow any exception from task + } + } + } +} + +void IsolateShim::QueueMicrotask(JsValueRef task) { + this->microtaskQueue.emplace_back(task); +} + /*static*/ bool IsolateShim::RunSingleStepOfReverseMoveLoop(v8::Isolate* isolate, uint64_t* moveMode, diff --git a/deps/chakrashim/src/jsrtisolateshim.h b/deps/chakrashim/src/jsrtisolateshim.h index 46110f209f0..56e76150d59 100644 --- a/deps/chakrashim/src/jsrtisolateshim.h +++ b/deps/chakrashim/src/jsrtisolateshim.h @@ -122,6 +122,9 @@ class IsolateShim { } } + void RunMicrotasks(); + void QueueMicrotask(JsValueRef task); + JsValueRef GetChakraShimJsArrayBuffer(); JsValueRef GetChakraInspectorShimJsArrayBuffer(); @@ -164,6 +167,27 @@ class IsolateShim { void SetPromiseRejectCallback(v8::PromiseRejectCallback callback); private: + struct MicroTask { + explicit MicroTask(JsValueRef task) : task(task) { + JsAddRef(this->task, nullptr); + } + + ~MicroTask() { + if (this->task) { + JsRelease(this->task, nullptr); + } + } + + MicroTask(MicroTask&& other) + : task(other.task) { + other.task = nullptr; + } + + MicroTask(const MicroTask&) = delete; + + JsValueRef task; + }; + // Construction/Destruction should go thru New/Dispose explicit IsolateShim(JsRuntimeHandle runtime); ~IsolateShim(); @@ -200,6 +224,7 @@ class IsolateShim { uv_timer_t idleGc_timer_handle_; bool jsScriptExecuted = false; bool isIdleGcScheduled = false; + std::vector microtaskQueue; }; } // namespace jsrt diff --git a/deps/chakrashim/src/jsrtpromise.cc b/deps/chakrashim/src/jsrtpromise.cc index 7b65b381109..5dbb69c0e03 100644 --- a/deps/chakrashim/src/jsrtpromise.cc +++ b/deps/chakrashim/src/jsrtpromise.cc @@ -25,11 +25,7 @@ namespace jsrt { static void CHAKRA_CALLBACK PromiseContinuationCallback(JsValueRef task, void *callbackState) { - JsValueRef enqueueMicrotaskFunction = - ContextShim::GetCurrent()->GetenqueueMicrotaskFunction(); - - JsValueRef result; - jsrt::CallFunction(enqueueMicrotaskFunction, task, &result); + jsrt::IsolateShim::GetCurrent()->QueueMicrotask(task); } JsErrorCode InitializePromise() { diff --git a/deps/chakrashim/src/v8isolate.cc b/deps/chakrashim/src/v8isolate.cc index 039f864100e..61e7e04707e 100644 --- a/deps/chakrashim/src/v8isolate.cc +++ b/deps/chakrashim/src/v8isolate.cc @@ -153,7 +153,7 @@ void Isolate::EnqueueMicrotask(MicrotaskCallback microtask, void* data) { } void Isolate::RunMicrotasks() { - jsrt::ContextShim::GetCurrent()->RunMicrotasks(); + jsrt::IsolateShim::FromIsolate(this)->RunMicrotasks(); } void Isolate::SetMicrotasksPolicy(MicrotasksPolicy policy) { diff --git a/test/parallel/parallel.status b/test/parallel/parallel.status index f60b23f3cd5..99adf6e9a8c 100644 --- a/test/parallel/parallel.status +++ b/test/parallel/parallel.status @@ -184,6 +184,10 @@ test-v8-stats : SKIP # This test uses the V8 untrusted code mitigation and accompanying V8 option test-v8-untrusted-code-mitigations : SKIP +# This test should be enabled once chakracore fixes +# https://github.com/Microsoft/ChakraCore/issues/5196 +test-repl-top-level-await : SKIP + # This test seems to fail under system stress, marking it flaky for now # https://github.com/nodejs/node-chakracore/issues/494 test-async-hooks-destroy-on-gc : PASS,FLAKY diff --git a/test/parallel/test-promise-context.js b/test/parallel/test-promise-context.js new file mode 100755 index 00000000000..2205c610f1e --- /dev/null +++ b/test/parallel/test-promise-context.js @@ -0,0 +1,32 @@ +'use strict'; + +require('../common'); +const assert = require('assert'); +const vm = require('vm'); + +const responses = []; +function saveResult(x) { + responses.push(x); +} + +const p1 = new Promise((resolve, reject) => resolve(1)); +p1.then(saveResult).then(() => { saveResult(6); }); + +const p2 = new Promise((resolve, reject) => reject(2)); +p2.catch(saveResult); + +const p5 = vm.runInNewContext(` +const p3 = new Promise((resolve, reject) => resolve(3)).then(saveResult); +p3.then(() => { saveResult(7); }); +const p4 = new Promise((resolve, reject) => reject(4)).catch(saveResult); +new Promise((resolve, reject) => resolve(5)); +`, { saveResult }); +p5.then(saveResult); + +// Note: using setImmediate here since that is a macrotask and will happen +// after the microtask/promise queue is completely drained. +setImmediate(() => { + console.log(responses); + assert.strictEqual(responses.length, 7); + assert.deepStrictEqual(responses, [1, 2, 3, 4, 5, 6, 7]); +});