FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs, stream: add initial `Symbol.dispose` and `Symbol.asyncDispose` support by MoLow · Pull Request #48518 · nodejs/node · GitHub

/ node Public

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

Merged
nodejs-github-bot merged 10 commits into
nodejs:mainfrom
MoLow:symbol-dispose
Jun 25, 2023
ConversationCommits (10)ChecksFiles changed
Merged

fs, stream: add initial Symbol.dispose and Symbol.asyncDispose support#48518
nodejs-github-bot merged 10 commits into
nodejs:mainfrom
MoLow:symbol-dispose

Conversation

MoLow commented Jun 22, 2023

Copy link
Copy Markdown
Member

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 commented Jun 22, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

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 reviewed Jun 22, 2023
View reviewed changes
Comment thread doc/api/fs.md Outdated
ronag reviewed Jun 22, 2023
View reviewed changes
Comment thread doc/api/stream.md Outdated
mscdex reviewed Jun 22, 2023
View reviewed changes
Comment thread doc/api/fs.md Outdated
ronag reviewed Jun 22, 2023
View reviewed changes
Comment thread lib/internal/streams/readable.js Outdated
mscdex reviewed Jun 22, 2023
View reviewed changes
Comment thread doc/api/stream.md Outdated
ronag reviewed Jun 22, 2023
View reviewed changes
Comment thread lib/internal/process/pre_execution.js Outdated
mscdex reviewed Jun 22, 2023
View reviewed changes
Comment thread lib/internal/process/pre_execution.js Outdated
ronag reviewed Jun 22, 2023
View reviewed changes
Comment thread lib/internal/process/pre_execution.js Outdated
ronag requested changes Jun 22, 2023
View reviewed changes

ronag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I added some comments.

ronag reviewed Jun 22, 2023
View reviewed changes

ronag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

MoLow commented Jun 22, 2023

Copy link
Copy Markdown
Member Author

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 reviewed Jun 22, 2023
View reviewed changes

ronag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Implement for stream.Writable as well?

ronag reviewed Jun 22, 2023
View reviewed changes
Comment thread lib/internal/process/pre_execution.js Outdated
ronag reviewed Jun 22, 2023
View reviewed changes
Comment thread lib/internal/streams/readable.js Outdated
MoLow and others added 3 commits June 22, 2023 12:51
Update lib/internal/process/pre_execution.js
8dc2ee7 
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Update lib/internal/streams/readable.js
223a2d8 
Co-authored-by: Robert Nagy <ronagy@icloud.com>
lint
40ba972

benjamingr commented Jun 22, 2023

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

ronag reviewed Jun 22, 2023
View reviewed changes
Comment thread lib/internal/streams/readable.js Outdated
ronag reviewed Jun 22, 2023
View reviewed changes
Comment thread lib/internal/streams/readable.js Outdated
benjamingr and others added 2 commits June 22, 2023 13:37
Update lib/internal/streams/readable.js
e8a014f 
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Update lib/internal/streams/readable.js
48602bb 
Co-authored-by: Robert Nagy <ronagy@icloud.com>
rixtox mentioned this pull request Jul 10, 2023
Open
nodejs-github-bot pushed a commit that referenced this pull request Jul 12, 2023
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
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
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
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
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
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
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
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 commented Sep 10, 2023

Copy link
Copy Markdown
Member

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

atlowChemi mentioned this pull request Sep 11, 2023
Merged
atlowChemi pushed a commit to atlowChemi/node that referenced this pull request Sep 11, 2023
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 mentioned this pull request Sep 11, 2023
Closed
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
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 added a commit that referenced this pull request Sep 11, 2023
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
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
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
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
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
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
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 mentioned this pull request Oct 5, 2023
Closed
targos pushed a commit that referenced this pull request Nov 27, 2023
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 mentioned this pull request Nov 10, 2024
Merged
9 tasks
skyclouds2001 mentioned this pull request Apr 5, 2025
Closed
coderabbitai Bot mentioned this pull request Jul 6, 2026
Merged
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ljharb ljharb left review comments
benjamingr benjamingr left review comments
aduh95 aduh95 left review comments
mcollina mcollina approved these changes
anonrig anonrig approved these changes
ronag ronag approved these changes
ErickWendel ErickWendel approved these changes
+3 more reviewers
mscdex mscdex left review comments
zloirock zloirock left review comments
rbuckton rbuckton left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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.

Back | FazBrowse Home | New Git URL