Skip to content

Remove async_hooks setTrigger API #14238

Closed
@AndreasMadsen

Description

@AndreasMadsen
  • 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:

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    async_hooksIssues and PRs related to the async hooks subsystem.discussIssues opened for discussions and feedbacks.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions