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

Ergonomic AsyncContext.wrap / AsyncResource alternative #21

Closed
legendecas opened this issue Jan 30, 2023 · 7 comments · Fixed by #55
Closed

Ergonomic AsyncContext.wrap / AsyncResource alternative #21

legendecas opened this issue Jan 30, 2023 · 7 comments · Fixed by #55

Comments

@legendecas
Copy link
Member

legendecas commented Jan 30, 2023

AsyncContext.wrap captures the storage with a function. If multiple callbacks needs to be wrapped in the same context, AsyncContext.wrap needs to be called multiple times and the storage needs to be captured respectively.

A snapshotting API would be helpful for capturing the context used for multiple callbacks, instead of wrapping each individually. Node.js provides AsyncResource to capture the storage and allow multiple callbacks to be run with the captured storage: asyncResource.runInAsyncScope(cb).

Possible API:

class AsyncContext {
    // Returns a callback that captures the current async context. It invokes a
    // callback passed into it within the captured async context.
    static snapshot(): (fn, ...args) => any;
}

Or it can be reified as a proper class like AsyncContext.Snapshot.

/cc @jasnell

@jasnell
Copy link

jasnell commented Jan 30, 2023

Thank you. It would be worth pointing out that the implementation of snapshot() is as simple as:

static snapshot() {
  return AsyncContext.wrap((cb, ... args) => cb(... args));
}

@legendecas legendecas changed the title Ergonomic AsyncContext.wrap Ergonomic AsyncContext.wrap / alternative to AsyncResource Feb 2, 2023
@legendecas legendecas changed the title Ergonomic AsyncContext.wrap / alternative to AsyncResource Ergonomic AsyncContext.wrap / AsyncResource alternative Feb 2, 2023
@mcollina
Copy link

mcollina commented Feb 2, 2023

I do not think wrap() is equivalent to AsyncResource, as this implementation adds a new function allocation for every wrapped function. This is costly even if implemented in the VM: in applications that use context tracking, I expect wrap to be called hundreds of thousands of times per second.

Node.js AsyncResource does not perform any additional allocation: https://github.com/nodejs/node/blob/main/lib/async_hooks.js#L197-L210.

I might be crossing the issues a bit, but AsyncResource does help with support for EventEmitter and similar: https://github.com/nodejs/node/blob/60996372c4e0f3b535460460ad1ca6d9a2d564b0/lib/events.js#L167.


As a side note: I think the wrap() function is very ergonomic, however, having a lower-level primitive might be preferable given the target audience of these APIs.

Having a Snapshot class would actually achieve the same result: new Snapshot().wrap(fn), with the benefit of having a more performant API if there is a need.

@jridgewell
Copy link
Member

jridgewell commented Feb 2, 2023

I do not think wrap() is equivalent to AsyncResource, as this implementation adds a new function allocation for every wrapped function.

You can create a single wrapped function, and reuse its context-restoration for multiple callbacks. This is the example given by snapshot in #21 (comment)

const snapshot = AsyncContext.wrap((cb, ...args) => cb(...args));

snapshot((a, b, c) => {  } , 1, 2, 3);
snapshot(fn1);
snapshot(fn2);

We might ease this by making a static AsyncContext.snapshot() method which is exactly as proposed in that comment.

@jasnell
Copy link

jasnell commented Feb 3, 2023

@mcollina ... it's not wrap() by itself that is equivalent to AsyncResource, it's specifically the snapshot pattern that @jridgewell describes. Specifically, if we ignore the async_hooks integration of AsyncResource, the following two are equivalent:

// Using AsyncResource
class Foo extends AsyncResource {
  constructor() { super("Foo") }
  bar() { this.runInAsyncScope(() => {}) }
}

// Using AsyncContext.snapshot / AsyncContext.wrap((cb, ...args) => cb(...args))
class Foo {
  #runInAsyncScope = AsyncContext.snapshot();
  bar() { this.runInAsyncScope(() => {}) }
}

@mcollina
Copy link

mcollina commented Feb 3, 2023

This is ok, thanks for the clarification.

I would recommend including snapshot, as wrap can be misused creating a significant performance issue.

@mhofman
Copy link
Member

mhofman commented Apr 18, 2023

If we chose to model this as a class, I would like to keep the ability to "wrap" functions to run in the snapshotted context.

I think that could be solved by having a method on these snapshot instances, e.g.:

const wrapped = snapshot.bind(fn);

@jridgewell
Copy link
Member

During today's meeting, we decided that the ergonomics of AsyncContext.wrap() really aren't that good (evidenced by this proposal to ease HOF). Instead, we'd like to rework this as 2 classes (tying into #44):

  1. AsyncContext will be renamed to AsyncLocal (or AsyncVariable, naming is just a placeholder)
  2. AsyncContext.wrap() will be replaced by an AsyncSnapshot class (or AsyncContext class, because "context" represents the full mapping and not an individual value in the mapping)

The exact naming and structure is TBD. Do we use a namespace? Factory methods? Two global classes that can be constructed? Don't know, but we're definitely going to remove AsyncContext.wrap() as a function wrapper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants