async_hooks: prevent async storage methods leaking to outer context by Qard · Pull Request #31950 · nodejs/node · GitHub

The _exit logic is not actually necessary as explained here. This eliminates it and gives sync context runs a proper AsyncResource.

cc @vdeturckheim @puzpuzpuz @Flarna

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

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

Couple comments, also, I am afraid this comes with a perf cost in the *SyncAndReturn(...) 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.

PR-URL: nodejs#31950 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

Sounds fine to me. 👍

`runSyncAndReturn` was removed from Node.js in nodejs/node#31950

PR-URL: nodejs#31950 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

PR-URL: #31950 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

Notable Changes: * async_hooks**: * Merge `run` and `exit` methods (Andrey Pechkurov) #31950 * Prevent sync methods of async storage exiting outer context (Stephen Belanger) #31950 * vm: * Add `importModuleDynamically` option to compileFunction (Gus Caplan) #32985 New core collaborators: With this release, we welcome two new Node.js core collaborators: * Juan José Arboleda @juanarbol #32906 * Andrey Pechkurov @puzpuzpuz #32817 PR-URL: #33122

PR-URL: #31950 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

Notable Changes: * async_hooks**: * Merge `run` and `exit` methods (Andrey Pechkurov) #31950 * Prevent sync methods of async storage exiting outer context (Stephen Belanger) #31950 * vm: * Add `importModuleDynamically` option to compileFunction (Gus Caplan) #32985 New core collaborators: With this release, we welcome two new Node.js core collaborators: * Juan José Arboleda @juanarbol #32906 * Andrey Pechkurov @puzpuzpuz #32817 PR-URL: #33122

@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.

Qard added lib / src Issues and PRs related to general changes in the lib or src directory. async_hooks Issues and PRs related to the async hooks subsystem. labels Feb 25, 2020

Qard self-assigned this Feb 25, 2020

Qard changed the title Async storage eliminate exit transition async_hooks: eliminate _exit in async storage Feb 25, 2020

Qard mentioned this pull request Feb 25, 2020

async_hooks: add sync enterWith to async storage #31945

Closed

4 tasks

vdeturckheim suggested changes Feb 25, 2020

View reviewed changes

Comment thread lib/async_hooks.js Outdated

Comment thread lib/async_hooks.js Outdated

Comment thread lib/async_hooks.js Outdated

vdeturckheim reviewed Feb 25, 2020

View reviewed changes

Comment thread lib/async_hooks.js Outdated

puzpuzpuz reviewed Feb 25, 2020

View reviewed changes

Comment thread lib/async_hooks.js Outdated

Qard changed the title async_hooks: eliminate _exit in async storage async_hooks: prevent sync async storage methods from exiting outer context Feb 26, 2020

Qard changed the title async_hooks: prevent sync async storage methods from exiting outer context async_hooks: prevent sync methods of async storage exiting outer context Feb 26, 2020

Flarna reviewed Feb 26, 2020

View reviewed changes

Comment thread lib/async_hooks.js Outdated

Qard added the blocked PRs that are blocked by other issues or PRs. label Feb 26, 2020

Qard removed the blocked PRs that are blocked by other issues or PRs. label Feb 28, 2020

Qard force-pushed the async-storage-eliminate-exit-transition branch from 5e07676 to 2b6a63c Compare February 28, 2020 05:51

Qard force-pushed the async-storage-eliminate-exit-transition branch from 2b6a63c to 23a0772 Compare February 28, 2020 06:15

puzpuzpuz reviewed Feb 28, 2020

View reviewed changes

Comment thread lib/async_hooks.js Outdated

puzpuzpuz mentioned this pull request Feb 28, 2020

test: improve disable AsyncLocalStorage test #31998

Closed

3 tasks

Qard force-pushed the async-storage-eliminate-exit-transition branch from 417ff73 to 2db2994 Compare March 1, 2020 06:51

puzpuzpuz added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 14, 2020

BethGriggs mentioned this pull request Apr 14, 2020

2020-04-21, Version 14.0.0 (Current) #32181

Merged

puzpuzpuz added a commit to puzpuzpuz/node that referenced this pull request Apr 14, 2020

async_hooks: merge run and exit methods

c6f7f55

PR-URL: nodejs#31950 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

puzpuzpuz mentioned this pull request Apr 14, 2020

v13.13.0 release proposal #32813

Merged

targos added a commit to targos/async-http-context that referenced this pull request Apr 24, 2020

fix: use run method and throw error used outside of context

4e690d3

`runSyncAndReturn` was removed from Node.js in nodejs/node#31950

targos mentioned this pull request Apr 24, 2020

fix: use run method and throw error used outside of context RomainLanz/async-http-context#1

Merged

targos removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 25, 2020

targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020

async_hooks: merge run and exit methods

ccbf4f9

PR-URL: nodejs#31950 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

BridgeAR pushed a commit that referenced this pull request Apr 28, 2020

async_hooks: prevent sync methods of async storage exiting outer context

b304df9

PR-URL: #31950 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

BridgeAR pushed a commit that referenced this pull request Apr 28, 2020

async_hooks: merge run and exit methods

52d8afc

PR-URL: #31950 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

BridgeAR mentioned this pull request Apr 28, 2020

v13.14.0 proposal #33122

Merged

targos pushed a commit that referenced this pull request Apr 28, 2020

async_hooks: prevent sync methods of async storage exiting outer context

3eb3406

PR-URL: #31950 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

targos pushed a commit that referenced this pull request Apr 28, 2020

async_hooks: merge run and exit methods

a35e88c

PR-URL: #31950 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

Qard deleted the async-storage-eliminate-exit-transition branch May 4, 2020 18:05

vmarchaud mentioned this pull request Jun 18, 2020

AsyncLocalStorage based ContextManager open-telemetry/opentelemetry-js#1210

Merged

async_hooks: prevent async storage methods leaking to outer context by Qard · Pull Request #31950 · nodejs/node · GitHub

Uh oh!

Uh oh!

async_hooks: prevent async storage methods leaking to outer context - #31950

async_hooks: prevent async storage methods leaking to outer context#31950Qard wants to merge 2 commits intonodejs:masternodejs/node:masterfrom Qard:async-storage-eliminate-exit-transitionQard/node:async-storage-eliminate-exit-transitionCopy head branch name to clipboard

Conversation

Qard commented Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Qard commented Feb 25, 2020

Uh oh!

vdeturckheim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Qard commented Feb 25, 2020

Uh oh!

vdeturckheim commented Feb 25, 2020

Uh oh!

Uh oh!

Uh oh!

puzpuzpuz commented Feb 25, 2020

Uh oh!

Qard commented Feb 26, 2020

Uh oh!

Qard commented Feb 26, 2020

Uh oh!

Qard commented Feb 26, 2020

Uh oh!

puzpuzpuz commented Feb 26, 2020

Uh oh!

Uh oh!

vdeturckheim commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Qard commented Feb 26, 2020

Uh oh!

Qard commented Feb 28, 2020

Uh oh!

Uh oh!

vdeturckheim commented Feb 28, 2020

Uh oh!

puzpuzpuz commented Apr 14, 2020

Uh oh!

Qard commented Apr 14, 2020

Uh oh!

kibertoad commented May 4, 2020

Uh oh!

legendecas commented May 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

async_hooks: prevent async storage methods leaking to outer context#31950
Qard wants to merge 2 commits into
nodejs:masterfrom
Qard:async-storage-eliminate-exit-transition

Qard commented Feb 25, 2020 •
edited
Loading

vdeturckheim commented Feb 26, 2020 •
edited
Loading