GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — events: allow safely adding listener to abortSignal by atlowChemi · Pull Request #48596 · nodejs/node

Conversation

@atlowChemi

@atlowChemi atlowChemi commented Jun 29, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss and metcoder95 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@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
atlowChemi
atlowChemi commented Jun 29, 2023
View reviewed changes
Comment thread lib/util.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

mcollina
mcollina requested changes Jun 29, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@atlowChemi

atlowChemi commented Jun 29, 2023

Copy link
Copy Markdown
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 🙈😅

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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

benjamingr commented Jun 29, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

atlowChemi
atlowChemi commented Jun 29, 2023
View reviewed changes
Comment thread lib/util.js Outdated

@atlowChemi atlowChemi Jun 29, 2023

Copy link
Copy Markdown
Member Author

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 🙂

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@benjamingr benjamingr Jun 29, 2023

Copy link
Copy Markdown
Member

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MoLow reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@atlowChemi atlowChemi Jun 29, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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 🙂

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@atlowChemi atlowChemi Jun 29, 2023

Copy link
Copy Markdown
Member Author

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 🙂

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@benjamingr benjamingr Jun 29, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@atlowChemi atlowChemi Jun 30, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benjamingr reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
benjamingr
benjamingr reviewed Jun 29, 2023
View reviewed changes
Comment thread doc/api/util.md Outdated

@benjamingr benjamingr Jun 29, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@atlowChemi atlowChemi Jun 29, 2023

Copy link
Copy Markdown
Member Author

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 🙂

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@atlowChemi atlowChemi Jun 29, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
atlowChemi
atlowChemi commented Jun 29, 2023
View reviewed changes
Comment thread doc/api/util.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

atlowChemi
atlowChemi commented Jun 29, 2023
View reviewed changes
Comment thread doc/api/util.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

@atlowChemi
atlowChemi requested a review from mcollina June 29, 2023 17:11
benjamingr
benjamingr reviewed Jun 29, 2023
View reviewed changes
Comment thread doc/api/util.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr reviewed Jun 29, 2023
View reviewed changes
Comment thread lib/util.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr reviewed Jun 29, 2023
View reviewed changes
Comment thread lib/util.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr reviewed Jun 29, 2023
View reviewed changes

@benjamingr benjamingr left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

atlowChemi reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@mcollina

mcollina commented Jun 29, 2023

Copy link
Copy Markdown
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 and MoLow reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@atlowChemi

atlowChemi commented Jun 29, 2023

Copy link
Copy Markdown
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 🙂

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina
mcollina reviewed Jun 29, 2023
View reviewed changes
Comment thread doc/api/util.md Outdated

@mcollina mcollina Jun 29, 2023

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@atlowChemi atlowChemi Jun 29, 2023

Copy link
Copy Markdown
Member Author

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 @MoLow? 🙂

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@benjamingr benjamingr Jun 30, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

atlowChemi reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@mcollina mcollina Jul 3, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
mcollina
mcollina requested changes Jun 29, 2023
View reviewed changes
Comment thread doc/api/util.md Outdated

@mcollina mcollina Jun 29, 2023

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@benjamingr benjamingr Jun 30, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

atlowChemi reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@atlowChemi atlowChemi Jun 30, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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
image

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benjamingr reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@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

ronag commented Jun 30, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@benjamingr

benjamingr commented Jun 30, 2023

Copy link
Copy Markdown
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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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
MoLow
MoLow approved these changes Jul 2, 2023
View reviewed changes

@MoLow MoLow left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
mcollina
mcollina approved these changes Jul 3, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@atlowChemi

atlowChemi commented Jul 3, 2023

Copy link
Copy Markdown
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

benjamingr, MoLow, mcollina, and ronag reacted with thumbs up emoji lin72h reacted with eyes emoji
All reactions
  • 👍 4 reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

juanarbol added a commit that referenced this pull request Jul 20, 2023
@juanarbol
2023-07-18, Version 20.5.0 (Current)
d5761a4
Notable changes:

doc:
  * add atlowChemi to collaborators (atlowChemi) #48757
events:
  * (SEMVER-MINOR) allow safely adding listener to abortSignal (Chemi Atlow) #48596
fs:
  * add a fast-path for readFileSync utf-8 (Yagiz Nizipli) #48658
test_runner:
  * (SEMVER-MINOR) add shards support (Raz Luvaton) #48639

PR-URL: #48761
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@atlowChemi @Ceres6
events: allow safely adding listener to abortSignal
28b7a74
PR-URL: nodejs#48596
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@juanarbol @Ceres6
2023-07-18, Version 20.5.0 (Current)
4c6cd1d
Notable changes:

doc:
  * add atlowChemi to collaborators (atlowChemi) nodejs#48757
events:
  * (SEMVER-MINOR) allow safely adding listener to abortSignal (Chemi Atlow) nodejs#48596
fs:
  * add a fast-path for readFileSync utf-8 (Yagiz Nizipli) nodejs#48658
test_runner:
  * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs#48639

PR-URL: nodejs#48761
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@atlowChemi @Ceres6
events: allow safely adding listener to abortSignal
41b5955
PR-URL: nodejs#48596
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@juanarbol @Ceres6
2023-07-18, Version 20.5.0 (Current)
067c4ce
Notable changes:

doc:
  * add atlowChemi to collaborators (atlowChemi) nodejs#48757
events:
  * (SEMVER-MINOR) allow safely adding listener to abortSignal (Chemi Atlow) nodejs#48596
fs:
  * add a fast-path for readFileSync utf-8 (Yagiz Nizipli) nodejs#48658
test_runner:
  * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs#48639

PR-URL: nodejs#48761
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
@atlowChemi @ruyadorno
events: allow safely adding listener to abortSignal
1c58ee7
PR-URL: #48596
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
ruyadorno added a commit that referenced this pull request Sep 11, 2023
@ruyadorno
2023-09-14, Version 18.18.0 'Hydrogen' (LTS)
1186fdb
Notable changes:

crypto:
  * update root certificates to NSS 3.90 (Node.js GitHub Bot) #48416
doc:
  * add vmoroz to collaborators (Vladimir Morozov) #48527
  * add kvakil to collaborators (Keyhan Vakil) #48449
esm:
  * (SEMVER-MINOR) add `--import` flag (Moshe Atlow) #43942
events:
  * (SEMVER-MINOR) allow safely adding listener to abortSignal (Chemi Atlow) #48596
net:
  * add autoSelectFamily global getter and setter (Paolo Insogna) #45777
url:
  * (SEMVER-MINOR) add value argument to has and delete methods (Sankalp Shubham) #47885

PR-URL: #49220
@ruyadorno ruyadorno mentioned this pull request Sep 11, 2023
v18.18.0 proposal #49220
Merged
ruyadorno added a commit that referenced this pull request Sep 11, 2023
@ruyadorno
2023-09-14, Version 18.18.0 'Hydrogen' (LTS)
7c96325
Notable changes:

build:
  * sync libuv header change (Jiawen Geng) #48078
crypto:
  * update root certificates to NSS 3.90 (Node.js GitHub Bot) #48416
deps:
  * add missing thread-common.c in uv.gyp (Santiago Gimeno) #48078
  * upgrade to libuv 1.46.0 (Santiago Gimeno) #48078
  * upgrade to libuv 1.45.0 (Santiago Gimeno) #48078
doc:
  * add vmoroz to collaborators (Vladimir Morozov) #48527
  * add kvakil to collaborators (Keyhan Vakil) #48449
esm:
  * (SEMVER-MINOR) add `--import` flag (Moshe Atlow) #43942
events:
  * (SEMVER-MINOR) allow safely adding listener to abortSignal (Chemi Atlow) #48596
fs, stream:
  * initial `Symbol.dispose` and `Symbol.asyncDispose` support (Moshe Atlow) #48518
net:
  * add autoSelectFamily global getter and setter (Paolo Insogna) #45777
url:
  * (SEMVER-MINOR) add value argument to has and delete methods (Sankalp Shubham) #47885

PR-URL: #49220
ruyadorno added a commit that referenced this pull request Sep 12, 2023
@ruyadorno
2023-09-14, Version 18.18.0 'Hydrogen' (LTS)
89e7fd1
Notable changes:

build:
  * sync libuv header change (Jiawen Geng) #48078
crypto:
  * update root certificates to NSS 3.93 (Node.js GitHub Bot) #49341
  * update root certificates to NSS 3.90 (Node.js GitHub Bot) #48416
deps:
  * add missing thread-common.c in uv.gyp (Santiago Gimeno) #48078
  * upgrade to libuv 1.46.0 (Santiago Gimeno) #48078
  * upgrade to libuv 1.45.0 (Santiago Gimeno) #48078
doc:
  * add atlowChemi to collaborators (atlowChemi) #48757
  * add vmoroz to collaborators (Vladimir Morozov) #48527
  * add kvakil to collaborators (Keyhan Vakil) #48449
esm:
  * (SEMVER-MINOR) add `--import` flag (Moshe Atlow) #43942
events:
  * (SEMVER-MINOR) allow safely adding listener to abortSignal (Chemi Atlow) #48596
fs, stream:
  * initial `Symbol.dispose` and `Symbol.asyncDispose` support (Moshe Atlow) #48518
net:
  * add autoSelectFamily global getter and setter (Paolo Insogna) #45777
url:
  * (SEMVER-MINOR) add value argument to has and delete methods (Sankalp Shubham) #47885

PR-URL: #49220
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
@atlowChemi @ruyadorno
events: allow safely adding listener to abortSignal
0a3ae9f
PR-URL: #48596
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
ruyadorno added a commit that referenced this pull request Sep 13, 2023
@ruyadorno
2023-09-14, Version 18.18.0 'Hydrogen' (LTS)
e873eef
Notable changes:

build:
  * sync libuv header change (Jiawen Geng) #48078
crypto:
  * update root certificates to NSS 3.93 (Node.js GitHub Bot) #49341
  * update root certificates to NSS 3.90 (Node.js GitHub Bot) #48416
deps:
  * add missing thread-common.c in uv.gyp (Santiago Gimeno) #48078
  * upgrade to libuv 1.46.0 (Santiago Gimeno) #48078
  * upgrade to libuv 1.45.0 (Santiago Gimeno) #48078
doc:
  * add atlowChemi to collaborators (atlowChemi) #48757
  * add vmoroz to collaborators (Vladimir Morozov) #48527
  * add kvakil to collaborators (Keyhan Vakil) #48449
esm:
  * (SEMVER-MINOR) add `--import` flag (Moshe Atlow) #43942
events:
  * (SEMVER-MINOR) allow safely adding listener to abortSignal (Chemi Atlow) #48596
fs, stream:
  * initial `Symbol.dispose` and `Symbol.asyncDispose` support (Moshe Atlow) #48518
net:
  * add autoSelectFamily global getter and setter (Paolo Insogna) #45777
url:
  * (SEMVER-MINOR) add value argument to has and delete methods (Sankalp Shubham) #47885

PR-URL: #49220
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
@atlowChemi @ruyadorno
events: allow safely adding listener to abortSignal
a68a67f
PR-URL: #48596
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
ruyadorno added a commit that referenced this pull request Sep 17, 2023
@ruyadorno
2023-09-18, Version 18.18.0 'Hydrogen' (LTS)
ab5fa2a
Notable changes:

build:
  * sync libuv header change (Jiawen Geng) #48078
crypto:
  * update root certificates to NSS 3.93 (Node.js GitHub Bot) #49341
  * update root certificates to NSS 3.90 (Node.js GitHub Bot) #48416
deps:
  * add missing thread-common.c in uv.gyp (Santiago Gimeno) #48078
  * upgrade to libuv 1.46.0 (Santiago Gimeno) #48078
  * upgrade to libuv 1.45.0 (Santiago Gimeno) #48078
