Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async_hooks: proper id stacking for Promises #13585

Merged
merged 1 commit into from
Jun 16, 2017

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jun 9, 2017

Fixes: #13583

This won’t fix everything, but I think this is something that just qualifies as a right thing to do.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

async_hooks

/cc @nodejs/async_hooks

@addaleax addaleax added async_hooks Issues and PRs related to the async hooks subsystem. promises Issues and PRs related to ECMAScript promises. labels Jun 9, 2017
@nodejs-github-bot nodejs-github-bot added async_wrap c++ Issues and PRs that require attention from people who are familiar with C++. labels Jun 9, 2017
async_hooks.createHook({
init: common.mustCallAtLeast((id, type, triggerId) => {
if (type === 'PROMISE') {
assert.strictEqual(promiseAsyncIds[promiseAsyncIds.length - 1] || 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why test that the triggerId propagates correctly? Maybe you were thinking of testing async_hooks.triggerId()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, this is just because it seemed easy to add an assertion for. Do you think I should drop it / test other things as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the assert hard to read, so either add a comment or remove it. You should definitely check that async_hooks.triggerId() is correct, since that is the second parameter in push_ids.

Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could have bad side-effects in combination with #13509. Consider:

Promise.resolve(1).then(function () {
  // something that implicitly requires `async_hooks` and enables PromiseHooks
  process.nextTick(function () {

  });

  // At this point, PromiseHooks will pop the id stack, but nothing will have been
  // pushed because PromiseHooks was enabled late.
});

@addaleax
Copy link
Member Author

addaleax commented Jun 9, 2017

@AndreasMadsen Good point, I’ll take care of that. Give me a minute. :)

@addaleax addaleax force-pushed the async-hooks-promise-pushpop branch from 553773b to 931ab11 Compare June 9, 2017 23:40
@addaleax
Copy link
Member Author

addaleax commented Jun 9, 2017

@AndreasMadsen You are right. I’ve rebased this on #13509 (which in turn is rebased on master) … I had to do some clever¹ stuff to get things working, but I think this makes sense and is consistent.

¹ edit to clarify: the bad kind of clever. ;)

@addaleax addaleax force-pushed the async-hooks-promise-pushpop branch from 931ab11 to f0344ef Compare June 9, 2017 23:48
Copy link
Contributor

@trevnorris trevnorris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. If you wouldn't mind adding more detail to the git commit message about what it is you're doing. And you may need to fix the timing on some of the tests after #13509 lands.

@addaleax addaleax force-pushed the async-hooks-promise-pushpop branch 2 times, most recently from 324731d to 9718b5f Compare June 10, 2017 09:57
@addaleax
Copy link
Member Author

@trevnorris Okay, updated. The commit message is a bit more descriptive now, and the tests should pass now that #13509 has landed.

CI: https://ci.nodejs.org/job/node-test-commit/10486/

// Check that internal fields are no longer being set. This needs to be delayed
// a bit because the `disable()` call only schedules disabling the hook in a
// future microtask.
setImmediate(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, interesting. Is this something we should fix? With for example

if (env->async_hooks()->fields()[AsyncHooks::kTotals])
  return;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can’t because we need the pop_ids() call to happen, so we need the PromiseHook working until at least the kAfter occurs.

This shouldn’t be a problem, it’s just emitting events for nobody to witness.

}


static void DisablePromiseHook(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
env->RemovePromiseHook(PromiseHook, nullptr);

// Delay the call to `RemovePromiseHook` because we might currently be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are going with the temp fix, I guess this is okay. But I really want to see a better solution. I'm not really convinced there aren't some obscure bug with the right .enable() and .disable() calls.

Environment* env = Environment::GetCurrent(context);

// PromiseHook() should never be called if no hooks have been enabled.
CHECK_GT(env->async_hooks()->fields()[AsyncHooks::kTotals], 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't loose this check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevnorris I know why you put it there, and I am not a fan of removing it, but do you have a better suggestion?

It’s good the cctest is there, it basically checks the same thing, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yup. See now why it must to be removed. Nm.

@trevnorris
Copy link
Contributor

@addaleax Thanks for working on this. I'm running a couple tests now and will get back with you as soon as I'm done.

@trevnorris
Copy link
Contributor

trevnorris commented Jun 12, 2017

@addaleax Your patch gave me an idea. That disable() doesn't need to trigger removing the PromiseHook. Instead we identify where in the stack we should check if PromiseHook should be removed. Here's a patch on top of yours that shows what I mean, and passes all tests:

AsyncWrap remove PromiseHook callback
diff --git a/lib/async_hooks.js b/lib/async_hooks.js
index 53ce038..7deb5ed 100644
--- a/lib/async_hooks.js
+++ b/lib/async_hooks.js
@@ -139,8 +139,8 @@ class AsyncHook {
     hook_fields[kTotals] += hook_fields[kDestroy] -= +!!this[destroy_symbol];
     hooks_array.splice(index, 1);
 
-    if (prev_kTotals > 0 && hook_fields[kTotals] === 0)
-      async_wrap.disablePromiseHook();
+    // Disabling of PromiseHook() will happen automatically at the end of
+    // synchronous execution.
 
     return this;
   }
diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index 327fe36..41eba0f 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -437,15 +437,10 @@ static void EnablePromiseHook(const FunctionCallbackInfo<Value>& args) {
 }
 
 
-static void DisablePromiseHook(const FunctionCallbackInfo<Value>& args) {
-  Environment* env = Environment::GetCurrent(args);
-
-  // Delay the call to `RemovePromiseHook` because we might currently be
-  // between the `before` and `after` calls of a Promise.
-  env->isolate()->EnqueueMicrotask([](void* data) {
-    Environment* env = static_cast<Environment*>(data);
-    env->RemovePromiseHook(PromiseHook, data);
-  }, static_cast<void*>(env));
+void AsyncWrap::CheckAndRemovePromiseHook(Environment* env) {
+  if (env->async_hooks()->fields()[AsyncHooks::kTotals] == 0) {
+    env->RemovePromiseHook(PromiseHook, static_cast<void*>(env));
+  }
 }
 
 
@@ -506,7 +501,6 @@ void AsyncWrap::Initialize(Local<Object> target,
   env->SetMethod(target, "clearIdStack", ClearIdStack);
   env->SetMethod(target, "addIdToDestroyList", QueueDestroyId);
   env->SetMethod(target, "enablePromiseHook", EnablePromiseHook);
-  env->SetMethod(target, "disablePromiseHook", DisablePromiseHook);
 
   v8::PropertyAttribute ReadOnlyDontDelete =
       static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete);
@@ -691,6 +685,8 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
   // Finally... Get to running the user's callback.
   MaybeLocal<Value> ret = cb->Call(env()->context(), object(), argc, argv);
 
+  CheckAndRemovePromiseHook(env());
+
   Local<Value> ret_v;
   if (!ret.ToLocal(&ret_v)) {
     return Local<Value>();
@@ -717,6 +713,8 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
   CHECK_EQ(env()->current_async_id(), 0);
   CHECK_EQ(env()->trigger_id(), 0);
 
+  CheckAndRemovePromiseHook(env());
+
   Local<Object> process = env()->process_object();
 
   if (tick_info->length() == 0) {
diff --git a/src/async-wrap.h b/src/async-wrap.h
index fa37ea1..d657343 100644
--- a/src/async-wrap.h
+++ b/src/async-wrap.h
@@ -111,6 +111,8 @@ class AsyncWrap : public BaseObject {
   static bool EmitBefore(Environment* env, double id);
   static bool EmitAfter(Environment* env, double id);
 
+  static void CheckAndRemovePromiseHook(Environment* env);
+
   inline ProviderType provider_type() const;
 
   inline double get_id() const;
diff --git a/src/env.cc b/src/env.cc
index 0087f71..1f4ed15 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -204,6 +204,7 @@ bool Environment::RemovePromiseHook(promise_hook_func fn, void* arg) {
 
   if (it == promise_hooks_.end()) return false;
 
+  CHECK_GT(it->enable_count_, 0);
   if (--it->enable_count_ > 0) return true;
 
   promise_hooks_.erase(it);
diff --git a/src/node.cc b/src/node.cc
index bbce102..090bcab 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1326,6 +1326,8 @@ MaybeLocal<Value> MakeCallback(Environment* env,
 
     ret = callback->Call(env->context(), recv, argc, argv);
 
+    AsyncWrap::CheckAndRemovePromiseHook(env);
+
     if (ret.IsEmpty()) {
       // NOTE: For backwards compatibility with public API we return Undefined()
       // if the top level call threw.
@@ -1365,7 +1367,11 @@ MaybeLocal<Value> MakeCallback(Environment* env,
     return ret;
   }
 
-  if (env->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
+  auto tc_ret = env->tick_callback_function()->Call(process, 0, nullptr);
+
+  AsyncWrap::CheckAndRemovePromiseHook(env);
+
+  if (tc_ret.IsEmpty()) {
     return Undefined(env->isolate());
   }
 
@@ -3611,6 +3617,8 @@ void LoadEnvironment(Environment* env) {
   // like Node's I/O bindings may want to replace 'f' with their own function.
   Local<Value> arg = env->process_object();
   f->Call(Null(env->isolate()), 1, &arg);
+
+  AsyncWrap::CheckAndRemovePromiseHook(env);
 }
 
 static void PrintHelp() {

@addaleax
Copy link
Member Author

@trevnorris I see, that would probably work. But what’s the advantage? It should have the same effects, right? In that case I’d probably prefer a version that keeps the information flow a bit more localized, instead of spread out over the various async calling facilities…

(Side note, since this is reminding me: What’s speaking against merging the two MakeCallback implementations? It seemed to pass testing last I checked, and the methods are already in that dangerous almost-but-not-quite identical shape…)

@trevnorris
Copy link
Contributor

@addaleax

It should have the same effects, right? In that case I’d probably prefer a version that keeps the information flow a bit more localized, instead of spread out over the various async calling facilities…

It causes a timing discrepancy in one edge case. Since the microtask queue is only drained after the nexttick queue is drained there's a timing issue with 'uncaughtException':

'use strict';

process.on('uncaughtException', () => { });

const s = setImmediate(() => Promise.resolve(42));

const h = require('async_hooks').createHook({
  init(id, type) { process._rawDebug(id, type) },
}).enable();

process.nextTick(() => {
  process.nextTick(() => h.disable());
  throw new Error();
});

I'd expect no PROMISE in the init() call but it shows up. Also, having it added to the microtask queue means it will disable the PromiseHook while interleaving between nextTick calls.

What’s speaking against merging the two MakeCallback implementations?

To verify, you mean node::MakeCallback and node::AsyncWrap::MakeCallback?

@addaleax
Copy link
Member Author

@trevnorris Ah, I see… I dug around that whole code a bit on the weekend, and yes, it’s a terrible tangle of edge cases. :/

Maybe ~AsyncCallbackScope would be a better case to call RemovePromiseHook, though? That should make things a bit simpler (I’d also like to merge that with AsyncHooks::ExecScope, if you don’t think that’s a terrible idea…).

What’s speaking against merging the two MakeCallback implementations?

To verify, you mean node::MakeCallback and node::AsyncWrap::MakeCallback?

Yup, those two. :)

@trevnorris
Copy link
Contributor

@addaleax

Maybe ~AsyncCallbackScope would be a better case to call RemovePromiseHook, though?

Ah yes. Should have seen this before. This wraps all necessary locations.

I’d also like to merge that with AsyncHooks::ExecScope, if you don’t think that’s a terrible idea…

If you can get AsyncCallbackScope to do the same as ExecScope::Dispose() (b/c lifetime of the id isn't the same as the lifetime of the AsyncCallbackScope) then I'd say we go for it.

Yup, those two. :)

Let me look back at why they weren't merged. When I first implemented AsyncWrap I wanted to make a single MakeCallback but there were a couple important reasons why I didn't. Sounds like you already have a patch for this. If you do, mind sharing?

@addaleax
Copy link
Member Author

Sounds like you already have a patch for this. If you do, mind sharing?

Nothing particularly exciting, I’d say:

diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index d7cdc4198c94..6ab8ec227537 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -666,65 +666,10 @@ void AsyncWrap::EmitAsyncInit(Environment* env,
 Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
                                      int argc,
                                      Local<Value>* argv) {
-  CHECK(env()->context() == env()->isolate()->GetCurrentContext());
-
-  Environment::AsyncCallbackScope callback_scope(env());
-
-  Environment::AsyncHooks::ExecScope exec_scope(env(),
-                                                get_id(),
-                                                get_trigger_id());
-
-  if (!PreCallbackExecution(this, true)) {
-    return Local<Value>();
-  }
-
-  // Finally... Get to running the user's callback.
-  MaybeLocal<Value> ret = cb->Call(env()->context(), object(), argc, argv);
-
-  Local<Value> ret_v;
-  if (!ret.ToLocal(&ret_v)) {
-    return Local<Value>();
-  }
-
-  if (!PostCallbackExecution(this, true)) {
-    return Local<Value>();
-  }
-
-  exec_scope.Dispose();
-
-  if (callback_scope.in_makecallback()) {
-    return ret_v;
-  }
-
-  Environment::TickInfo* tick_info = env()->tick_info();
-
-  if (tick_info->length() == 0) {
-    env()->isolate()->RunMicrotasks();
-  }
-
-  // Make sure the stack unwound properly. If there are nested MakeCallback's
-  // then it should return early and not reach this code.
-  CHECK_EQ(env()->current_async_id(), 0);
-  CHECK_EQ(env()->trigger_id(), 0);
-
-  Local<Object> process = env()->process_object();
-
-  if (tick_info->length() == 0) {
-    tick_info->set_index(0);
-    return ret_v;
-  }
-
-  MaybeLocal<Value> rcheck =
-      env()->tick_callback_function()->Call(env()->context(),
-                                            process,
-                                            0,
-                                            nullptr);
-
-  // Make sure the stack unwound properly.
-  CHECK_EQ(env()->current_async_id(), 0);
-  CHECK_EQ(env()->trigger_id(), 0);
-
-  return rcheck.IsEmpty() ? Local<Value>() : ret_v;
+  return node::MakeCallback(env()->isolate(),
+                            object(), cb, argc, argv,
+                            get_id(), get_trigger_id())
+      .FromMaybe(Local<Value>());
 }
 
 
diff --git a/src/node.cc b/src/node.cc
index bbce10220fe1..643e1096cac9 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1355,8 +1355,8 @@ MaybeLocal<Value> MakeCallback(Environment* env,
 
   // Make sure the stack unwound properly. If there are nested MakeCallback's
   // then it should return early and not reach this code.
-  CHECK_EQ(env->current_async_id(), async_id);
-  CHECK_EQ(env->trigger_id(), trigger_id);
+  /*CHECK_EQ(env->current_async_id(), async_id);
+  CHECK_EQ(env->trigger_id(), trigger_id);*/
 
   Local<Object> process = env->process_object();
 

I don’t quite recall why the last checks were failing.

@addaleax addaleax force-pushed the async-hooks-promise-pushpop branch from 9718b5f to fa167a1 Compare June 16, 2017 17:27
Until now, the async_hooks PromiseHook did not register the Promise’s
async id and trigger id on the id stack, so inside the `.then()` handler
those ids would be invalid.

To fix this, add push and pop calls to its `before` and `after` parts,
respectively. Some care needs to be taken for the cases that the
Promise hook is being disabled or enabled during the execution
of a Promise handler; in the former case, actually removing the hook
is delayed by adding another task to the microtask queue, in the latter
case popping the id off the async id stack is skipped if the ids don’t
match.

Fixes: nodejs#13583
PR-URL: nodejs#13585
Reviewed-By: Trevor Norris <trevnorris@gmail.com>
@addaleax addaleax force-pushed the async-hooks-promise-pushpop branch from fa167a1 to af1a551 Compare June 16, 2017 17:28
@addaleax
Copy link
Member Author

Landed in af1a551

@addaleax addaleax closed this Jun 16, 2017
@addaleax addaleax deleted the async-hooks-promise-pushpop branch June 16, 2017 17:43
@addaleax addaleax merged commit af1a551 into nodejs:master Jun 16, 2017
addaleax added a commit that referenced this pull request Jun 17, 2017
Until now, the async_hooks PromiseHook did not register the Promise’s
async id and trigger id on the id stack, so inside the `.then()` handler
those ids would be invalid.

To fix this, add push and pop calls to its `before` and `after` parts,
respectively. Some care needs to be taken for the cases that the
Promise hook is being disabled or enabled during the execution
of a Promise handler; in the former case, actually removing the hook
is delayed by adding another task to the microtask queue, in the latter
case popping the id off the async id stack is skipped if the ids don’t
match.

Fixes: #13583
PR-URL: #13585
Reviewed-By: Trevor Norris <trevnorris@gmail.com>
@addaleax addaleax mentioned this pull request Jun 17, 2017
addaleax added a commit that referenced this pull request Jun 21, 2017
Until now, the async_hooks PromiseHook did not register the Promise’s
async id and trigger id on the id stack, so inside the `.then()` handler
those ids would be invalid.

To fix this, add push and pop calls to its `before` and `after` parts,
respectively. Some care needs to be taken for the cases that the
Promise hook is being disabled or enabled during the execution
of a Promise handler; in the former case, actually removing the hook
is delayed by adding another task to the microtask queue, in the latter
case popping the id off the async id stack is skipped if the ids don’t
match.

Fixes: #13583
PR-URL: #13585
Reviewed-By: Trevor Norris <trevnorris@gmail.com>
addaleax added a commit that referenced this pull request Jun 24, 2017
Until now, the async_hooks PromiseHook did not register the Promise’s
async id and trigger id on the id stack, so inside the `.then()` handler
those ids would be invalid.

To fix this, add push and pop calls to its `before` and `after` parts,
respectively. Some care needs to be taken for the cases that the
Promise hook is being disabled or enabled during the execution
of a Promise handler; in the former case, actually removing the hook
is delayed by adding another task to the microtask queue, in the latter
case popping the id off the async id stack is skipped if the ids don’t
match.

Fixes: #13583
PR-URL: #13585
Reviewed-By: Trevor Norris <trevnorris@gmail.com>
rvagg pushed a commit that referenced this pull request Jun 29, 2017
Until now, the async_hooks PromiseHook did not register the Promise’s
async id and trigger id on the id stack, so inside the `.then()` handler
those ids would be invalid.

To fix this, add push and pop calls to its `before` and `after` parts,
respectively. Some care needs to be taken for the cases that the
Promise hook is being disabled or enabled during the execution
of a Promise handler; in the former case, actually removing the hook
is delayed by adding another task to the microtask queue, in the latter
case popping the id off the async id stack is skipped if the ids don’t
match.

Fixes: #13583
PR-URL: #13585
Reviewed-By: Trevor Norris <trevnorris@gmail.com>
addaleax added a commit that referenced this pull request Jul 11, 2017
Until now, the async_hooks PromiseHook did not register the Promise’s
async id and trigger id on the id stack, so inside the `.then()` handler
those ids would be invalid.

To fix this, add push and pop calls to its `before` and `after` parts,
respectively. Some care needs to be taken for the cases that the
Promise hook is being disabled or enabled during the execution
of a Promise handler; in the former case, actually removing the hook
is delayed by adding another task to the microtask queue, in the latter
case popping the id off the async id stack is skipped if the ids don’t
match.

Fixes: #13583
PR-URL: #13585
Reviewed-By: Trevor Norris <trevnorris@gmail.com>
@MylesBorins MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 14, 2017
@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

async_hooks: not able to preserve continuation local storage inside Promises
5 participants