async_hooks,async_wrap: fixups and fewer aborts by trevnorris · Pull Request #14722 · nodejs/node

@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

AndreasMadsen

addaleax

refack

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

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>

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

This was referenced

Sep 15, 2017

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

addaleax pushed a commit to ayojs/ayo that referenced this pull request

Dec 7, 2017