doc:
  * add atlowChemi to collaborators (atlowChemi) #48757
  * add vmoroz to collaborators (Vladimir Morozov) #48527
  * add kvakil to collaborators (Keyhan Vakil) #48449
esm:
  * (SEMVER-MINOR) add `--import` flag (Moshe Atlow) #43942
events:
  * (SEMVER-MINOR) allow safely adding listener to abortSignal (Chemi Atlow) #48596
fs, stream:
  * initial `Symbol.dispose` and `Symbol.asyncDispose` support (Moshe Atlow) #48518
net:
  * add autoSelectFamily global getter and setter (Paolo Insogna) #45777
url:
  * (SEMVER-MINOR) add value argument to has and delete methods (Sankalp Shubham) #47885

PR-URL: #49220
ruyadorno added a commit that referenced this pull request Sep 18, 2023
@ruyadorno
2023-09-18, Version 18.18.0 'Hydrogen' (LTS)
78ab0ee
Notable changes:

build:
  * sync libuv header change (Jiawen Geng) #48078
crypto:
  * update root certificates to NSS 3.93 (Node.js GitHub Bot) #49341
  * update root certificates to NSS 3.90 (Node.js GitHub Bot) #48416
deps:
  * add missing thread-common.c in uv.gyp (Santiago Gimeno) #48078
  * upgrade to libuv 1.46.0 (Santiago Gimeno) #48078
  * upgrade to libuv 1.45.0 (Santiago Gimeno) #48078
doc:
  * add atlowChemi to collaborators (atlowChemi) #48757
  * add vmoroz to collaborators (Vladimir Morozov) #48527
  * add kvakil to collaborators (Keyhan Vakil) #48449
esm:
  * (SEMVER-MINOR) add `--import` flag (Moshe Atlow) #43942
events:
  * (SEMVER-MINOR) allow safely adding listener to abortSignal (Chemi Atlow) #48596
fs, stream:
  * initial `Symbol.dispose` and `Symbol.asyncDispose` support (Moshe Atlow) #48518
net:
  * add autoSelectFamily global getter and setter (Paolo Insogna) #45777
url:
  * (SEMVER-MINOR) add value argument to has and delete methods (Sankalp Shubham) #47885

PR-URL: #49220
@atlowChemi atlowChemi mentioned this pull request Mar 14, 2024
events: extract addAbortListener for safe internal use #52081
Closed
atlowChemi added a commit to atlowChemi/node that referenced this pull request Mar 15, 2024
@atlowChemi
events: extract addAbortListener for safe internal use
29ee68a
Refs: nodejs#48596
nodejs-github-bot pushed a commit that referenced this pull request Mar 16, 2024
@atlowChemi @nodejs-github-bot
events: extract addAbortListener for safe internal use
e263946
Refs: #48596
PR-URL: #52081
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Mar 16, 2024
@atlowChemi @nodejs-github-bot
child_process: use internal addAbortListener
562369f
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Mar 16, 2024
@atlowChemi @nodejs-github-bot
dgram: use internal addAbortListener
8362caa
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Mar 16, 2024
@atlowChemi @nodejs-github-bot
net: use internal addAbortListener
d89fc73
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Mar 16, 2024
@atlowChemi @nodejs-github-bot
readline: use internal addAbortListener
ff93f3e
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Mar 16, 2024
@atlowChemi @nodejs-github-bot
http2: use internal addAbortListener
b9d8a14
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Mar 16, 2024
@atlowChemi @nodejs-github-bot
stream: use internal addAbortListener
a9528e8
PR-URL: #52081
Refs: #48596
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@benjamingr benjamingr benjamingr approved these changes
@ronag ronag ronag approved these changes
@MoLow MoLow MoLow approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. events Issues and PRs related to EventEmitter and the events module. 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@atlowChemi @benjamingr @mcollina @ronag @nodejs-github-bot @MoLow
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.