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

async_hooks experimental status #14717

Closed
martinkuba opened this issue Aug 9, 2017 · 18 comments
Closed

async_hooks experimental status #14717

martinkuba opened this issue Aug 9, 2017 · 18 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem. question Issues that look for answers.

Comments

@martinkuba
Copy link

We are looking at using async_hooks to maintain state in our instrumentations when async/await is used. What is your recommendation for using async_hooks in production code at this point?

We realize that it is marked as experimental API. However, it is not gated with a flag. Is the API likely to change? When do you think it will come out of experimental status?

@tniessen tniessen added question Issues that look for answers. async_hooks Issues and PRs related to the async hooks subsystem. labels Aug 9, 2017
@tniessen
Copy link
Member

tniessen commented Aug 9, 2017

cc @nodejs/async_hooks

@refack
Copy link
Contributor

refack commented Aug 9, 2017

@martinkuba hello, a little bit of context:

  1. The module isn't gated with a flag since the code that implements it is very pervasive, and essentially it's impossible to turn it off, so a flag would have been a "false flag".
  2. Indeed the experimental demarcation is there to indicate that the API might change with no correlation to semver rules. But the work that has been done during the v8.x cycle tried to be semver-minor nonetheless (refactored APIs were back-shimmed for compatibility, e.g. async_hooks: rename currentId to currentAsyncId #13490).
    The implementation itself is not considered buggy in anyway.
  3. Some big players are betting on async_hooks for tracing and APM, but I don't know first hand of any big projects that use it in production, maybe the others can help?

My very subjective estimation is that the "user" API will not change much. Most of the work being done is on the "embedder" API i.e. code that wants to trigger the hooks, not code that wants to use the hooks and query the graph.

IMHO go for it, but I'm a bleeding edge kind of guy, so it is very dependant on your needs and organizational constraints.

@AndreasMadsen
Copy link
Member

In my opinion: treat it as if it were behind a flag.

I can almost guarantee there are going to be API changes, although we will try to make them backward compatible in 8.x, but that is only until it becomes to inconvenient.

During 8.x we have already deprecated 6 function calls or something like that.

@martinkuba
Copy link
Author

@refack @AndreasMadsen Thanks for your replies, I really appreciate it.

Have there been any performance tests done - to determine overhead and also to look for memory leaks when running over long period of time? If yes, can you please point me to those tests?

We are running benchmark tests ourselves, and would like to compare with any existing results.

@refack
Copy link
Contributor

refack commented Aug 12, 2017

@martinkuba that's a good questions re:

In this case I can only contribute anecdotal data-points. Personally I know of one project (small-medium) that uses async-hooks in a long running system in production, they are not doing rigorous benchmarking, but have not been noticing memory leaks or performance degradation. It's also a complicated data-point to analyse since they upgraded from 6.x specifically so they could use async-hooks...

  1. I believe everyone would be happy to hear New Relic's results, if you could share them.
  2. I'll post a Request For Comments from the general public.

P.S. https://twitter.com/refack/status/896460320079785987 - #14794

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Sep 15, 2017

@mcollina asked about the plans for the experimental status. Let's have the discussion here.

What I would like to see done before moving out of experimental status:

edited: added new links to #15572 and #15454.
edited: spelling and poor phrasing.

@trevnorris
Copy link
Contributor

@AndreasMadsen

Opt-in async_hooks state validation. We had decent state validation before (#14722 and #14387) but it was always active which was very problematic.

Oh, good idea. I was thinking of just leaving it for debug build, but adding a flag or env var is an excellent idea. You have any ideas about how it should be implemented?

And for future generations reading this: the only reason I removed some of the validation was because of the ecosystem breakage and not being able to find an alternative solution quickly enough. If the edge cases that caused those failures could be determined I'd like to eventually re-add all state validation.

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Sep 15, 2017

@trevnorris Well, as I have mentioned a few times I think we should just check async_hook_fields [kTotals] > 0. I realize this doesn't ensure correctness if only executionAsyncId() or triggerAsyncId() is used, but it is so trivial to add some empty hooks and we can document that "hack". One could argue that async_hooks is useful with incorrect states, I honestly think it will only lead to confusion and fewer critical bug reports. If it turns out to be a problem, we could change the behavior when moving to a stable state.

@refack
Copy link
Contributor

refack commented Sep 15, 2017

Just for context, AFAIK there hasn't been a critical async_hooks in about 2 months (I think #14387 was the latest), so the code is converging anyway.

@AndreasMadsen
Copy link
Member

@refack That is likely a false positive, there hasn't been an issue because we removed/relaxed the asserts. That doesn't mean there is no async_hooks state errors. It just means we are not seeing them anymore.

@mcollina
Copy link
Member

@AndreasMadsen we cannot ship a Node.js release with tight asserts by default. If we need them to exists, then let's put them behind a flag. In this way, we can test this in applications without causing problems for everybody else that might not even be using async_hooks.

@addaleax
Copy link
Member

Fwiw, #15448 is a pretty fresh crash coming from async_hooks, although there seems to be more going on in it.

@AndreasMadsen
Copy link
Member

we cannot ship a Node.js release with tight asserts by default. If we need them to exists, then let's put them behind a flag. In this way, we can test this in applications without causing problems for everybody else that might not even be using async_hooks.

Everyone seems to misunderstand what I'm saying. I will try and prepare a pull request today.

@AndreasMadsen
Copy link
Member

@trevnorris @mcollina #15454 is what I have in mind.

@AndreasMadsen
Copy link
Member

/cc @jasnell, see #14717 (comment) for my stable-requirements list.

MylesBorins pushed a commit that referenced this issue Oct 23, 2017
PR-URL: #15454
Ref: #14387
Ref: #14722
Ref: #14717
Ref: #15448
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 26, 2017
PR-URL: nodejs/node#15454
Ref: nodejs/node#14387
Ref: nodejs/node#14722
Ref: nodejs/node#14717
Ref: nodejs/node#15448
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this issue Dec 7, 2017
PR-URL: nodejs/node#15454
Ref: nodejs/node#14387
Ref: nodejs/node#14722
Ref: nodejs/node#14717
Ref: nodejs/node#15448
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@benjamn
Copy link
Contributor

benjamn commented Dec 22, 2017

Has anyone tried using async_hooks in an application that uses multiple threads?

V8 supports a kind of cooperative multithreading, provided you use v8::Locker and v8::Unlocker to lock the Isolate object correctly, but it appears there's just one async_ids_stack_ for the whole node::Environment, and it doesn't get unwound/archived/restored when switching between threads.

Here's an example of where this can go wrong: laverdet/node-fibers#357. Note: although Fibers are not technically threads, they could be implemented as threads, and V8 treats them as if they were threads, so the analogy holds.

Essentially, if you push an async ID on one thread, then switch threads and push another ID, then switch back to the first thread and pop the first ID, you get the corruption error because async_ids_stack_ still has the ID from the second thread at the top of the stack.

How can we make the async_hooks API more aware of threads? Should each thread have its own stack of async IDs? Should the AsyncHooks object live in thread-local storage, perhaps?

@AndreasMadsen
Copy link
Member

@benjamn Please open another issue, then we can discuss it there.

@Trott
Copy link
Member

Trott commented Oct 18, 2018

It seems like perhaps this should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. I'm just tidying up and not acting on a super-strong opinion or anything like that.

@Trott Trott closed this as completed Oct 18, 2018
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. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

9 participants