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.

Sign up for GitHub

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

Jump to bottom

fs, stream: add initial Symbol.dispose and Symbol.asyncDispose support #48518

Merged
merged 10 commits into from
Jun 25, 2023
Merged

fs, stream: add initial Symbol.dispose and Symbol.asyncDispose support #48518

merged 10 commits into from
Jun 25, 2023

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Jun 22, 2023

Both TypeScript and Bable already support using (explicit resource management proposal),
adding this to node will be convenient

Co-authored-by: Benjamin Gruenbaum benjamingr@gmail.com

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jun 22, 2023
ronag
ronag reviewed Jun 22, 2023
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
ronag
ronag reviewed Jun 22, 2023
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Jun 22, 2023
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Jun 22, 2023
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
ronag
ronag requested changes Jun 22, 2023
View reviewed changes
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

I added some comments.

ronag
ronag reviewed Jun 22, 2023
View reviewed changes
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

Q: Does await using fallback to Symbol.dispose if the async version doesn't exist?

@MoLow @benjamingr
fs, stream: initial Symbol.dispose and Symbol.asyncDispose support
8a25397 
Co-authored-by: Benjamin Gruenbaum <benjamingr@gmail.com>
@MoLow
Copy link
Member Author

MoLow commented Jun 22, 2023

Q: Does await using fallback to Symbol.dispose if the async version doesn't exist?

according to this it does:
https://github.com/tc39/proposal-explicit-resource-management#await-using-declarations-with-explicit-local-bindings

ronag
ronag reviewed Jun 22, 2023
View reviewed changes
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

Implement for stream.Writable as well?

MoLow and others added 3 commits June 22, 2023 12:51
@MoLow @ronag
Update lib/internal/process/pre_execution.js
8dc2ee7 
Co-authored-by: Robert Nagy <ronagy@icloud.com>
@MoLow @ronag
Update lib/internal/streams/readable.js
223a2d8 
Co-authored-by: Robert Nagy <ronagy@icloud.com>
@MoLow
lint
40ba972
@benjamingr
Copy link
Member

@ronag

Implement for stream.Writable as well?

Yeah, and for a lot of other things (http.Server etc), the idea here was to add the symbols and one example so we can distribute the work like we did last year with iterator-helpers.

benjamingr and others added 2 commits June 22, 2023 13:37
@benjamingr @ronag
Update lib/internal/streams/readable.js
e8a014f 
Co-authored-by: Robert Nagy <ronagy@icloud.com>
@benjamingr @ronag
Update lib/internal/streams/readable.js
48602bb 
Co-authored-by: Robert Nagy <ronagy@icloud.com>
ljharb added a commit to ljharb/node that referenced this pull request Jul 8, 2023
@ljharb
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
d8e640c 
Followup to nodejs#48518; fixes nodejs#48699
ljharb added a commit to ljharb/node that referenced this pull request Jul 8, 2023
@ljharb
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
c70ae63 
Followup to nodejs#48518; fixes nodejs#48699
@rixtox rixtox mentioned this pull request Jul 10, 2023
Open
nodejs-github-bot pushed a commit that referenced this pull request Jul 12, 2023
@ljharb
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
c2c7260 
Followup to #48518; fixes #48699

PR-URL: #48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
juanarbol pushed a commit that referenced this pull request Jul 13, 2023
@ljharb @juanarbol
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
eb0aba5 
Followup to #48518; fixes #48699

PR-URL: #48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@MoLow @benjamingr @Ceres6
fs, stream: initial Symbol.dispose and Symbol.asyncDispose support
64bfd1e 
Co-authored-by: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: nodejs#48518
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Erick Wendel <erick.workspace@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@RafaelGSS @Ceres6
2023-07-05, Version 20.4.0 (Current)
7fa148c 
Notable changes:

crypto:
  * update root certificates to NSS 3.90 (Node.js GitHub Bot) nodejs#48416
