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: use a default callback for fs.close() #37174

Closed
wants to merge 3 commits into from
Closed

fs: use a default callback for fs.close() #37174

wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 1, 2021

The fs.close() function requires a callback. Most often the only thing
that callback does is check and rethrow the error if one occurs. To
eliminate common boilerplate, make the callback optional with a default
that checks and rethrows the error as an uncaught exception.

fs.close(fd);

// is equivalent to

fs.close(fd, (err) => {
  if (err != null) throw err;
});

/cc @mcollina

Signed-off-by: James M Snell jasnell@gmail.com

@jasnell
fs: use a default callback for fs.close()
61c243f 
The `fs.close()` function requires a callback. Most often the only thing
that callback does is check and rethrow the error if one occurs. To
eliminate common boilerplate, make the callback optional with a default
that checks and rethrows the error as an uncaught exception.

Signed-off-by: James M Snell <jasnell@gmail.com>
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Feb 1, 2021
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 1, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35871/

juanarbol
juanarbol approved these changes Feb 1, 2021
View reviewed changes
Copy link
Member

@juanarbol juanarbol left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I like this

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2021
@aduh95
Copy link
Contributor

aduh95 commented Feb 1, 2021

Note that this was the behaviour until Node.js v7.0.0 (deprecated in #7897 and removed in #12562).
Should we make the change for other functions as well? I think we can make the same assumption for fs.copyFile, fs.rename, fs.rm, fs.unlink, fs.writeFile, etc.

@jasnell
Copy link
Member Author

jasnell commented Feb 1, 2021

Should we make the change for other functions as well?

I wouldn't. fs.close() is a bit special since errors there are fairly rare. There are really good reasons why we'd want to require a callback be provided in those cases to ensure that things are handled appropriately. With errors on close, however, most often all you want to do is crash.

aduh95
aduh95 approved these changes Feb 1, 2021
View reviewed changes
lib/fs.js Outdated Show resolved Hide resolved
mcollina
mcollina approved these changes Feb 2, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

benjamingr
benjamingr approved these changes Feb 2, 2021
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

This is also good because it follows the same behaviour as fsPromises :]

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35922/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35932/

lpinca
lpinca approved these changes Feb 3, 2021
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
jasnell added a commit that referenced this pull request Feb 4, 2021
@jasnell
fs: use a default callback for fs.close()
254514c 
The `fs.close()` function requires a callback. Most often the only thing
that callback does is check and rethrow the error if one occurs. To
eliminate common boilerplate, make the callback optional with a default
that checks and rethrows the error as an uncaught exception.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37174
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
jasnell and others added 2 commits February 4, 2021 10:40
@jasnell @lpinca
[Squash] nits
602d8ba 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
@jasnell @aduh95
[Squash] nit
e696b41 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35944/

@jasnell
Copy link
Member Author

jasnell commented Feb 5, 2021

Landed in 7d0f680

@jasnell jasnell closed this Feb 5, 2021
jasnell added a commit that referenced this pull request Feb 5, 2021
@jasnell
fs: use a default callback for fs.close()
7d0f680 
The `fs.close()` function requires a callback. Most often the only thing
that callback does is check and rethrow the error if one occurs. To
eliminate common boilerplate, make the callback optional with a default
that checks and rethrows the error as an uncaught exception.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37174
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
@jasnell @danielleadams
fs: use a default callback for fs.close()
655d196 
The `fs.close()` function requires a callback. Most often the only thing
that callback does is check and rethrow the error if one occurs. To
eliminate common boilerplate, make the callback optional with a default
that checks and rethrows the error as an uncaught exception.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37174
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams added a commit that referenced this pull request Feb 16, 2021
@danielleadams
2021-02-16, Version 15.9.0 (Current)
5133659 
Notable Changes:

* crypto:
  * add keyObject.export() 'jwk' format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
This was referenced Feb 16, 2021
Closed
Merged
danielleadams added a commit that referenced this pull request Feb 17, 2021
@danielleadams
2021-02-17, Version 15.9.0 (Current)
889ffde 
Notable Changes:

* crypto:
  * add keyObject.export() 'jwk' format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
danielleadams added a commit that referenced this pull request Feb 17, 2021
@danielleadams
2021-02-17, Version 15.9.0 (Current)
b471267 
Notable Changes:

* crypto:
  * add keyObject.export() jwk format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
danielleadams added a commit that referenced this pull request Feb 17, 2021
@danielleadams
2021-02-17, Version 15.9.0 (Current)
877f66e 
PR-URL: #37406

Notable Changes:

* crypto:
  * add keyObject.export() jwk format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
danielleadams added a commit that referenced this pull request Feb 18, 2021
@danielleadams
2021-02-17, Version 15.9.0 (Current)
3bf50a7 
PR-URL: #37406

Notable Changes:

* crypto:
  * add keyObject.export() jwk format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
@aduh95 aduh95 mentioned this pull request Feb 20, 2021
Merged
targos pushed a commit that referenced this pull request May 1, 2021
@jasnell @targos
fs: use a default callback for fs.close()
92348a9 
The `fs.close()` function requires a callback. Most often the only thing
that callback does is check and rethrow the error if one occurs. To
eliminate common boilerplate, make the callback optional with a default
that checks and rethrows the error as an uncaught exception.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37174
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes

@juanarbol juanarbol juanarbol approved these changes

@aduh95 aduh95 aduh95 approved these changes

@Lxxyx Lxxyx Lxxyx approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr 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. fs Issues and PRs related to the fs subsystem / file system. 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.
9 participants
@jasnell @nodejs-github-bot @aduh95 @mcollina @benjamingr @lpinca @Lxxyx @juanarbol @RaisinTen