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

fix: Remove usage of experimental API #89

Closed
wants to merge 1 commit into from

Conversation

RDIL
Copy link

@RDIL RDIL commented Nov 3, 2023

Node.js has moved async hooks to experimental status, and it's use is strongly discouraged now. I don't entirely understand why async hooks is needed here, so I just removed it entirely. This should yield a decent performance improvement, as async hooks is known to have a lot of overhead.

See nodejs/node#45369.

@dougwilson
Copy link
Member

So for thr history, it was added bc everyone begged for it to be added because I guess there are tracing and other libs that use async hooks so such and this was required so the context was not lost. There is a test for this in our test suite, which of course now fails with this removal. I don't think we can just remove this unless the ecosystem as a whole moves away from using whatever these modules are for tracing.

@dougwilson
Copy link
Member

I looked at the Node.js docs and the Node.js PR you referenced and it seems that AsyncLocalStorage is still supported and stable. But without the code you removed, the AsyncLocalStorage breaks due to this module. I am happy to remove this if you have an alternative solution that keeps AsyncLocalStorage functioning with this module.

@RDIL
Copy link
Author

RDIL commented Nov 20, 2023

Not entirely sure how to fix it. @Qard, do you mind taking a look if you have time? You probably understand these APIs better than I do.

@Qard
Copy link

Qard commented Nov 21, 2023

Ah, yeah, AsyncResource is stable and this is still required for AsyncLocalStorage to function correctly. This can't be removed. Any time callbacks are delayed or deferred in some way there will need to be AsyncResource to connect the callback to where the task originated.

@RDIL
Copy link
Author

RDIL commented Nov 22, 2023

Got it, thanks for the insight!

@RDIL RDIL closed this Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants