Remove async_hooks setTrigger API · Issue #14238 · nodejs/node (original) (raw)

• 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 setInitTriggerId and triggerIdScope. (this)
• edit: a discussion about runInAsyncIdScope. (Remove async_hooks runInAsyncIdScope API #14328)
• a discussion about newUid, emitInit, emitBefore, emitAfter and emitDestroy (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.

• setInitTriggerId sets the async_uid_fields[kInitTriggerId] directly and is used in a single case in node-core. If initTriggerId is not called after when creating the handle object this will have unintended side-effects for the next handle object.
• triggerIdScope is a more safe version of setInitTriggerId that creates a temporary scope, outside said scope the async_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 asyncuidfields[kInitTriggerId] itself, that may come at later time.

Issues

• setInitTriggerId is 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.

• triggerIdScope is 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 initTriggerId is called because the handle logic in node-core is so complex. For example, when debugging with console.log a new handle can be created causing initTriggerId to 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 setInitTriggerId to require('internal/async_hooks.js')
• The user should specify the triggerAsyncId directly in AsyncResource or emitInit. 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