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 AsyncResource.bindCurrent() #43033

Closed
wants to merge 1 commit into from

Conversation

bengl
Copy link
Member

@bengl bengl commented May 10, 2022

This new static method does roughly the same thing as AsyncResource.bind(), except that it does not create a new AsyncResource. Instead, it relies on the existing current one.

This is an optimization useful in basically any situation where bind() would be called without passing (or relying on) the type argument.

I opted to make a completely different method, rather than overload bind() even more than it already is, but I can change that if desired.

This static method does roughly the same thing as AsyncResource.bind(),
except that it does not create a new AsyncResource. Instead, it relies
on the existing current one.

This is useful in situations where a callback is intended to be run in
the _current_ context regardless of when/where it's called.
@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. needs-ci PRs that need a full CI run. labels May 10, 2022
@targos
Copy link
Member

targos commented May 10, 2022

@nodejs/async_hooks

@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label May 10, 2022
added: REPLACEME
-->

* `fn` {Function} The function to bind to the current execution context.
Copy link
Member

Choose a reason for hiding this comment

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

When would one use bindCurrent instead of bind ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Almost any time you'd think you'd want to use the static bind. It avoids the creation of a new AsyncResource instance, which in many cases isn't actually needed.

Copy link
Member

Choose a reason for hiding this comment

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

Should we explain in the documentation when bind is actually needed?

@bengl
Copy link
Member Author

bengl commented May 10, 2022

Another option here would be to make bind() avoid creating a new AsyncResource when type is not provided. That would potentially be a semver-major change (although the docs make no guarantees of resources being created), but perhaps warranted?

@bengl bengl marked this pull request as draft May 11, 2022 14:30
@bengl
Copy link
Member Author

bengl commented May 11, 2022

Converted this to a draft, because I'd like to explore just modifying bind()'s behaviour first. That would arguably be semver-patch, since neither the docs nor the tests make any assertions about whether a new AsyncResource is created when type is not provided.

@bengl
Copy link
Member Author

bengl commented May 12, 2022

Closing this in favour of #43065. It turned out this approach (i.e. replacing the the new AsyncResource() with calls to executionAsyncResource(), etc.) wasn't particularly beneficial.

@bengl bengl closed this May 12, 2022
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. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants