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 runInAsyncIdScope API #14328

Closed
AndreasMadsen opened this issue Jul 17, 2017 · 6 comments
Closed

Remove async_hooks runInAsyncIdScope API #14328

AndreasMadsen opened this issue Jul 17, 2017 · 6 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem. discuss Issues opened for discussions and feedbacks.

Comments

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Jul 17, 2017

  • 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 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

runInAsyncIdScope(asyncId, cb) creates a new scope with the asyncId as
the executionAsyncId and with the current executionAsyncId as
the triggerAsyncId. It does so without invoking the before and after hooks.

runInAsyncIdScope was not part of the original async_hooks EP but
was included in the async_hooks PR.

runInAsyncIdScope is not used anywhere in node-core and as such, it purpose
is not well documented. @trevnorris mentions it only a single time:

This could also be used by something like a database module, where multiple queries should be treated as their own async operations, but the actual operation is combined into a single query internally. This is the reason for async_hooks.runInAsyncIdScope():

// A pooled resource on top of a native resource
class MyResource inherits async_hooks.AsyncResource {
  constructor() {
    super('MyResource');
  }
  query(val, cb) {
    // query() will pool the actual request then call each callback with
    // the specific data it requested.
    nativeResource.query(val, (er, data) => {
      async_hooks.runInAsyncIdScope(this.asyncId(), () => cb(er, data));
    });
  }
}

const mr = new MyResource();
mr.query(val, (er, data) => {
  // Now when init() fires for writeFile() it will have both the currentId and
  // triggerId of the JS instance.
  fs.writeFile(path, data.toString());
});

Although, later @trevnorris says it is a bad example.

Issues

  • The primary issue is that emitBefore and emitAfter are not used, thus
    the before and after hooks are never invoked. This can be an issue if the user
    depends on the executionAsyncId() to match the asyncId in the before hook.

For example, in trace I at some point used:

let stack = [];

createHook({
  before(asyncId) {
    stack.push(states.get(asyncId));
  },
  after(asyncId) {
    stack.pop();
  }
})

However, because of runInAsyncIdScope this is actually invalid code.

Solution

  • We remove runInAsyncIdScope
  • The user should use AsyncResource and emitBefore/emitAfter to change
    executionAsyncId() and triggerAsyncId().

Note: We may want to just deprecate the API in node 8 and remove it in a future version.

For example, the DB resource example should be implemented as:

// A pooled resource on top of a native resource
class MyResource inherits async_hooks.AsyncResource {
  constructor() {
    super('MyResource');
  }
  query(val, cb) {
    // query() will pool the actual request then call each callback with
    // the specific data it requested.
    nativeResource.query(val, (er, data) => {
      this.emitBefore();
      cb(er, data);
      this.emitAfter();
    });
  }
}

/cc @nodejs/async_hooks

@AndreasMadsen AndreasMadsen added async_hooks Issues and PRs related to the async hooks subsystem. discuss Issues opened for discussions and feedbacks. labels Jul 17, 2017
@TimothyGu
Copy link
Member

I'd like to voice an issue I encountered when trying to use AsyncResource. Because it is a ES6 class, it is impossible for me to create a class that extends both AsyncResource and, say, EventEmitter. Is there a way to resolve this issue?

@AndreasMadsen
Copy link
Member Author

@TimothyGu Short answer is that you can use newUid, emitInit, emitBefore, emitAfter and emitDestroy. I will address it in a future issue about emitInit and friends.

@TimothyGu
Copy link
Member

TimothyGu commented Jul 18, 2017

@AndreasMadsen Okay. I would still much prefer a more generic solution though, like so:

class MyClass extends EventEmitter {
  constructor() {
    super();
    AsyncResource.call(this, 'name');
  }

  method(cb) {
    this.on('event', () => {
      native((err, data) => {
        this.emitBefore();
        try {
          cb(err, data);
        } finally {
          this.emitAfter();
        }
      });
    });
  }
}
Object.defineProperties(
    MyClass.prototype,
    Object.getOwnPropertyDescriptors(AsyncResource.prototype));

I don't want to derail the original conversation of course, and I'd be happy to move this into a new issue.

@trevnorris
Copy link
Contributor

trevnorris commented Jul 18, 2017

runInAsyncIdScope() is a bad API. It's should actually be initTriggerIdScope() (i.e. it should dictate the triggerId passed to init(), nothing more). The pre-PR implementation did this, but I must have overlooked the functionality difference along the way. In short, this API shouldn't have existed in the first place.

So, setInitTriggerId() and runInAsyncIdScope() don't address the same problem. Let's remove this API completely, and continue discussion about the need for setInitTriggerId() in #14238.

EDIT: I tracked down the last commit left in my repo to show that it did at one point do as previously intended at trevnorris@4b16a58#diff-0bb01a51b135a5f68d93540808bac801R179

@AndreasMadsen
Copy link
Member Author

AndreasMadsen commented Jul 18, 2017

I don't want to derail the original conversation of course, and I'd be happy to move this into a new issue.

@TimothyGu Please do.

@trevnorris
Copy link
Contributor

@TimothyGu Please add the link to the new issue here when it's created.

MylesBorins pushed a commit that referenced this issue Jan 8, 2018
PR-URL: #16972
Refs: #14328
Refs: #15572
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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 <anna@addaleax.net>
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 <anna@addaleax.net>
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. discuss Issues opened for discussions and feedbacks.
Projects
None yet
Development

No branches or pull requests

3 participants