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: prevent async storage methods leaking to outer context #31950

Closed

Conversation

Qard
Copy link
Member

@Qard Qard commented Feb 25, 2020

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

@Qard 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 Qard self-assigned this Feb 25, 2020
@Qard Qard changed the title Async storage eliminate exit transition async_hooks: eliminate _exit in async storage Feb 25, 2020
@Qard
Copy link
Member Author

Qard commented Feb 25, 2020

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

Copy link
Member

@vdeturckheim vdeturckheim left a comment

Choose a reason for hiding this comment

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

Couple comments, also, I am afraid this comes with a perf cost in the *SyncAndReturn(...) methods

lib/async_hooks.js Outdated Show resolved Hide resolved
lib/async_hooks.js Show resolved Hide resolved
lib/async_hooks.js Outdated Show resolved Hide resolved
@Qard
Copy link
Member Author

Qard commented Feb 25, 2020

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

@vdeturckheim
Copy link
Member

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.

lib/async_hooks.js Outdated Show resolved Hide resolved
lib/async_hooks.js Outdated Show resolved Hide resolved
@puzpuzpuz
Copy link
Member

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.

@Qard
Copy link
Member Author

Qard commented Feb 26, 2020

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.

@Qard
Copy link
Member Author

Qard commented Feb 26, 2020

Added a test to verify it fixes that issue.

@Qard 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 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
@Qard
Copy link
Member Author

Qard commented Feb 26, 2020

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

@puzpuzpuz
Copy link
Member

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.

lib/async_hooks.js Outdated Show resolved Hide resolved
@Qard Qard added the blocked PRs that are blocked by other issues or PRs. label Feb 26, 2020
@vdeturckheim
Copy link
Member

vdeturckheim commented Feb 26, 2020

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

@Qard
Copy link
Member Author

Qard commented Feb 26, 2020

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.

@Qard Qard removed the blocked PRs that are blocked by other issues or PRs. label Feb 28, 2020
@Qard Qard force-pushed the async-storage-eliminate-exit-transition branch from 5e07676 to 2b6a63c Compare February 28, 2020 05:51
@Qard
Copy link
Member Author

Qard commented Feb 28, 2020

Rebased as #31930 has landed now.

@Qard Qard force-pushed the async-storage-eliminate-exit-transition branch from 2b6a63c to 23a0772 Compare February 28, 2020 06:15
lib/async_hooks.js Outdated Show resolved Hide resolved
@vdeturckheim
Copy link
Member

Probably worth waiting for #31998 to land first

@Qard Qard force-pushed the async-storage-eliminate-exit-transition branch from 417ff73 to 2db2994 Compare March 1, 2020 06:51
@puzpuzpuz puzpuzpuz added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 14, 2020
@puzpuzpuz
Copy link
Member

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.

puzpuzpuz added a commit to puzpuzpuz/node that referenced this pull request Apr 14, 2020
PR-URL: nodejs#31950
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
@Qard
Copy link
Member Author

Qard commented Apr 14, 2020

Sounds fine to me. 👍

targos added a commit to targos/async-http-context that referenced this pull request Apr 24, 2020
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
PR-URL: nodejs#31950
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
PR-URL: #31950
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
PR-URL: #31950
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Apr 28, 2020
BridgeAR added a commit that referenced this pull request Apr 28, 2020
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
targos pushed a commit that referenced this pull request Apr 28, 2020
PR-URL: #31950
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
PR-URL: #31950
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
BridgeAR added a commit that referenced this pull request Apr 28, 2020
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
BridgeAR added a commit that referenced this pull request Apr 29, 2020
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
@kibertoad
Copy link
Contributor

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

@legendecas
Copy link
Member

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

@Qard Qard deleted the async-storage-eliminate-exit-transition branch May 4, 2020 18:05
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. lib / src Issues and PRs related to general changes in the lib or src directory. 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.

10 participants