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: add sync enterWith to async storage #31945

Closed
wants to merge 1 commit into from

Conversation

@Qard
Copy link
Member

Qard commented Feb 25, 2020

This allows transitioning the entire following sync and async execution sub-tree to the given async storage context. With this one can be sure the context binding will remain for any following sync activity and all descending async execution whereas the run*(...) methods must wrap everything that is intended to exist within the context. This is helpful for scenarios such as prepending a 'connection' event handler to an http server which binds everything that occurs within each request to the given context. This is helpful for APMs to minimize the need for patching and especially adding closures.

Depends on #31930

cc @vdeturckheim @puzpuzpuz @Flarna

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@Qard Qard requested review from mcollina and vdeturckheim Feb 25, 2020
@Qard Qard self-assigned this Feb 25, 2020
@Qard Qard force-pushed the Qard:async-storage-start branch 2 times, most recently Feb 25, 2020
Copy link
Member

vdeturckheim left a comment

I am ambivalent on this as it can introduce hard-to-understand behaviors. So if we land this, we need to be extra-clear regareding doc.

doc/api/async_hooks.md Show resolved Hide resolved
doc/api/async_hooks.md Show resolved Hide resolved
@Qard

This comment has been minimized.

Copy link
Member Author

Qard commented Feb 25, 2020

@vdeturckheim I disagree that the behaviour of this is any more "hard to understand" than the run*(...) methods. It's just applying the same behaviour at a different time. The run methods, while currently implemented like this:

run(store, callback, ...args) {
  this._enter(store);
  process.nextTick(callback, ...args);
  this._exit();
}

Are functionally identical to this:

run(store, callback, ...args) {
  process.nextTick(() => {
    this._enter(store);
    callback(...args);
  });
}

In a related note, I pointed out in my AsyncLocal PR that "exit" events are actually redundant, and this is why. An enter/exit is actually just a single state transition. There's never actually a case where the context would need to exit without explicit user intervention when they specifically want to escape the context for something such as preventing a connection pool from associating with the wrong thing. The sync version could just as easily simply be an AsyncResource and it'd work exactly the same way, needing no exit behaviour.

With the run*(...) methods, it's currently saying "in the next tick, bind all execution and async descendants to this context object" while the enterWith(store) method is just saying "from now on, bind all execution and async descendants to this context object" which is easier to reason about from an access perspective. The run*(...) methods have the additional cognitive burden of understanding that the store is no longer accessible after the callback executes whereas with enterWith(store) it's just there, they don't need to do anything else to ensure their other code can reach the store.

@vdeturckheim

This comment has been minimized.

Copy link
Member

vdeturckheim commented Feb 25, 2020

@Qard I remeber you pointed out that such method were actually problematic in domain when it was suggested we exposed enterSync. What would be the main difference between then and enterWith (appart from passing the store as argument)?

@Qard

This comment has been minimized.

Copy link
Member Author

Qard commented Feb 25, 2020

It's not enter that is the problem, it's exit. Misuse of an exit method can result in context loss.

In the case of domains, they also had the problem that domains are bound globally so one could do process.domain.enter() and process.domain.exit() at any time and totally mess with the expectations of whatever actually created that domain. In this case, an async local storage is not globally bound so the only things interacting with it will be the things specifically given access to it, which makes it a more controlled environment and therefore can better trust that the state transitions applied to it are valid.

@vdeturckheim

This comment has been minimized.

Copy link
Member

vdeturckheim commented Feb 25, 2020

I get your point regarding globally exposed objects or not, I still think there will be shared instances of AsyncLocalStorage that would, in fine, be the same thing as exposing it globally.

The domain post-mortem highlights an issue only based on domain.enter() as doing:

d.enter()
const dep = require('some-dep');
dep.method(); // Uh-oh! This method doesn't actually exist.

is problematic if the module some-dep might return a partially populated export. Even if scoping does not remove the issue totally, it still reduces it's impact (actually, the run(...) method is even safer from this point)
This API will be used for error management for sure.

That's what I understood from

I'm not a fan of exposing enterSync() and exitSync(). That same pattern created a ton of bugs with domains and I suspect the same would be the case here. 🤔

@Qard

This comment has been minimized.

Copy link
Member Author

Qard commented Feb 25, 2020

That's because domains take a multi-tenant environment and reduce it to a single-tenant error processor. Entering one domain should not exit another, a domain should only care about its own state transitions. The model should be redesigned so multiple domains can listen to errors from the same code and just have a top-level domain that handles the "default" behaviour of the "uncaughtException" event. Whenever other domains beyond the top-level one are created, the top-level one could just turn its handler off. 🤔

@vdeturckheim vdeturckheim dismissed their stale review Feb 25, 2020

Doc changes LGTM.

@Qard Qard added the blocked label Feb 26, 2020
@Qard Qard force-pushed the Qard:async-storage-start branch Feb 28, 2020
@Qard Qard removed the blocked label Feb 28, 2020
@Qard

This comment has been minimized.

Copy link
Member Author

Qard commented Feb 28, 2020

Rebased as #31930 has landed now.

@puzpuzpuz

This comment has been minimized.

Copy link
Contributor

puzpuzpuz commented Feb 29, 2020

My 2 cents: if no one comes with a better name (#31945 (comment)), enterWith should be at least renamed into enter, as we don't have With postfix in run* methods which also accept the store.

Update. A crazy idea: we could rename exit* into something like escape* or runExited* and the naming conflict would be solved.

@Qard

This comment has been minimized.

Copy link
Member Author

Qard commented Mar 1, 2020

I'm fine with enter(...) rather than enterWith(...), if that's the preferred name. It'd be nice if there was a name that didn't suggest that a corresponding exit/end is needed though. I liked that about the AsyncLocal design--it just had a set(...) method to store the context.

As for the escape(...) naming, I'm good with that too. It feels a little more clearly disconnected from enter(...) with that naming. 🤔

@vdeturckheim

This comment has been minimized.

Copy link
Member

vdeturckheim commented Mar 1, 2020

The name must be about starting propagation. Focusing on the nature of the store instead of the started mechanism would be a logical mistake: a store holding mechanism should not spread a payload differently based on its nature.

As for this PR reducing the need for patching in an APM/RASP context. It seems to me that implementer would still need to patch the http.Server class to place a first listener on the 'request' event for each new instances. Is that right? In that case, I'd be globally -1 in having this for now.

@Qard

This comment has been minimized.

Copy link
Member Author

Qard commented Mar 1, 2020

Agreed that it should communicates that it's "starting" something, which is why I've stated that I don't have a strong feeling/intuition of what exactly the naming should be, just that I'm a little bit uncomfortable with the current enter/exit naming.

As for needing to patch http.Server: no, only the createServer method would need to be patched, the http.Server class itself can remain unpatched. Also, it makes it a lot easier for a user to manually set up the instrumentation by just doing something like apm.watchHTTPServer(server) rather than needing to patch the entire module system and http modules just to make a basic trace.

@Flarna

This comment has been minimized.

Copy link
Member

Flarna commented Mar 1, 2020

It would be quite simple to add a listener in http module to notify any tool that a new server has been created to allow to add it's request handler completly without patching.
I thought loud about this in nodejs/quic#201 for quic but in the end I got stuck with context passing. With having the context on the resource it looks quite promising.

The big question is where to start: Allow such usecases in AsyncLocalStore and than add hooks in http/... or first add the hooks there and only if there is no gap left we enhance AsyncLocalStore.

@puzpuzpuz

This comment has been minimized.

Copy link
Contributor

puzpuzpuz commented Mar 1, 2020

Agreed that it should communicates that it's "starting" something, which is why I've stated that I don't have a strong feeling/intuition of what exactly the naming should be, just that I'm a little bit uncomfortable with the current enter/exit naming.

Exactly. That's why I propose to think a bit more and try to find better names for those methods.

Another option that I came up with. Keep enterWith method name and do these renames: run -> runWith, exit -> runWithout

@Qard

This comment has been minimized.

Copy link
Member Author

Qard commented Mar 2, 2020

What about enterWithContext and runWithContext along with escapeContext?

@puzpuzpuz

This comment has been minimized.

Copy link
Contributor

puzpuzpuz commented Mar 2, 2020

What about enterWithContext and runWithContext along with escapeContext?

Sounds like a more intuitive naming than the current one to me.

@Qard

This comment has been minimized.

Copy link
Member Author

Qard commented Mar 2, 2020

Or maybe dropping the “with” part, to be not too terribly verbose. 🤔

@Qard Qard force-pushed the Qard:async-storage-start branch to 6960bb7 Mar 9, 2020
Copy link
Member

mhdawson left a comment

LGTM

@mhdawson

This comment has been minimized.

Copy link
Member

mhdawson commented Mar 9, 2020

Looks to me like everything was ok and somehow a second travis build was created that seems stalled. Going to land.

@mhdawson

This comment has been minimized.

Copy link
Member

mhdawson commented Mar 9, 2020

Hmm, seems like the CI run is stale.

New CI Run: https://ci.nodejs.org/job/node-test-pull-request/29666/

@nodejs-github-bot

This comment has been minimized.

@mhdawson

This comment has been minimized.

Copy link
Member

mhdawson commented Mar 9, 2020

Ok new CI run is good landing.

mhdawson added a commit that referenced this pull request Mar 9, 2020
This allows transitioning the entire following sync and async execution
sub-tree to the given async storage context. With this one can be sure
the context binding will remain for any following sync activity and all
descending async execution whereas the `run*(...)` methods must wrap
everything that is intended to exist within the context. This is helpful
for scenarios such as prepending a `'connection'` event to an http
server which binds everything that occurs within each request to
the given context. This is helpful for APMs to minimize the need
for patching and especially adding closures.

PR-URL: #31945
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@mhdawson

This comment has been minimized.

Copy link
Member

mhdawson commented Mar 9, 2020

Landed in d368dcc

@mhdawson mhdawson closed this Mar 9, 2020
MylesBorins added a commit that referenced this pull request Mar 9, 2020
This allows transitioning the entire following sync and async execution
sub-tree to the given async storage context. With this one can be sure
the context binding will remain for any following sync activity and all
descending async execution whereas the `run*(...)` methods must wrap
everything that is intended to exist within the context. This is helpful
for scenarios such as prepending a `'connection'` event to an http
server which binds everything that occurs within each request to
the given context. This is helpful for APMs to minimize the need
for patching and especially adding closures.

PR-URL: #31945
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2020
MylesBorins added a commit that referenced this pull request Mar 10, 2020
Notable changes:

* [[`ff58854dbe`](ff58854)] - **(SEMVER-MINOR)** **fs**: return first folder made by mkdir recursive (Benjamin Coe) [#31530](#31530)
* [[`258a80d3cc`](258a80d)] - **(SEMVER-MINOR)** **src**: create a getter for kernel version (Juan José Arboleda) [#31732](#31732)
* [[`4d5981be96`](4d5981b)] - **(SEMVER-MINOR)** **async_hooks**: add sync enterWith to ALS (Stephen Belanger) [#31945](#31945)
* [[`dd83bd266d`](dd83bd2)] - **(SEMVER-MINOR)** **wasi**: add returnOnExit option (Colin Ihrig) [#32101](#32101)

PR-URL: #32185
MylesBorins added a commit that referenced this pull request Mar 11, 2020
Notable changes:

* async_hooks:
  - add sync enterWith to ALS (Stephen Belanger)
    #31945
* cli:
  - allow --jitless V8 flag in NODE\_OPTIONS (Andrew Neitsch)
    #32100
* fs:
  - return first folder made by mkdir recursive (Benjamin Coe)
    #31530
* n-api:
  - define release 6 (Gabriel Schulhof)
    #32058
* src:
  - create a getter for kernel version (Juan José Arboleda)
    #31732
* wasi:
  - add returnOnExit option (Colin Ihrig)
    #32101

PR-URL: #32185
MylesBorins added a commit that referenced this pull request Mar 11, 2020
Notable changes:

* async_hooks:
  - add sync enterWith to ALS (Stephen Belanger)
    #31945
* cli:
  - allow --jitless V8 flag in NODE\_OPTIONS (Andrew Neitsch)
    #32100
* fs:
  - return first folder made by mkdir recursive (Benjamin Coe)
    #31530
* n-api:
  - define release 6 (Gabriel Schulhof)
    #32058
* src:
  - create a getter for kernel version (Juan José Arboleda)
    #31732
* wasi:
  - add returnOnExit option (Colin Ihrig)
    #32101

PR-URL: #32185
MylesBorins added a commit that referenced this pull request Mar 12, 2020
Notable changes:

* async_hooks:
  - add sync enterWith to ALS (Stephen Belanger)
    #31945
* cli:
  - allow --jitless V8 flag in NODE\_OPTIONS (Andrew Neitsch)
    #32100
* fs:
  - return first folder made by mkdir recursive (Benjamin Coe)
    #31530
* n-api:
  - define release 6 (Gabriel Schulhof)
    #32058
* src:
  - create a getter for kernel version (Juan José Arboleda)
    #31732
* wasi:
  - add returnOnExit option (Colin Ihrig)
    #32101

PR-URL: #32185
MylesBorins added a commit that referenced this pull request Mar 12, 2020
Notable changes:

* async_hooks:
  - add sync enterWith to ALS (Stephen Belanger)
    #31945
* cli:
  - allow --jitless V8 flag in NODE\_OPTIONS (Andrew Neitsch)
    #32100
* fs:
  - return first folder made by mkdir recursive (Benjamin Coe)
    #31530
* n-api:
  - define release 6 (Gabriel Schulhof)
    #32058
* src:
  - create a getter for kernel version (Juan José Arboleda)
    #31732
* wasi:
  - add returnOnExit option (Colin Ihrig)
    #32101

PR-URL: #32185
@deluxor deluxor mentioned this pull request Mar 16, 2020
puzpuzpuz added a commit to puzpuzpuz/node that referenced this pull request Mar 17, 2020
This allows transitioning the entire following sync and async execution
sub-tree to the given async storage context. With this one can be sure
the context binding will remain for any following sync activity and all
descending async execution whereas the `run*(...)` methods must wrap
everything that is intended to exist within the context. This is helpful
for scenarios such as prepending a `'connection'` event to an http
server which binds everything that occurs within each request to
the given context. This is helpful for APMs to minimize the need
for patching and especially adding closures.

PR-URL: nodejs#31945
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@puzpuzpuz

This comment has been minimized.

Copy link
Contributor

puzpuzpuz commented Mar 17, 2020

v12 backport: #32318

puzpuzpuz added a commit to puzpuzpuz/node that referenced this pull request Apr 2, 2020
This allows transitioning the entire following sync and async execution
sub-tree to the given async storage context. With this one can be sure
the context binding will remain for any following sync activity and all
descending async execution whereas the `run*(...)` methods must wrap
everything that is intended to exist within the context. This is helpful
for scenarios such as prepending a `'connection'` event to an http
server which binds everything that occurs within each request to
the given context. This is helpful for APMs to minimize the need
for patching and especially adding closures.

PR-URL: nodejs#31945
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.