GeezerVison

Also, with the addition of the AsyncResource in the *SyncAndReturn methods, I'm not convinced we still need separate sync and async methods.

Yes, there would be some perf cost to the AsyncResource, however it's the more correct way to handle this and the perf impact should be fairly minimal as AsyncResource skips the native side of async_hooks, instead just calling the lifecycle handlers directly..

Yes, there would be some perf cost to the AsyncResource, however it's the more correct way to handle this and the perf impact should be fairly minimal as AsyncResource skips the native side of async_hooks, instead just calling the lifecycle handlers directly..

Because of _propagate that would still mean that the perf impact of calling it grows with the current number of active instances of AsyncLocalStorage. I'd prefer to avoid calling the init hook alltogether in the sync track.

This PR was aimed to fix nested .run*() calls problem discussed here: #31945 (comment)

Yet, I don't see any tests that verify the fix.

No it wasn't, I made this as an improvement not as a fix. I can rework it to verify that it fixes that too though, if you want, as it probably does.

Added a test to verify it fixes that issue.

I restored the _exit method as the PR is not really about that anymore.

Also, what do you all think about just making the *SyncAndReturn methods into the base methods and eliminating the existing run and exit? They are somewhat redundant as they are functionally pretty much the same with the AsyncResource in there. The only difference being the callback is not actually executed async, which I don't think actually matters. If anything, I'd find it a bit unexpected that they are async by default. It's similar to the function given to a new Promise, in my opinion. 🤔

I restored the _exit method as the PR is not really about that anymore.

Also, what do you all think about just making the *SyncAndReturn methods into the base methods and eliminating the existing run and exit?

I like the idea of keeping only a single pair of methods. But I'd rather rename *SyncAndReturn into run and exit and update their documentation. It doesn't make sense to use longer names when there are no alternative methods.

@Qard I don't really understand the benchmarks, did you delete one message? Also, if I understand well, we still introduce a perf impact on the sync path that grows with the number of concurent instances. I'd like to avoid this case.

Yes, I deleted a comment awhile ago as I ran the benchmark wrong. The correct benchmark is the one in the thread near the top of the PR. It shows that the performance difference is basically non-existent.

Rebased as #31930 has landed now.

Probably worth waiting for #31998 to land first

Marked this one with semver-minor label, as it shouldn't land in a patch release. Let me know if you don't feel the same.

Sounds fine to me. 👍

@Qard Sorry if I misunderstand something, but was this merged only to 12.x but not to 14.x?

@kibertoad a683e87 is the commit that landed on 14.x branch and is released with v14.0.0.