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: add AsyncLocal class #27172

Closed
wants to merge 6 commits into from
Closed

async_hooks: add AsyncLocal class #27172

wants to merge 6 commits into from

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Apr 10, 2019

This introduces a new API to provide asynchronous storage via a new
AsyncLocal class. Besides setting a value which is passed along
asynchronous invocations it allows to register a callback to get
informed about changes of the current value.

The implementation is based on async_hooks but it doesn't expose
internals like execution Ids, resources or the hooks itself.

Naming and implementation is inspired by .NET AsyncLocal.

There is already #26540 adding similar functionality then this one and
we should for sure not add both.
I'm not sure if creating a new PR in parallel is the best way to
discuss this but I think it's the easiest variant to allow anyone to
give both a try.

Main differences between #26540 and this one:

  • async-hooks: introduce async-storage API #26540 follows trigger path of resources wheres this on follows execution path
  • This one has a copy on write semantic for the async value
  • This one allows to install a callback to get notified if value changes

As prove of concept I tried locally to port domains to use AsyncLocal instead
async_hooks. I was able to get all tests green except 3 base on native
addons using a domain property on the resource which seems to be deprecated
meanwhile so I stopped there. The notification callback was needed to get this far.

Refs: https://docs.microsoft.com/en-us/dotnet/api/system.threading.asynclocal-1?view=netframework-4.7.2
Refs: #26540

fyi @nodejs/diagnostics @watson @vdeturckheim

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Apr 10, 2019
const pVal = stack[stack.length - 1];
stack.push(pVal);
if (cVal !== pVal)
this[kOnChangedCb](pVal, cVal, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This invocation is very risky. User callbask should be called outside of async hooks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, then it will not work as moving it outside of async hooks would require something like nextTick which is a new async operation. As a result the signaled values and the real values don't match anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this cannot be made safe, I don't think supporting this "onchanged" behavior is compatible for the goal of "ease of use" that this API is supposed to have.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will improve docs in this, add a check to catch recursion cases (e.g. onChangedCb sets a new value) and handle exceptions thrown by onChangedCb. Hopefully this renders this callback as save as all the other callbacks which can be registered in node core.

const pVal = stack[stack.length - 1];
stack.push(pVal);
if (cVal !== pVal)
this[kOnChangedCb](pVal, cVal, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this cannot be made safe, I don't think supporting this "onchanged" behavior is compatible for the goal of "ease of use" that this API is supposed to have.

@mscdex
Copy link
Contributor

mscdex commented Apr 10, 2019

Judging by the included example, it's difficult to see why we would ever need more than one instance of AsyncLocal, which if that's the case, why not just have it be a singleton in node core?

@jasnell
Copy link
Member

jasnell commented Apr 10, 2019

I'm in agreement with @mcollina on the callback issue. It's not clear if there's a safe way of doing that.

On the AsyncLocal singleton, if there's only going to be a single instance, I'd prefer an API like...

const { setAsyncLocal, getAsyncLocal } = require('async_hooks');

const value = { /* ... */ };
setAsyncLocal(value);

// ...
const value = getAsyncLocal();

// ...
setAsyncLocal();  // Reset to undefined

@jkrems
Copy link
Contributor

jkrems commented Apr 10, 2019

I think building proper namespaces into the API makes sense (as opposed to having one singleton). E.g. the application may want to track its own metadata while also using a 3rd party/APM tool that may use these APIs. With a singleton, that seems to be a bit dangerous (🤞 that no keys are reused).

@Flarna
Copy link
Member Author

Flarna commented Apr 10, 2019

If this cannot be made safe, I don't think supporting this "onchanged" behavior is compatible for the goal of "ease of use" that this API is supposed to have.

Actually I had two targets in mind with this:

  • Simple API for asynchronous storage/CLS. In most cases AsyncLocal will work without supplying a onChanged callback.
  • Allow more complex use cases similar as domains where you have to change some other state on a context change. The onChanged callback enables this. If we remove it such usecase will still require AsyncHooks in public API.

By the way, I don't understand why a user callbacks here is more dangerous then in async hooks itself or in any other place where I can install a callback. Maybe it's just a mater of adding more documentation that it may be called "frequently" and that async operations should be not triggered there similar as we have it in async_hooks currently.

Regarding singleton: I don't think this will fit well if you consider independent users, e.g. an APM, a Logger. They should be isolated similar as they would be isolated if using a thread local in a thread based system. The AsyncLocal instance should be actually seen as your private key into the "AsyncStorage".
I guess async_hooks are multi user capable for exactly the same reason.

@sam-github
Copy link
Contributor

/cc @misterdjules (you may be interested)

lib/async_hooks.js Show resolved Hide resolved
lib/async_hooks.js Show resolved Hide resolved
test/async-hooks/test-async-local.js Show resolved Hide resolved
@Flarna
Copy link
Member Author

Flarna commented Apr 19, 2019

I added some more docs and a check to disallow setting of a new value from the onChangedCb. Hope I added the new error code correct.
I haven't added anything like catching exceptions from callback as this is also not done at other places like EventEmitter.

@mcollina @jasnell Please let me know if you think this callback is now as safe as other callbacks.

This introduces a new API to provide asynchronous storage via a new
AsyncLocal class. Besides setting a value which is passed along
asynchronous invocations it allows to register a callback to get
informed about changes of the current value.

The implementation is based on async_hooks but it doesn't expose
internals like execution Ids, resources or the hooks itself.

Naming and implementation is inspired by .NET AsyncLocal.
@Flarna
Copy link
Member Author

Flarna commented Jan 23, 2020

Closing as it seems there is not much interest in this and there are other PRs implementing parts of this.

Besides that some more work would be needed:

  • allow to stop propagation depending on some conditon (e.g. avoid propagating forever through an interval timer,...)
  • disable/clear an async local

Maybe I will publish this as an userspace module at some time

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

Successfully merging this pull request may close these issues.

8 participants