deps: fixing microtask behavior with multiple contexts #539

MSLaguana · 2018-05-18T19:19:08Z

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.

Partially addresses #420 in that promises should now work in vm.runInContext

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

MSLaguana · 2018-05-18T19:19:54Z

I'll add some tests for this in a bit, running the existing tests locally to make sure I didn't break anything obvious there.

MSLaguana · 2018-05-18T21:04:53Z

CI: https://ci.nodejs.org/job/chakracore-test-pull-request/259/

Turns out that one of node's upstream tests had been hit by this bug causing it not to run, and now that it does run it turns out it fails due to an error in upstream chakracore.

digitalinfinity · 2018-05-18T21:13:00Z

deps/chakrashim/src/jsrtisolateshim.cc

+
+void IsolateShim::QueueMicrotask(JsValueRef task) {
+  JsAddRef(task, nullptr);
+  this->microtaskQueue = new MicroTask(task, this->microtaskQueue);


What if this fails? Do we leak the task?

By "this" do you mean the new allocation? If that fails, then things are probably in a bad way (this is only allocating 2 pointers worth of data) but I hadn't given much thought to that scenario. The previous behavior was that we would push the task into an array in javascript, which I guess would OOM in the worst case and terminate the process, while here if the allocation fails then we leak any prior tasks, and also never run them.

digitalinfinity · 2018-05-18T21:14:43Z

deps/chakrashim/src/jsrtisolateshim.cc

+    this->microtaskQueue = nullptr;
+
+    // Ensure that we handle things in-order
+    MicroTask* ordered = nullptr;


I don't follow- what is the intent here?

Microtasks are supposed to run in the order that they are queued. For simplicity I've just implemented a stack, but to operate in FIFO order I just reverse the stack here.

Can we use a tail pointer and append to the end (i.e implement an actual queue)

I'd also just settle for a less terse comment

Updated the comment

kfarnung · 2018-05-18T22:27:23Z

deps/chakrashim/src/jsrtisolateshim.cc

+      t = next;
+    }
+
+    while (ordered != nullptr) {;


rogue semicolon?

kfarnung · 2018-05-18T22:58:39Z

deps/chakrashim/src/jsrtisolateshim.h

@@ -164,6 +167,12 @@ class IsolateShim {
  void SetPromiseRejectCallback(v8::PromiseRejectCallback callback);

 private:
+  struct MicroTask {
+    MicroTask(JsValueRef task, MicroTask* next) : task(task), next(next) {}


Could we have this manage the lifetime of the JsValueRef?

kfarnung · 2018-05-18T22:59:56Z

deps/chakrashim/src/jsrtisolateshim.cc

+  // Loop until we've handled all the tasks, including the ones
+  // added by these tasks
+  while (this->microtaskQueue != nullptr) {
+    MicroTask* batch = this->microtaskQueue;


I'd recommend using std::vector and then using std::swap to get the batch:

std::vector<MicroTask> batch; std::swap(this->microtaskQueue, batch);

MSLaguana · 2018-05-21T16:25:59Z

new CI: https://ci.nodejs.org/job/chakracore-test-pull-request/260/

MSLaguana · 2018-05-21T16:58:01Z

deps/chakrashim/src/jsrtisolateshim.cc

+}
+
+void IsolateShim::QueueMicrotask(JsValueRef task) {
+  this->microtaskQueue.emplace_back(task);


@kfarnung Would you mind double checking this is correct? I'm intending that this constructs the MicroTask in place in the vector

Looks correct to me.

kfarnung · 2018-05-21T18:10:28Z

deps/chakrashim/src/jsrtisolateshim.cc

+    std::vector<MicroTask> batch;
+    std::swap(batch, this->microtaskQueue);
+
+    for (unsigned int i = 0; i < batch.size(); ++i) {


You might be able to do:

for (const MicroTask& microTask : batch) {

which provides a const reference to the item in the vector (and thus doesn't copy it)

kfarnung · 2018-05-21T18:10:49Z

deps/chakrashim/src/jsrtisolateshim.cc

+    std::swap(batch, this->microtaskQueue);
+
+    for (unsigned int i = 0; i < batch.size(); ++i) {
+      JsValueRef notUsed;


nit: Probably still worth initializing to JS_INVALID_REFERENCE

kfarnung · 2018-05-21T18:11:46Z

deps/chakrashim/src/jsrtisolateshim.h

@@ -164,6 +167,24 @@ class IsolateShim {
  void SetPromiseRejectCallback(v8::PromiseRejectCallback callback);

 private:
+  struct MicroTask {
+    MicroTask(JsValueRef task) : task(task) {
+      JsAddRef(task, nullptr);


Shouldn't matter, but maybe use this->task to disambiguate?

kfarnung

LGTM

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: nodejs#539 Reviewed-By: Kyle Farnung <kfarnung@microsoft.com> Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>

MSLaguana · 2018-05-21T18:16:19Z

Third time's a charm: https://ci.nodejs.org/job/chakracore-test-pull-request/261/

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: nodejs#539 Reviewed-By: Kyle Farnung <kfarnung@microsoft.com> Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>

MSLaguana requested a review from kfarnung May 18, 2018 19:19

digitalinfinity approved these changes May 18, 2018

View reviewed changes

kfarnung reviewed May 18, 2018

View reviewed changes

MSLaguana force-pushed the fixContextMicrotasks branch from 9f3390d to d2e0fcb Compare May 21, 2018 16:06

MSLaguana force-pushed the fixContextMicrotasks branch from 9e8c0cb to f08c5fd Compare May 21, 2018 16:26

MSLaguana commented May 21, 2018

View reviewed changes

kfarnung reviewed May 21, 2018

View reviewed changes

kfarnung approved these changes May 21, 2018

View reviewed changes

MSLaguana force-pushed the fixContextMicrotasks branch from e584a17 to e32cb2f Compare May 21, 2018 18:14

MSLaguana force-pushed the fixContextMicrotasks branch from e32cb2f to 4926aa5 Compare May 21, 2018 20:06

MSLaguana force-pushed the fixContextMicrotasks branch from 4926aa5 to 576bc86 Compare May 21, 2018 20:12

MSLaguana merged commit 576bc86 into nodejs:master May 21, 2018

MSLaguana deleted the fixContextMicrotasks branch May 21, 2018 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: fixing microtask behavior with multiple contexts #539

deps: fixing microtask behavior with multiple contexts #539

MSLaguana commented May 18, 2018 •

edited

Loading

MSLaguana commented May 18, 2018

MSLaguana commented May 18, 2018

digitalinfinity May 18, 2018

MSLaguana May 18, 2018

digitalinfinity May 18, 2018

MSLaguana May 18, 2018

digitalinfinity May 18, 2018

MSLaguana May 18, 2018

kfarnung May 18, 2018 •

edited

Loading

kfarnung May 18, 2018

kfarnung May 18, 2018

MSLaguana commented May 21, 2018

MSLaguana May 21, 2018

kfarnung May 21, 2018

kfarnung May 21, 2018

kfarnung May 21, 2018

kfarnung May 21, 2018

kfarnung left a comment

MSLaguana commented May 21, 2018

deps: fixing microtask behavior with multiple contexts #539

deps: fixing microtask behavior with multiple contexts #539

Conversation

MSLaguana commented May 18, 2018 • edited Loading

Checklist

MSLaguana commented May 18, 2018

MSLaguana commented May 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfarnung May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSLaguana commented May 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfarnung left a comment

Choose a reason for hiding this comment

MSLaguana commented May 21, 2018

MSLaguana commented May 18, 2018 •

edited

Loading

kfarnung May 18, 2018 •

edited

Loading