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,async_wrap: fixups and fewer aborts #14722

Closed
wants to merge 4 commits into from

Conversation

trevnorris
Copy link
Contributor

@trevnorris trevnorris commented Aug 9, 2017

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, async_wrap

Commit summary:

  • Fix case where returning early from AsyncWrap::MakeCallback because of a disposed domain caused an abort.
  • Improvements to code comments and logic
  • Force exit less frequently

Note: Still reasoning about automatically coercing undefined values in a way that won't corrupt the stack.

CI: https://ci.nodejs.org/job/node-test-pull-request/9577/

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 9, 2017
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.

Only took a quick look.

@@ -127,8 +127,8 @@ inline v8::Local<v8::String> Environment::AsyncHooks::provider_string(int idx) {

inline void Environment::AsyncHooks::push_ids(double async_id,
double trigger_id) {
CHECK_GE(async_id, 0);
CHECK_GE(trigger_id, 0);
CHECK_GE(async_id, -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 is -1 now a special allowed value?

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK -1 is now used to represent a recoverable error in the graph.
https://github.com/trevnorris/node/blob/d21e3d9e7875ec8dfe478b587a932f7f3c56221c/lib/async_hooks.js#L381-L384
See the last comment in the OP Note: Still reasoning about automatically coercing undefined values in a way that won't corrupt the stack. the idea is to coerce all the places that exploded till now, because the asyncIDs where undefined, to -1 and treat those as "graph-opaque" bad actors instead of aborting.

Copy link
Member

Choose a reason for hiding this comment

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

I see. My personal opinion is that those checks should simply be skipped when AsyncHooks isn't used. Relaxing the check when AsyncHooks is used, isn't a good solution. I know that is an unpopular opinion, so I won't block it now, but I really feel we are on a steap slope to some very difficult to debug errors. In the future I want a more detailed discussion about this, unfortunately I don't have time these days.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from commit message body:

  • id values of -1 are allowed. They indicate that the id was never
    correctly assigned to the async resource. These will appear in any
    call graph, and will only be apparent to those using the async_hooks
    module, then reported in an issue.

we can't guarantee the async id is always assigned properly, and was one reason applications were aborting. history has taught me that it's impossible to make sure the async id is always properly defined because of how much node internals can be abused. so -1 is to indicate that the async/trigger id wasn't properly set.

i'm working on one more commit to translate nan values to -1, but there are some edge case when tracking the stack that can leave the application in a bad state. if i can't figure those out soon will push it up regardless and look for feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack.
I tend to agree, but the -1 trick currently isn't public API, so we could change it if we agree on a better solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AndreasMadsen

My personal opinion is that those checks should simply be skipped when AsyncHooks isn't used.

It's not explicitly documented, but one goal of the API was to allow async_hooks.executionAsyncId() and async_hooks.triggerAsyncId() to be used regardless whether a hook has been enabled or not. We could possibly do the same trick that we do with domains and enable the checks if async_hooks is required, but domains is for performance reasons. Always performing these checks should be possible without ever causing the application to crash, and the additional cost probably isn't measurable.

Relaxing the check when AsyncHooks is used, isn't a good solution.

I fully agree'd with this until I realized how badly node core can be abused by allowing users to pass in arbitrary objects instead of ones created by core constructors (e.g. timers). So it is necessary to coerce, at minimum, async ids of undefined.

I really feel we are on a steap slope to some very difficult to debug errors.

Again, I fully agree'd with this until I came to terms with the reason pointed out above.

@refack

I tend to agree, but the -1 trick currently isn't public API, so we could change it if we agree on a better solution.

I was thinking about doing something more complex, like

  • -1 - indicates the async_id_symbol was undefined
  • -2 - indicates async_id_symbol was set but not a positive integer

etc. But figured that's too much to anticipate, and wouldn't be worth the time.

Copy link
Member

Choose a reason for hiding this comment

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

Please don't document it. I'm on vacation and of all the things I disagree with, our general approach to being robust against implementation errors is what I disagree the most about.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 on keeping this an "internal implementation" detail for now.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

PromiseHook() never needs to run domains

Why does it not need to do that?

@@ -696,7 +697,7 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
}

if (!PostCallbackExecution(this, true)) {
return Local<Value>();
return Undefined(env()->isolate());
Copy link
Member

Choose a reason for hiding this comment

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

Please also take a look at #14697 when you get the chance.

restoreTmpHooks();
// Hooks can only be restored if there have been no recursive hook calls.
// Also the active hooks do not need to be restored if enable()/disable()
// weren't called during hook execution. In which case
Copy link
Member

Choose a reason for hiding this comment

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

grammar nit: during hook execution, in which case

restoreTmpHooks();
// Hooks can only be restored if there have been no recursive hook calls.
// Also the active hooks do not need to be restored if enable()/disable()
// weren't called during hook execution. In which case
Copy link
Member

Choose a reason for hiding this comment

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

same grammar nit ;)

@refack refack added async_hooks Issues and PRs related to the async hooks subsystem. async_wrap labels Aug 10, 2017
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

CRs in tests
Other comments are nits/suggestions

// Used to make sure active_hooks_array isn't altered in mid execution if
// another hook is added or removed. A counter is used to track nested calls.
var processing_hook = 0;
// Use a counter to track nested calls of async hook callbacks and make sure
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:
since these four values are used to protect active_hooks_array's atomicity, we could group them into a "namespace":

const active_hooks = {
  array: [],
  call_depth: 0,
  tmp_array: null,
  tmp_fields: null,
  getHookArrays(),
  storeActiveHooks(),
  restoreActiveHooks()
}

Copy link
Contributor Author

@trevnorris trevnorris Aug 11, 2017

Choose a reason for hiding this comment

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

I like it.

EDIT: don't feel the need to have the functions in the same namespace since they're already defined outside of the script.

@@ -42,6 +54,8 @@ const { kInit, kBefore, kAfter, kDestroy, kTotals, kCurrentAsyncId,
kCurrentTriggerId, kAsyncUidCntr,
kInitTriggerId } = async_wrap.constants;

// Symbols used to store the respective ids on both AsyncResource instances and
// internal resources.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this more correct?
// Symbols used to store the respective ids on AsyncResource instances.
// Also used to cache the ids on internal resources for easy JS access.

Copy link
Contributor Author

@trevnorris trevnorris Aug 11, 2017

Choose a reason for hiding this comment

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

yes, but should also add that they'll be added to any object that enters the internal async path; because users can pass arbitrary objects instead of using internally constructed ones.

EDIT: forgot the reason I left it at "internal resources" is because timers aren't AsyncResource instances and also don't read their asyncId from c++. this may also change in the future when the PR lands that allows arbitrary ids to be assigned to new AsyncWrap instances; which allows a JS object to receive an asyncId early.

@@ -53,6 +67,8 @@ const emitBeforeNative = emitHookFactory(before_symbol, 'emitBeforeNative');
const emitAfterNative = emitHookFactory(after_symbol, 'emitAfterNative');
const emitDestroyNative = emitHookFactory(destroy_symbol, 'emitDestroyNative');

const abort_regex = /^--abort[_-]on[_-]uncaught[_-]exception$/;
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add a TODO(refack): move to node-config.cc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -127,8 +127,8 @@ inline v8::Local<v8::String> Environment::AsyncHooks::provider_string(int idx) {

inline void Environment::AsyncHooks::push_ids(double async_id,
double trigger_id) {
CHECK_GE(async_id, 0);
CHECK_GE(trigger_id, 0);
CHECK_GE(async_id, -1);
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK -1 is now used to represent a recoverable error in the graph.
https://github.com/trevnorris/node/blob/d21e3d9e7875ec8dfe478b587a932f7f3c56221c/lib/async_hooks.js#L381-L384
See the last comment in the OP Note: Still reasoning about automatically coercing undefined values in a way that won't corrupt the stack. the idea is to coerce all the places that exploded till now, because the asyncIDs where undefined, to -1 and treat those as "graph-opaque" bad actors instead of aborting.

// When AsyncWrap::MakeCallback() returned an empty handle the
// MaybeLocal::ToLocalChecked() call caused the process to abort. By returning
// v8::Undefined() it allows the error to propagate to the 'error' event.
s.on('error', common.mustCall((e) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What triggers this? the dispose on next tick?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The abort or the 'error' event?

The abort is because we're currently returning Local<Value>() from AsyncWrap::MakeCallback(); which now returns a MaybeLocal<Value>. So it will abort on MaybeLocal::ToLocalChecked() if the domain is disposed. Instead of propagating the error correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

the error event? since there is not interaction with s, I'm assuming that it's some cleanup code that triggers the error event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@refack implementation details of how the JSStream instance dictates how the Socket connects. Because the JSStream instance isn't connected to anything the socket reports EINVAL.

'use strict';

const common = require('../common');
const JSStream = process.binding('js_stream').JSStream;
Copy link
Contributor

Choose a reason for hiding this comment

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

could you put this together with Socket and comment something like // arbitrary objects that trigger async_hooks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, not sure what you mean by "put this together with Socket". Mind clarifying?

Copy link
Contributor

Choose a reason for hiding this comment

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

const common = require('../common');
const assert = require('assert');
const domain = require('domain');

// These were picked arbitrarily and are only used to trigger arync_hooks
const JSStream = process.binding('js_stream').JSStream;
const Socket = require('net').Socket;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -101,6 +101,8 @@ module.exports = exports = {
// Note: Please try to keep these in alphabetical order
E('ERR_ARG_NOT_ITERABLE', '%s must be iterable');
E('ERR_ASSERTION', '%s');
E('ERR_ASYNC_CALLBACK', (name) => `${name} must be a function`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add perfunctory tests in test/parallel/test-internal-errors.js as per https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md#testing-new-errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@trevnorris
Copy link
Contributor Author

@addaleax

PromiseHook() never needs to run domains

Why does it not need to do that?

I inferred that PromiseHook() never needed to run domains because it wasn't running domains in the previous code. I'd need to ask @matthewloring as to why it wasn't running domains before this.

@trevnorris
Copy link
Contributor Author

@refack

the idea is to coerce all the places that exploded till now, because the asyncIDs where undefined, to -1 and treat those as "graph-opaque" bad actors instead of aborting.

One of the reasons for application crashing is because the async_id on the object was undefined, which necessitates properly coercing nan values internally. Brief list of the edge cases for handling this properly:

  • Setting async_id = -1 if async_id == nan in the before() call may become invalid if the instance's async_id value is set between the before() and after() calls. Technically invalidating the graph.
  • The async_id value may be set to -1 or nan if the instance is destroyed between the before() or after().

Simple way to handle this is to always succeed on the condition of either async_id or uid_fields_[kCurrentAsyncId] equaling -1. I believe this is a viable solution, but felt like I haven't worked though all the logic cases. Eh, I'll throw it up and run it through cigtm.

@refack
Copy link
Contributor

refack commented Aug 11, 2017

PromiseHook() never needs to run domains

Why does it not need to do that?

I asked myself the same quastion and when I saw

PreCallbackExecution(wrap, false);

where the false argument means don't do domains.

v8::MaybeLocal::ToLocalChecked() will abort on an empty handle.
AsyncWrap::MakeCallback returns an empty handle if the domain is
disposed, but this should not cause the process to abort. So instead
return v8::Undefined() and allow the error to be handled normally.

PR-URL: nodejs#14722
* Reword several of the comments to be more descriptive.
* Rename functions to better indicate what they are doing.
* Change AsyncHooks::uid_fields_ to be a fixed array instead of a
  pointer.
* Define regex early so line ends before 80 columns.
* Remove obsolete comments.
* Rename AsyncHooks::InitScope::uid_fields_ to uid_fields_ptr_ because
  using the same name as AsyncHooks::uid_fields_ was confusing.
* Place variables that are used to store the new set of hooks if another
  hook is enabled/disabled during hook execution into an object to act
  as a namespace.

PR-URL: nodejs#14722
* Because Emit{Before,After}() will always exit the process if there's
  an exception there's no need to check a return value. This simplifies
  the conditions and makes {Pre,Post}CallbackExecution() unnecessary.
  They have been removed and relevant code has been moved to the
  respective call sites. Which are:
  * PromiseHook() never needs to run domains, and without a return value
    to check place the calls to Emit{Before,After}() on location.
  * The logic in MakeCallback() is simplified by moving the single calls
    to Emit{Before,After}() then doing a simpler conditional to check if
    the domain has been disposed.
* Change Domain{Enter,Exit}() to return true if the instance has been
  disposed. Makes the conditional check in MakeCallback() simpler to
  reason about.
* Add UNREACHABLE() after FatalException() to make sure all error
  handlers have been cleared and the process really does exit.

PR-URL: nodejs#14722
* id values of -1 are allowed. They indicate that the id was never
  correctly assigned to the async resource. These will appear in any
  call graph, and will only be apparent to those using the async_hooks
  module, then reported in an issue.
* ids < -1 are still not allowed and will cause the application to
  exit the process; because there is no scenario where this should ever
  happen.
* Add asyncId range checks to emitAfterScript().
* Fix emitBeforeScript() range checks which should have been || not &&.
* Replace errors with entries in internal/errors.
* Fix async_hooks tests that check for exceptions to match new
  internal/errors entries.

NOTE: emit{Before,After,Destroy}() must continue to exit the process
because in the case of an exception during hook execution the state of
the application is unknowable. For example, an exception could cause a
memory leak:

    const id_map = new Map();

    before(id) {
      id_map.set(id, /* data object or similar */);
    },
    after(id) {
      throw new Error('id never dies!');
      id_map.delete(id);
    }

Allowing a recoverable exception may also cause an abort because of a
stack check in Environment::AsyncHooks::pop_ids() that verifies the
async id and pop'd ids match. This case would be more difficult to debug
than if fatalError() (lib/async_hooks.js) was called immediately.

    try {
      async_hooks.emitBefore(null, NaN);
    } catch (e) { }
    // do something
    async_hooks.emitAfter(5);

It also allows an edge case where emitBefore() could be called twice and
not have the pop_ids() CHECK fail:

    try {
      async_hooks.emitBefore(5, NaN);
    } catch (e) { }
    async_hooks.emitBefore(5);
    // do something
    async_hooks.emitAfter(5);

There is the option of allowing mismatches in the stack and ignoring the
check if no async hooks are enabled, but I don't believe going this far
is necessary.

PR-URL: nodejs#14722
@addaleax
Copy link
Member

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

I intend to merge this if CI comes back good, but I agree that maybe this should not be the last word re: robustness of the checks.

@addaleax
Copy link
Member

CI is green

Landed in 11a2ca2...062beb0

@addaleax addaleax closed this Aug 14, 2017
addaleax pushed a commit that referenced this pull request Aug 14, 2017
v8::MaybeLocal::ToLocalChecked() will abort on an empty handle.
AsyncWrap::MakeCallback returns an empty handle if the domain is
disposed, but this should not cause the process to abort. So instead
return v8::Undefined() and allow the error to be handled normally.

PR-URL: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 14, 2017
* Reword several of the comments to be more descriptive.
* Rename functions to better indicate what they are doing.
* Change AsyncHooks::uid_fields_ to be a fixed array instead of a
  pointer.
* Define regex early so line ends before 80 columns.
* Remove obsolete comments.
* Rename AsyncHooks::InitScope::uid_fields_ to uid_fields_ptr_ because
  using the same name as AsyncHooks::uid_fields_ was confusing.
* Place variables that are used to store the new set of hooks if another
  hook is enabled/disabled during hook execution into an object to act
  as a namespace.

PR-URL: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 14, 2017
* Because Emit{Before,After}() will always exit the process if there's
  an exception there's no need to check a return value. This simplifies
  the conditions and makes {Pre,Post}CallbackExecution() unnecessary.
  They have been removed and relevant code has been moved to the
  respective call sites. Which are:
  * PromiseHook() never needs to run domains, and without a return value
    to check place the calls to Emit{Before,After}() on location.
  * The logic in MakeCallback() is simplified by moving the single calls
    to Emit{Before,After}() then doing a simpler conditional to check if
    the domain has been disposed.
* Change Domain{Enter,Exit}() to return true if the instance has been
  disposed. Makes the conditional check in MakeCallback() simpler to
  reason about.
* Add UNREACHABLE() after FatalException() to make sure all error
  handlers have been cleared and the process really does exit.

PR-URL: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 14, 2017
* id values of -1 are allowed. They indicate that the id was never
  correctly assigned to the async resource. These will appear in any
  call graph, and will only be apparent to those using the async_hooks
  module, then reported in an issue.
* ids < -1 are still not allowed and will cause the application to
  exit the process; because there is no scenario where this should ever
  happen.
* Add asyncId range checks to emitAfterScript().
* Fix emitBeforeScript() range checks which should have been || not &&.
* Replace errors with entries in internal/errors.
* Fix async_hooks tests that check for exceptions to match new
  internal/errors entries.

NOTE: emit{Before,After,Destroy}() must continue to exit the process
because in the case of an exception during hook execution the state of
the application is unknowable. For example, an exception could cause a
memory leak:

    const id_map = new Map();

    before(id) {
      id_map.set(id, /* data object or similar */);
    },
    after(id) {
      throw new Error('id never dies!');
      id_map.delete(id);
    }

Allowing a recoverable exception may also cause an abort because of a
stack check in Environment::AsyncHooks::pop_ids() that verifies the
async id and pop'd ids match. This case would be more difficult to debug
than if fatalError() (lib/async_hooks.js) was called immediately.

    try {
      async_hooks.emitBefore(null, NaN);
    } catch (e) { }
    // do something
    async_hooks.emitAfter(5);

It also allows an edge case where emitBefore() could be called twice and
not have the pop_ids() CHECK fail:

    try {
      async_hooks.emitBefore(5, NaN);
    } catch (e) { }
    async_hooks.emitBefore(5);
    // do something
    async_hooks.emitAfter(5);

There is the option of allowing mismatches in the stack and ignoring the
check if no async hooks are enabled, but I don't believe going this far
is necessary.

PR-URL: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@trevnorris
Copy link
Contributor Author

@addaleax Thanks for landing it.

I agree that maybe this should not be the last word re: robustness of the checks.

I am happy to entertain discussion on this topic. As the one who originally wrote all the checks, I fully believe the checks should be strict, but after several painful bugs and realizing node's API allows arbitrary objects to be passed in in-place-of internally constructed instances I realized that relaxing the checks was necessary (if in no other case when the async_id is undefined).

MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
v8::MaybeLocal::ToLocalChecked() will abort on an empty handle.
AsyncWrap::MakeCallback returns an empty handle if the domain is
disposed, but this should not cause the process to abort. So instead
return v8::Undefined() and allow the error to be handled normally.

PR-URL: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
* Reword several of the comments to be more descriptive.
* Rename functions to better indicate what they are doing.
* Change AsyncHooks::uid_fields_ to be a fixed array instead of a
  pointer.
* Define regex early so line ends before 80 columns.
* Remove obsolete comments.
* Rename AsyncHooks::InitScope::uid_fields_ to uid_fields_ptr_ because
  using the same name as AsyncHooks::uid_fields_ was confusing.
* Place variables that are used to store the new set of hooks if another
  hook is enabled/disabled during hook execution into an object to act
  as a namespace.

PR-URL: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
* Because Emit{Before,After}() will always exit the process if there's
  an exception there's no need to check a return value. This simplifies
  the conditions and makes {Pre,Post}CallbackExecution() unnecessary.
  They have been removed and relevant code has been moved to the
  respective call sites. Which are:
  * PromiseHook() never needs to run domains, and without a return value
    to check place the calls to Emit{Before,After}() on location.
  * The logic in MakeCallback() is simplified by moving the single calls
    to Emit{Before,After}() then doing a simpler conditional to check if
    the domain has been disposed.
* Change Domain{Enter,Exit}() to return true if the instance has been
  disposed. Makes the conditional check in MakeCallback() simpler to
  reason about.
* Add UNREACHABLE() after FatalException() to make sure all error
  handlers have been cleared and the process really does exit.

PR-URL: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
* id values of -1 are allowed. They indicate that the id was never
  correctly assigned to the async resource. These will appear in any
  call graph, and will only be apparent to those using the async_hooks
  module, then reported in an issue.
* ids < -1 are still not allowed and will cause the application to
  exit the process; because there is no scenario where this should ever
  happen.
* Add asyncId range checks to emitAfterScript().
* Fix emitBeforeScript() range checks which should have been || not &&.
* Replace errors with entries in internal/errors.
* Fix async_hooks tests that check for exceptions to match new
  internal/errors entries.

NOTE: emit{Before,After,Destroy}() must continue to exit the process
because in the case of an exception during hook execution the state of
the application is unknowable. For example, an exception could cause a
memory leak:

    const id_map = new Map();

    before(id) {
      id_map.set(id, /* data object or similar */);
    },
    after(id) {
      throw new Error('id never dies!');
      id_map.delete(id);
    }

Allowing a recoverable exception may also cause an abort because of a
stack check in Environment::AsyncHooks::pop_ids() that verifies the
async id and pop'd ids match. This case would be more difficult to debug
than if fatalError() (lib/async_hooks.js) was called immediately.

    try {
      async_hooks.emitBefore(null, NaN);
    } catch (e) { }
    // do something
    async_hooks.emitAfter(5);

It also allows an edge case where emitBefore() could be called twice and
not have the pop_ids() CHECK fail:

    try {
      async_hooks.emitBefore(5, NaN);
    } catch (e) { }
    async_hooks.emitBefore(5);
    // do something
    async_hooks.emitAfter(5);

There is the option of allowing mismatches in the stack and ignoring the
check if no async hooks are enabled, but I don't believe going this far
is necessary.

PR-URL: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
v8::MaybeLocal::ToLocalChecked() will abort on an empty handle.
AsyncWrap::MakeCallback returns an empty handle if the domain is
disposed, but this should not cause the process to abort. So instead
return v8::Undefined() and allow the error to be handled normally.

PR-URL: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
* Reword several of the comments to be more descriptive.
* Rename functions to better indicate what they are doing.
* Change AsyncHooks::uid_fields_ to be a fixed array instead of a
  pointer.
* Define regex early so line ends before 80 columns.
* Remove obsolete comments.
* Rename AsyncHooks::InitScope::uid_fields_ to uid_fields_ptr_ because
  using the same name as AsyncHooks::uid_fields_ was confusing.
* Place variables that are used to store the new set of hooks if another
  hook is enabled/disabled during hook execution into an object to act
  as a namespace.

PR-URL: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
* Because Emit{Before,After}() will always exit the process if there's
  an exception there's no need to check a return value. This simplifies
  the conditions and makes {Pre,Post}CallbackExecution() unnecessary.
  They have been removed and relevant code has been moved to the
  respective call sites. Which are:
  * PromiseHook() never needs to run domains, and without a return value
    to check place the calls to Emit{Before,After}() on location.
  * The logic in MakeCallback() is simplified by moving the single calls
    to Emit{Before,After}() then doing a simpler conditional to check if
    the domain has been disposed.
* Change Domain{Enter,Exit}() to return true if the instance has been
  disposed. Makes the conditional check in MakeCallback() simpler to
  reason about.
* Add UNREACHABLE() after FatalException() to make sure all error
  handlers have been cleared and the process really does exit.

PR-URL: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
* id values of -1 are allowed. They indicate that the id was never
  correctly assigned to the async resource. These will appear in any
  call graph, and will only be apparent to those using the async_hooks
  module, then reported in an issue.
* ids < -1 are still not allowed and will cause the application to
  exit the process; because there is no scenario where this should ever
  happen.
* Add asyncId range checks to emitAfterScript().
* Fix emitBeforeScript() range checks which should have been || not &&.
* Replace errors with entries in internal/errors.
* Fix async_hooks tests that check for exceptions to match new
  internal/errors entries.

NOTE: emit{Before,After,Destroy}() must continue to exit the process
because in the case of an exception during hook execution the state of
the application is unknowable. For example, an exception could cause a
memory leak:

    const id_map = new Map();

    before(id) {
      id_map.set(id, /* data object or similar */);
    },
    after(id) {
      throw new Error('id never dies!');
      id_map.delete(id);
    }

Allowing a recoverable exception may also cause an abort because of a
stack check in Environment::AsyncHooks::pop_ids() that verifies the
async id and pop'd ids match. This case would be more difficult to debug
than if fatalError() (lib/async_hooks.js) was called immediately.

    try {
      async_hooks.emitBefore(null, NaN);
    } catch (e) { }
    // do something
    async_hooks.emitAfter(5);

It also allows an edge case where emitBefore() could be called twice and
not have the pop_ids() CHECK fail:

    try {
      async_hooks.emitBefore(5, NaN);
    } catch (e) { }
    async_hooks.emitBefore(5);
    // do something
    async_hooks.emitAfter(5);

There is the option of allowing mismatches in the stack and ignoring the
check if no async hooks are enabled, but I don't believe going this far
is necessary.

PR-URL: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
v8::MaybeLocal::ToLocalChecked() will abort on an empty handle.
AsyncWrap::MakeCallback returns an empty handle if the domain is
disposed, but this should not cause the process to abort. So instead
return v8::Undefined() and allow the error to be handled normally.

PR-URL: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
* Reword several of the comments to be more descriptive.
* Rename functions to better indicate what they are doing.
* Change AsyncHooks::uid_fields_ to be a fixed array instead of a
  pointer.
* Define regex early so line ends before 80 columns.
* Remove obsolete comments.
* Rename AsyncHooks::InitScope::uid_fields_ to uid_fields_ptr_ because
  using the same name as AsyncHooks::uid_fields_ was confusing.
* Place variables that are used to store the new set of hooks if another
  hook is enabled/disabled during hook execution into an object to act
  as a namespace.

PR-URL: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
* Because Emit{Before,After}() will always exit the process if there's
  an exception there's no need to check a return value. This simplifies
  the conditions and makes {Pre,Post}CallbackExecution() unnecessary.
  They have been removed and relevant code has been moved to the
  respective call sites. Which are:
  * PromiseHook() never needs to run domains, and without a return value
    to check place the calls to Emit{Before,After}() on location.
  * The logic in MakeCallback() is simplified by moving the single calls
    to Emit{Before,After}() then doing a simpler conditional to check if
    the domain has been disposed.
* Change Domain{Enter,Exit}() to return true if the instance has been
  disposed. Makes the conditional check in MakeCallback() simpler to
  reason about.
* Add UNREACHABLE() after FatalException() to make sure all error
  handlers have been cleared and the process really does exit.

PR-URL: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
* id values of -1 are allowed. They indicate that the id was never
  correctly assigned to the async resource. These will appear in any
  call graph, and will only be apparent to those using the async_hooks
  module, then reported in an issue.
* ids < -1 are still not allowed and will cause the application to
  exit the process; because there is no scenario where this should ever
  happen.
* Add asyncId range checks to emitAfterScript().
* Fix emitBeforeScript() range checks which should have been || not &&.
* Replace errors with entries in internal/errors.
* Fix async_hooks tests that check for exceptions to match new
  internal/errors entries.

NOTE: emit{Before,After,Destroy}() must continue to exit the process
because in the case of an exception during hook execution the state of
the application is unknowable. For example, an exception could cause a
memory leak:

    const id_map = new Map();

    before(id) {
      id_map.set(id, /* data object or similar */);
    },
    after(id) {
      throw new Error('id never dies!');
      id_map.delete(id);
    }

Allowing a recoverable exception may also cause an abort because of a
stack check in Environment::AsyncHooks::pop_ids() that verifies the
async id and pop'd ids match. This case would be more difficult to debug
than if fatalError() (lib/async_hooks.js) was called immediately.

    try {
      async_hooks.emitBefore(null, NaN);
    } catch (e) { }
    // do something
    async_hooks.emitAfter(5);

It also allows an edge case where emitBefore() could be called twice and
not have the pop_ids() CHECK fail:

    try {
      async_hooks.emitBefore(5, NaN);
    } catch (e) { }
    async_hooks.emitBefore(5);
    // do something
    async_hooks.emitAfter(5);

There is the option of allowing mismatches in the stack and ignoring the
check if no async hooks are enabled, but I don't believe going this far
is necessary.

PR-URL: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
PR-URL: #15454
Ref: #14387
Ref: #14722
Ref: #14717
Ref: #15448
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
PR-URL: nodejs/node#15454
Ref: nodejs/node#14387
Ref: nodejs/node#14722
Ref: nodejs/node#14717
Ref: nodejs/node#15448
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
PR-URL: nodejs/node#15454
Ref: nodejs/node#14387
Ref: nodejs/node#14722
Ref: nodejs/node#14717
Ref: nodejs/node#15448
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants