-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
Description
- Version: master (016d81c)
- Platform: all
- Subsystem: async_hooks
As I mentioned a few months ago I'm not comfortable exposing the low-level async_hooks JS API. In the documentation PR #12953 we agreed to keep the low-level JS API undocumented. Some time have now passed and I think we are in a better position to discuss this.
The low-level JS API is quite large, thus I would like to separate the discussion into:
- a discussion about
setInitTriggerIdandtriggerIdScope. (this) - edit: a discussion about
runInAsyncIdScope. (Remove async_hooks runInAsyncIdScope API #14328) - a discussion about
newUid,emitInit,emitBefore,emitAfterandemitDestroy(Remove async_hooks Sensitive/low-level Embedder API #15572).
edit: there is some overlap between emitInit and setInitTriggerId in terms of initTriggerId. Hopefully, that won't be an issue.
As I personally believe setInitTriggerId and triggerIdScope are the most questionable, I would like to discuss those first.
Background
async_hooks uses a global state called async_uid_fields[kInitTriggerId] to set the triggerAsyncId of the soon to be constructed handle object [1] [2]. It has been mentioned eariler by @trevnorris that a better solution is wanted.
To set the async_uid_fields[kInitTriggerId] field from JavaScript there are two methods setInitTriggerId and triggerIdScope.
setInitTriggerIdsets theasync_uid_fields[kInitTriggerId]directly and is used in a single case in node-core. IfinitTriggerIdis not called after when creating the handle object this will have unintended side-effects for the next handle object.triggerIdScopeis a more safe version ofsetInitTriggerIdthat creates a temporary scope, outside said scope theasync_uid_fields[kInitTriggerId]value is not modified. This is not used anywere in node-core.
Note: This issue is about removing setInitTriggerId and triggerIdScope as a public API. Not removing async_uid_fields[kInitTriggerId] itself, that may come at later time.
Issues
setInitTriggerIdis extremely sensitive and wrong use can have unintended side-effects. Consider:
async_hooks.setInitTriggerId(asyncId);
return createUserHandle();
function createUserHandle() {
if (state.freeConnections) {
// no handle is created, no initTriggerId is called
return state.getFreeConnection();
} else {
// handle is created and initTriggerId is called
return state.newConnection();
}
}In the above example setInitTriggerId will have an unintended side-effect when state.freeConnections is true. The implementation is obviously wrong but in a real world case I don't think this will be obvious.
triggerIdScopeis a safer option, but even that may fail if more than one handle is created depending on a condition.
state.newConnection = function () {
if (state.numConnection >= state.maxConnections) {
// This may do logging causing `initTriggerId` to be called.
state.emit('warnConnectionLimit');
}
// calls initTriggerId
return newConnection();
};- It is not obvious for the user when
initTriggerIdis called because the handle logic in node-core is so complex. For example, when debugging withconsole.loga new handle can be created causinginitTriggerIdto be called.
state.newConnection = function () {
// may call `initTriggerId`
console.log('new connection');
// calls initTriggerId
return newConnection();
};Such side-effect may be obvious for us, but I don't want to tell the user that console.log can have unintended side-effects.
- Handle creation may change. How many handles and the exact timing of this is not something we test. I thus think it is reasonable to believe that this is something that will cause an unintended issue in the future. Possibly during a semver-patch update.
Note, in all of these cases the handle creation and setting the initTriggerId may happen in two separate modules.
Solution
- We remove
triggerIdScope - We move
setInitTriggerIdtorequire('internal/async_hooks.js') - The user should specify the
triggerAsyncIddirectly inAsyncResourceoremitInit. The API already allows for this.
Note: We may want to just deprecate the API in node 8 and remove it in a future version.
/cc @nodejs/async_hooks