async_hooks,async_wrap: fixups and fewer aborts by trevnorris · Pull Request #14722 · nodejs/node
nodejs-github-bot
added
c++
labels
Aug 9, 2017v8::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 pushed a commit that referenced this pull request
Aug 14, 2017v8::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, 2017v8::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, 2017v8::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, 2017v8::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, 2017MylesBorins pushed a commit that referenced this pull request
Oct 23, 2017This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters