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: rename AsyncLocalStore#enterWith to set #45373

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions benchmark/diagnostics_channel/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ function patch() {
function wrappedEmit(...args) {
const [name, req, res] = args;
if (name === 'request') {
als.enterWith({
als.set({
url: req.url,
start: process.hrtime.bigint()
});
Expand Down Expand Up @@ -72,7 +72,7 @@ function diagnostics_channel() {
const finish = dc.channel('http.server.response.finish');

function onStart(req) {
als.enterWith({
als.set({
url: req.url,
start: process.hrtime.bigint()
});
Expand Down
28 changes: 21 additions & 7 deletions doc/api/async_context.md
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ added:
-->

Creates a new instance of `AsyncLocalStorage`. Store is only provided within a
`run()` call or after an `enterWith()` call.
`run()` call or after an `set()` call.

### `asyncLocalStorage.disable()`

Expand All @@ -139,7 +139,7 @@ added:

Disables the instance of `AsyncLocalStorage`. All subsequent calls
to `asyncLocalStorage.getStore()` will return `undefined` until
`asyncLocalStorage.run()` or `asyncLocalStorage.enterWith()` is called again.
`asyncLocalStorage.run()` or `asyncLocalStorage.set()` is called again.

When calling `asyncLocalStorage.disable()`, all current contexts linked to the
instance will be exited.
Expand All @@ -164,7 +164,7 @@ added:

Returns the current store.
If called outside of an asynchronous context initialized by
calling `asyncLocalStorage.run()` or `asyncLocalStorage.enterWith()`, it
calling `asyncLocalStorage.run()` or `asyncLocalStorage.set()`, it
returns `undefined`.

### `asyncLocalStorage.enterWith(store)`
Expand All @@ -173,13 +173,26 @@ returns `undefined`.
added:
- v13.11.0
- v12.17.0
deprecated:
- REPLACEME
-->

> Stability: 0 - Deprecated: Use [`asyncLocalStorage.set(store)`][]

This is a deprecated alias for [`asyncLocalStorage.set(store)`][].

### `asyncLocalStorage.set(store)`

<!-- YAML
added:
- REPLACEME
-->

> Stability: 1 - Experimental

* `store` {any}

Transitions into the context for the remainder of the current
Sets `store` on current execution context for the remainder of the current
synchronous execution and then persists the store through any following
asynchronous calls.

Expand All @@ -188,7 +201,7 @@ Example:
```js
const store = { id: 1 };
// Replaces previous store with the given store object
asyncLocalStorage.enterWith(store);
asyncLocalStorage.set(store);
asyncLocalStorage.getStore(); // Returns the store object
someAsyncOperation(() => {
asyncLocalStorage.getStore(); // Returns the same object
Expand All @@ -199,14 +212,14 @@ This transition will continue for the _entire_ synchronous execution.
This means that if, for example, the context is entered within an event
handler subsequent event handlers will also run within that context unless
specifically bound to another context with an `AsyncResource`. That is why
`run()` should be preferred over `enterWith()` unless there are strong reasons
`run()` should be preferred over `set()` unless there are strong reasons
to use the latter method.

```js
const store = { id: 1 };

emitter.on('my-event', () => {
asyncLocalStorage.enterWith(store);
asyncLocalStorage.set(store);
});
emitter.on('my-event', () => {
asyncLocalStorage.getStore(); // Returns the same object
Expand Down Expand Up @@ -816,4 +829,5 @@ const server = createServer((req, res) => {
[`EventEmitter`]: events.md#class-eventemitter
[`Stream`]: stream.md#stream
[`Worker`]: worker_threads.md#class-worker
[`asyncLocalStorage.set(store)`]: #asynclocalstoragesetstore
[`util.promisify()`]: util.md#utilpromisifyoriginal
5 changes: 4 additions & 1 deletion lib/async_hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ class AsyncLocalStorage {
}
}

enterWith(store) {
set(store) {
this._enable();
const resource = executionAsyncResource();
resource[this.kResourceStore] = store;
Expand Down Expand Up @@ -353,6 +353,9 @@ class AsyncLocalStorage {
}
}

// Create deprecated enterWith alias
AsyncLocalStorage.prototype.enterWith = AsyncLocalStorage.prototype.set;
Copy link
Member

Choose a reason for hiding this comment

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

Can you emit warning when enterWith is called?

Copy link
Member

Choose a reason for hiding this comment

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

Util.deprecate should do the trick

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a reason for not using a doc only deprecation here - at least for a while? Deprecation logs may break apps using the console output.

It's just a about an inoptimum naming not a functional problem which results in some risk/problem.

Copy link
Member

Choose a reason for hiding this comment

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

I'd argue we want the deprecation logs to show for a few versions before we remove enterWith . Alternativelly, we can keep enterWith and doc it as an alias for set, but if we want it out, we need to make sure we never break code with . enterWith is not a function

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with keeping it as documented alias like I did it here. My main intention is that looking into real world use cases as in the above linked PR the name looks better to me and results in easier to read code.

But enterWith is not that bad that I would force people to change. can't tell how many users there are actually.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe an advantage of enterWith is that it's "unique" and we can easily search for its usage in GitHub and other open source databases.

Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in the main thread, I changed my mind, I think we should keep enterWith as a forever alias due to its use in existing APMs and we don't want to break any of their customers on whatever version of node x version of the APM.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is used extensively at Datadog currently so I definitely would not want to go straight to warning deprecation. We should have a doc deprecation phase, an eventual warning phase, and then maybe removal.

As I stated further up in the issue though, I'm working on making this API (hopefully) obsolete so not sure changes the this will really matter much.


// Placing all exports down here because the exported classes won't export
// otherwise.
module.exports = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ const { AsyncLocalStorage } = require('async_hooks');

const asyncLocalStorage = new AsyncLocalStorage();

assert.strictEqual(AsyncLocalStorage.prototype.enterWith, AsyncLocalStorage.prototype.set);

setImmediate(() => {
const store = { foo: 'bar' };
asyncLocalStorage.enterWith(store);
asyncLocalStorage.set(store);

assert.strictEqual(asyncLocalStorage.getStore(), store);
setTimeout(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ let context;

// Bind requests to an AsyncLocalStorage context
dc.subscribe('http.server.request.start', common.mustCall((message) => {
als.enterWith(message);
als.set(message);
context = message;
}));

Expand Down