doc:
  * add vmoroz to collaborators (Vladimir Morozov) nodejs#48527
  * add kvakil to collaborators (Keyhan Vakil) nodejs#48449
fs, stream:
  * initial `Symbol.dispose` and `Symbol.asyncDispose` support (Moshe Atlow) nodejs#48518
test_runner:
  * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs#47775
tls:
  * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs#45190

PR-URL: nodejs#48643
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@ljharb @Ceres6
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
8396660 
Followup to nodejs#48518; fixes nodejs#48699

PR-URL: nodejs#48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@MoLow @benjamingr @Ceres6
fs, stream: initial Symbol.dispose and Symbol.asyncDispose support
fd44fe8 
Co-authored-by: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: nodejs#48518
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Erick Wendel <erick.workspace@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@RafaelGSS @Ceres6
2023-07-05, Version 20.4.0 (Current)
574f438 
Notable changes:

crypto:
  * update root certificates to NSS 3.90 (Node.js GitHub Bot) nodejs#48416
doc:
  * add vmoroz to collaborators (Vladimir Morozov) nodejs#48527
  * add kvakil to collaborators (Keyhan Vakil) nodejs#48449
fs, stream:
  * initial `Symbol.dispose` and `Symbol.asyncDispose` support (Moshe Atlow) nodejs#48518
test_runner:
  * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs#47775
tls:
  * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs#45190

PR-URL: nodejs#48643
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@ljharb @Ceres6
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
9c52644 
Followup to nodejs#48518; fixes nodejs#48699

PR-URL: nodejs#48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@ruyadorno
Copy link
Member

This commit does not land cleanly on v18.x-staging and will need manual backport in case we want it in v18.

@ruyadorno ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 10, 2023
@atlowChemi atlowChemi mentioned this pull request Sep 11, 2023
Merged
atlowChemi pushed a commit to atlowChemi/node that referenced this pull request Sep 11, 2023
@MoLow @benjamingr @atlowChemi
fs, stream: initial Symbol.dispose and Symbol.asyncDispose support
5fecb22 
Co-authored-by: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: nodejs#48518
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Erick Wendel <erick.workspace@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@atlowChemi atlowChemi mentioned this pull request Sep 11, 2023
Closed
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
@MoLow @benjamingr @ruyadorno
fs, stream: initial Symbol.dispose and Symbol.asyncDispose support
6747721 
Co-authored-by: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: #48518
Backport-PR-URL: #49598
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Erick Wendel <erick.workspace@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@ruyadorno ruyadorno added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels Sep 11, 2023
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
@MoLow @benjamingr @ruyadorno
fs, stream: initial Symbol.dispose and Symbol.asyncDispose support
eed884b 
Co-authored-by: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: #48518
Backport-PR-URL: #49598
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Erick Wendel <erick.workspace@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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
@MoLow @benjamingr @ruyadorno
fs, stream: initial Symbol.dispose and Symbol.asyncDispose support
3a8586b 
Co-authored-by: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: #48518
Backport-PR-URL: #49598
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Erick Wendel <erick.workspace@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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
@simatec simatec mentioned this pull request Oct 5, 2023
Closed
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@ruyadorno @alexfernandez
2023-09-18, Version 18.18.0 'Hydrogen' (LTS)
714f228 
Notable changes:

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

PR-URL: nodejs#49220
targos pushed a commit that referenced this pull request Nov 27, 2023
@ljharb @targos
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
e08fd98 
Followup to #48518; fixes #48699

PR-URL: #48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@lukastaegert lukastaegert mentioned this pull request Nov 10, 2024
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@rbuckton rbuckton rbuckton left review comments

@zloirock zloirock zloirock left review comments

@benjamingr benjamingr benjamingr left review comments

@mscdex mscdex mscdex left review comments

@ljharb ljharb ljharb left review comments

@ErickWendel ErickWendel ErickWendel approved these changes

@ronag ronag ronag approved these changes

@anonrig anonrig anonrig approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backported-to-v18.x PRs backported to the v18.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.