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

events: allow safely adding listener to abortSignal #48596

Conversation

atlowChemi
Copy link
Member

@atlowChemi atlowChemi commented Jun 29, 2023

I am not sure this is the correct way to expose such a util, and I still need to add docs and UT, just wanted to get some feedback first 🙂

Refs: #48550 (comment)

@mcollina @benjamingr CC

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Jun 29, 2023
@atlowChemi atlowChemi force-pushed the add-userLand-method-for-safely-adding-listener-to-abortSignal branch from 416dafb to d9f2b58 Compare June 29, 2023 10:27
lib/util.js Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Can you please add a unit test?

@atlowChemi
Copy link
Member Author

Can you please add a unit test?

I wrote in the PRs description I want to do that after getting some feedback about the idea 🙈😅

@atlowChemi atlowChemi force-pushed the add-userLand-method-for-safely-adding-listener-to-abortSignal branch from d9f2b58 to 0e45c8f Compare June 29, 2023 17:06
@benjamingr
Copy link
Member

I need to think about this, but generally good. also cc @ronag

lib/util.js Outdated
signal.addEventListener(
'abort',
listener,
{ __proto__: null, ...options, once: true, [kResistStopPropagation]: true },
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 think it makes sense to enforce the once option for the abort event, but I'd be happy to know if anyone thinks otherwise I'm happy to change 🙂

Copy link
Member

Choose a reason for hiding this comment

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

once: true makes sense for me here (and also not accepting options).

I'm wondering if we should do

if (signal.aborted) {
  queueMicrotask(() => listener())` 
}

(and either way the alternative is to at least check for signal.aborted since the behavior right now would be to add the listener and not do cleanup)

Copy link
Member Author

@atlowChemi atlowChemi Jun 29, 2023

Choose a reason for hiding this comment

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

I think I approached this with a different mindset than you 🙂

I was thinking of exposing a "safe" version of addEventListener.
I was thinking we could also use this internally for all places currently using kResistStopPropagation (even when they use kWeakHandler etc)

Since I was thinking of a safe implementation, I wanted it to behave quite the same, hence I accepted the options and did not check for aborted prior to adding the listener.

That said, I am happy to change this to work as you were expecting for it to work 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I re-implemented, let me know what you think 🙂

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of exposing a "safe" version of addEventListener.

Forgetting to cleanup if the signal is already aborted is a common bug and if we can help users with it that's good.

We can still use kWeakRef I believe? The idea of kWeakListener (or whatever we named it) is to prevent cases where the user forgets to remove the listener and the listener keeps a live reference to the object. I'm even less sure if we should expose it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I was not talking of exposing it, just about allowing us to use it via this utility, in internal calls

doc/api/util.md Outdated
@@ -14,6 +14,60 @@ it:
const util = require('node:util');
```

## `util.addSafeAbortSignalAbortListener(signal, resource)`
Copy link
Member

Choose a reason for hiding this comment

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

Bikeshed: can we get a shorter name? util.addAbortListener

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I was not happy with the name I gave, but was not sure what would be a good name 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to your suggestion

doc/api/util.md Outdated Show resolved Hide resolved
doc/api/util.md Outdated Show resolved Hide resolved
@atlowChemi atlowChemi requested a review from mcollina June 29, 2023 17:11
doc/api/util.md Outdated Show resolved Hide resolved
lib/util.js Outdated Show resolved Hide resolved
lib/util.js Outdated Show resolved Hide resolved
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Generally looks good, left some comments, need to think about this.

@mcollina
Copy link
Member

Can you please add a unit test?

I wrote in the PRs description I want to do that after getting some feedback about the idea 🙈😅

It's actually hard to reason about new functionality without seeing an example and the best form of examples are tests.

@atlowChemi
Copy link
Member Author

Can you please add a unit test?

I wrote in the PRs description I want to do that after getting some feedback about the idea 🙈😅

It's actually hard to reason about new functionality without seeing an example and the best form of examples are tests.

Makes sense, will keep that in mind 👍🏽
Anyway, I added tests 🙂

doc/api/util.md Outdated
// Do something when signal is aborted.
});
} finally {
disposable?.[Symbol.dispose]();
Copy link
Member

Choose a reason for hiding this comment

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

Are we planning to backport Symbol.dispose to v18.x?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes, hopefully, why?

People building transpiled code and target v18.x should still benefit from it.

Copy link
Member

Choose a reason for hiding this comment

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

This change depends on that.

doc/api/util.md Outdated
**Default:** `false`.
* `signal` {AbortSignal} The listener will be removed when the given
AbortSignal object's `abort()` method is called.
* Returns: {Disposable} that removes the `abort` listener.
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to return an object that is currently not standardized yet? Can we return something that we can control?

Copy link
Member

Choose a reason for hiding this comment

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

Can we return something that we can control?

I'm also not sure how the docs should look and I'm not sure "disposable" is the name (is it?) but it's basically "something that has a Symbol.dispose".

Unlike AbortSignal/EventTarget - disposables are just a function call - so we control this.

Copy link
Member Author

@atlowChemi atlowChemi Jun 30, 2023

Choose a reason for hiding this comment

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

@atlowChemi atlowChemi force-pushed the add-userLand-method-for-safely-adding-listener-to-abortSignal branch from 0e45c8f to d8c8109 Compare June 29, 2023 19:47
@ronag
Copy link
Member

ronag commented Jun 30, 2023

If we are doing this maybe we might as well expose the weak listener option as well?

@benjamingr
Copy link
Member

If we are doing this maybe we might as well expose the weak listener option as well?

That can be a parameter so maybe:

// Semantics are: 
// when the signal aborts _or_ it's already aborted - close the file handle
// if the file handle goes out of scope - remove the listener
using addAbortListener(signal, () => fileHandle.close(), fileHandle);

Do you think in terms oof footgun-ness to usefulness it's good enough to expose?

@atlowChemi atlowChemi marked this pull request as ready for review June 30, 2023 13:42
@atlowChemi atlowChemi force-pushed the add-userLand-method-for-safely-adding-listener-to-abortSignal branch 2 times, most recently from 9717ae4 to b708f4d Compare July 1, 2023 18:34
Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

I feel like adding this to node:events might be a better fit, besides that LGTM

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@atlowChemi
Copy link
Member Author

I feel like adding this to node:events might be a better fit, besides that LGTM

That makes sense to me, if anyone involved doesn't object I'll move it to node:events 🙂
@benjamingr @mcollina @ronag cc

nodejs-github-bot pushed a commit that referenced this pull request Mar 16, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Mar 16, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Mar 16, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
Refs: nodejs#48596
PR-URL: nodejs#52081
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
PR-URL: nodejs#52081
Refs: nodejs#48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
PR-URL: nodejs#52081
Refs: nodejs#48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
PR-URL: nodejs#52081
Refs: nodejs#48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
PR-URL: nodejs#52081
Refs: nodejs#48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
PR-URL: nodejs#52081
Refs: nodejs#48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
PR-URL: nodejs#52081
Refs: nodejs#48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
PR-URL: nodejs#52081
Refs: nodejs#48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
PR-URL: nodejs#52081
Refs: nodejs#48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
Refs: #48596
PR-URL: #52081
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
Refs: #48596
PR-URL: #52081
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. events Issues and PRs related to the events subsystem / EventEmitter. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. 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.

6 participants