-
Notifications
You must be signed in to change notification settings - Fork 317
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
What's the status of async_local_storage scope implementation? #1060
Comments
The decision was to make it an option initially as, at least at first, there was no solution to the thenables issue in V8 at the time of merging. The async_hooks scope manager has its own hack to fix it within the agent, but I later made a fix directly to V8. If you are using Node.js 14.x then AsyncLocalStorage should just work for basically anything you need it for. The backport of the fix has not yet reached 12.x though. Because the difference between the two scope managers is relatively significant it seemed best to at least give a bit of time with AsyncLocalStorage as an option rather immediately switch the default, just to make sure it doesn't have any unforeseen issues that could break many production users. To be clear: if you are on 14.x, it should be perfectly safe to use AsyncLocalStorage, as far as I can tell. |
@Qard Thanks for the context! Allow me double check my understanding.
Unfortunately we're on 12.x, and pre 12.17.0, which means we don't get AsyncLocalStorage, but I'm considering upgrading it. This all depends on whether we get the fix on later version. If it's on the V8 side, 12.x probably is not going to get it since my understanding is we don't upgrade V8 between minor versions. General curiosity, could you help me understand a bit about the thenable bug, its impact and point me to the fix you made? Thanks again! |
Yes, there was a bug in V8 which made thenables not emit proper async_hooks events. You can find a deeper explanation of the issue here: https://gist.github.com/Qard/faad53ba2368db54c95828365751d7bc The async_hooks scope manager has its own hack within the agent to deal with the issue. There was not a good way to replicate the hack for the AsyncLocalStorage scope manager. A fix has already been made to V8 and landed about two months ago. This is the fix targeting 14.x, which has landed: nodejs/node#33778 and this is the backport to 12.x, which has not yet landed: nodejs/node#34776 (The V8 fix was adapted to support the older V8 version which, in this particular case, was only minimally different) AsyncLocalStorage has actually been backported to 12.x already, but because the V8 fix has not yet landed there, it's not entirely stable yet. You can play with it now if you want though. As long as you avoid modules that use thenables, or wrap them in real promises, it should work fine. |
@Qard Thanks so much for the detailed explanation, the writeup is awesome! I guess this issue can be closed now, the followup seem to be
I'll subscribe the Node PR. I'll leave this open a while in case people think it's useful for tracking, but feel free to close! |
Just FYI: The fix landed in 12.x a few hours ago. 😄 |
Given async_hooks scope has pretty bad performance (reference #695), it seems that
async_local_storage
can be used. Looking at the implementation, it's a much easier/clearer approach.Based on my own local test, async_local_storage basically has no performance impact as if I set
enabled: false
, while async_hooks slowed down the program by about 2x.But I cannot see any documentation around
async_local_storage
, nor it's part ofindex.d.ts
's type definition of thescope
config (and plus the doc suggest against settingscope
in the first place), so I'm wondering what's the status of that implementation? Is it recommended at all? Has anyone used it in production?@Qard @rochdev Mentioned people on the original PR 🙏
The text was updated successfully, but these errors were encountered: