Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove async_hooks Sensitive/low-level Embedder API #15572

Closed
AndreasMadsen opened this issue Sep 23, 2017 · 0 comments
Closed

Remove async_hooks Sensitive/low-level Embedder API #15572

AndreasMadsen opened this issue Sep 23, 2017 · 0 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem.

Comments

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Sep 23, 2017

  • Version: master (7a95392)
  • 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 has 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:

Note: There is some overlap between emitInit and setInitTriggerId in terms of initTriggerId. Hopefully, that won't be an issue.


Background

For implementers the JavaScript Embedding API is used to emit the init,
before, after and destroy events. There are currently two APIs for
doing this.

  • The high-level AsyncResource class API.
  • The low-level newUid, emitInit, emitBefore, emitAfter, emitDestroy. Sometimes this is called the Sensitive Embedder API, but sometimes that also captures triggerAsyncIdScope(asyncId, cb) which is a very different discussion. Thus I will refer to it as low-level JavaScript Embedder API.

This discussion is just about the newUid, emitInit, emitBefore, emitAfter, emitDestroy API.

A common purpose for the Embedder API is to allow queued jobs to not lose the causal context. An example of this is setTimeout (shown implementation is only conceptual).

const { newUid, initTriggerId, emitInit,
        emitBefore, emitAfter, emitDestroy } = require('async_hooks');


function Timeout(timeout) {
  this.timeout = timeout;
  this.ontimeout = null;
}

function setTimeout(callback, timeout) {
  const handle = new Timeout(timeout);
  const asyncId = newUid();
  const triggerAsyncId = initTriggerId();
  emitInit(asyncId, 'Timeout', triggerAsyncId, handle);
  handle.ontimeout = function () {
    emitBefore(asyncId, triggerAsyncId);
    callback();
    emitAfter(asyncId);
    emitDestroy(asyncId);
  };

  // This may emit an init event if a new request is created, but it can also
  // add the timeout to an existing request. When the request is completed
  // it will emit before and after around all the callbacks.
  // The operation will typically be implemented in C++.
  // function done(callbackList) {
  //   emitBefore(...)
  //   callbackList.forEach((callback) => callback());
  //   emitAfter(...)
  //   emitDestroy(...)
  // }
  addToQueue(handle);
}

Alternatively, this can be implemented using the high-level AsyncResource
class.

const { AsyncResource } = require('async_hooks');

class Timeout extends AsyncResource {
  constructor(timeout) {
    this.timeout = timeout;
    this.ontimeout = null;
    super('Timeout');
  }
}

function setTimeout(callback, timeout) {
  const handle = new Timeout(timeout);
  handle.ontimeout = function () {
    handle.emitBefore();
    callback();
    handle.emitAfter();
    handle.emitDestroy();
  };

  addToQueue(handle);
}

The low-level JavaScript Embedder API exists to:

  • To make it easier to add the embedding calls to existing code.
  • To solve an important issue with
    multiple inheritance, where the Timeout class would already inherit from
    a different class, that the implementer doesn't control.

Issues

The issues with the low-level JavaScript Embedder API are:

  • emitInit can be called twice with the same async_id.
  • emitDestroy can be called twice with the same async_id. Also possible
    with AsyncResource, but we could quite easily fix that.
  • The triggerAsyncId in emitBefore can be different than in emitInit.
    Definetly the biggest issue, since it is rather confusing that triggerAsyncId
    can be null in emitInit but not in emitBefore.
  • It is possible to hijack an existing async_id and emit before and after.
    Might be very theoretical.

Okay, so the problems are perhaps not super serious. But I would like to argue that the reasons for the low-level JavaScript Embedder API are not valid, as one can easily use AsyncResource in all cases.

In general restricting the public API to just AsyncResource will give us much more flexibility in state validation. And I think it will be prone to much fewer errors.

Solution

The simplest solution is to create a separate handle object that just
points to the actual handle object.

const { AsyncResource } = require('async_hooks');

function Timeout(timeout) {
  this.timeout = timeout;
  this.ontimeout = null;
}

class TimeoutResource extends AsyncResource {
  constructor(handle) {
    this.handle = handle;
    super('Timeout');
  }
}

function setTimeout(callback, timeout) {
  const handle = new Timeout(timeout);
  const resource = new TimeoutResource(hanlde);

  handle.ontimeout = function () {
    resource.emitBefore();
    callback();
    resource.emitAfter();
    resource.emitDestroy();
  };

  addToQueue(handle);
}

If creating of the extra resource object is somehow inconvenient, it is
possible to call the AsyncResource methods directly on the handle object.

const { AsyncResource } = require('async_hooks');

function Timeout(timeout) {
  this.timeout = timeout;
  this.ontimeout = null;
}

function setTimeout(callback, timeout) {
  const handle = new Timeout(timeout);
  AsyncResource.call(handle, 'Timeout');

  handle.ontimeout = function () {
    AsyncResource.prototype.emitBefore.call(handle);
    callback();
    AsyncResource.prototype.emitAfter.call(handle);
    AsyncResource.prototype.emitDestroy.call(handle);
  };

  addToQueue(handle);
}

The latter solution would require that we use ES5 type classes. Alternatively
we could add a static emitInit method to AsyncResource that can act as a constructor.

const { AsyncResource } = require('async_hooks');

function Timeout(timeout) {
  this.timeout = timeout;
  this.ontimeout = null;
}

function setTimeout(callback, timeout) {
  const handle = new Timeout(timeout);
  AsyncResource.emitInit(handle, 'Timeout');

  handle.ontimeout = function () {
    AsyncResource.emitBefore(handle);
    callback();
    AsyncResource.emitAfter(handle);
    AsyncResource.emitDestroy(handle);
  };

  addToQueue(handle);
}

Our AsyncResource implementation would just be:

class AsyncResource {
  constructor(name) { AsyncResource.emitInit(this, name); }
  static emitInit(self, name) {
    // current constructor code
  }

  emitBefore() { AsyncResource.emitBefore(this); }
  static emitBefore(self) {
    // current AsyncResource.prototype.emitBefore code
  }

  emitAfter() { AsyncResource.emitAfter(this); }
  static emitAfter(self) {
    // current AsyncResource.prototype.emitAfter code
  }

  emitDestroy() { AsyncResource.emitDestroy(this); }
  static emitDestroy(self) {
    // current AsyncResource.prototype.emitDestroy code
  }

  // ...
}

/cc @nodejs/async_hooks

@AndreasMadsen AndreasMadsen added the async_hooks Issues and PRs related to the async hooks subsystem. label Sep 23, 2017
MylesBorins pushed a commit that referenced this issue Jan 8, 2018
PR-URL: #16972
Refs: #14328
Refs: #15572
Reviewed-By: Anna Henningsen <[email protected]>
gibfahn pushed a commit to gibfahn/node that referenced this issue Jan 17, 2018
PR-URL: nodejs#16972
Backport-PR-URL: nodejs#18179
Refs: nodejs#14328
Refs: nodejs#15572
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 19, 2018
Backport-PR-URL: #18179
PR-URL: #16972
Refs: #14328
Refs: #15572
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem.
Projects
None yet
Development

No branches or pull requests

1 participant