events: allow safely adding listener to abortSignal by atlowChemi · Pull Request #48596 · nodejs/node

atlowChemi · 2023-06-29T10:24:01Z

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

lib/util.js

mcollina

Can you please add a unit test?

atlowChemi · 2023-06-29T12:23:45Z

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 🙈😅

benjamingr · 2023-06-29T17:08:40Z

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

atlowChemi · 2023-06-29T17:09:07Z

lib/util.js

+  signal.addEventListener(
+    'abort',
+    listener,
+    { __proto__: null, ...options, once: true, [kResistStopPropagation]: true },


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 🙂

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)

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 🙂

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

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.

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

benjamingr · 2023-06-29T17:09:33Z

doc/api/util.md

 const util = require('node:util');
 ```

+## `util.addSafeAbortSignalAbortListener(signal, resource)`


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

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

Changed to your suggestion

doc/api/util.md

lib/util.js

benjamingr

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

mcollina · 2023-06-29T18:23:46Z

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 · 2023-06-29T19:15:13Z

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 🙂

mcollina · 2023-06-29T19:24:11Z

doc/api/util.md

+      // Do something when signal is aborted.
+    });
+  } finally {
+    disposable?.[Symbol.dispose]();


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

@benjamingr @MoLow? 🙂

Yes, hopefully, why?

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

This change depends on that.

mcollina · 2023-06-29T19:25:35Z

doc/api/util.md

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


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

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.

@benjamingr that is the name the spec calls it:
https://github.com/tc39/proposal-explicit-resource-management/blob/3b576c0bb756fcbb4b91c5556c3609806f9abaf9/README.md?plain=1#L1164-L1180

ronag · 2023-06-30T06:47:42Z

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

benjamingr · 2023-06-30T07:31:15Z

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?

MoLow

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

mcollina

lgtm

atlowChemi · 2023-07-03T07:39:06Z

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

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>

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>

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>

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>

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>

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>

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 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 force-pushed the add-userLand-method-for-safely-adding-listener-to-abortSignal branch from 416dafb to d9f2b58 Compare June 29, 2023 10:27

atlowChemi commented Jun 29, 2023

View reviewed changes

lib/util.js Outdated Show resolved Hide resolved

mcollina requested changes Jun 29, 2023

View reviewed changes

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

atlowChemi commented Jun 29, 2023

View reviewed changes