-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
events: allow safely adding listener to abortSignal #48596
Conversation
416dafb
to
d9f2b58
Compare
There was a problem hiding this 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?
I wrote in the PRs description I want to do that after getting some feedback about the idea 🙈😅 |
d9f2b58
to
0e45c8f
Compare
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 }, |
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)` |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to your suggestion
There was a problem hiding this 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.
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 👍🏽 |
doc/api/util.md
Outdated
// Do something when signal is aborted. | ||
}); | ||
} finally { | ||
disposable?.[Symbol.dispose](); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
0e45c8f
to
d8c8109
Compare
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? |
9717ae4
to
b708f4d
Compare
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
That makes sense to me, if anyone involved doesn't object I'll move it to |
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>
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>
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>
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>
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>
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>
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>
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>
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