fs: use a default callback for fs.close() by jasnell · Pull Request #37174 · nodejs/node

jasnell · 2021-02-01T19:58:37Z

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

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 · 2021-02-01T20:14:27Z

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

juanarbol

I like this

aduh95 · 2021-02-01T22:20:11Z

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 · 2021-02-01T22:27:47Z

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.

lib/fs.js

mcollina

lgtm

benjamingr

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

nodejs-github-bot · 2021-02-03T12:01:55Z

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

nodejs-github-bot · 2021-02-03T17:33:29Z

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

doc/api/fs.md

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>

Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot · 2021-02-04T18:45:00Z

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

jasnell · 2021-02-05T16:00:26Z

Landed in 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>

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

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

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

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>

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Feb 1, 2021

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 1, 2021

juanarbol approved these changes Feb 1, 2021

View reviewed changes

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 approved these changes Feb 1, 2021

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

mcollina approved these changes Feb 2, 2021

View reviewed changes

RaisinTen approved these changes Feb 2, 2021

View reviewed changes

benjamingr approved these changes Feb 2, 2021

View reviewed changes

Lxxyx approved these changes Feb 3, 2021

View reviewed changes

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 and others added 2 commits February 4, 2021 10:40

[Squash] nits

602d8ba

Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

[Squash] nit

e696b41

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

jasnell closed this Feb 5, 2021

This was referenced Feb 16, 2021

v15.9.0 proposal #37405

Closed

v15.9.0 proposal #37406

Merged

aduh95 mentioned this pull request Feb 20, 2021

lib: add chainErrors API to avoid error swallowing #37460

Merged

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

fs: use a default callback for fs.close()#37174
jasnell wants to merge 3 commits intonodejs:masterfrom
jasnell:default-fs-close-callback

jasnell commented Feb 1, 2021

Uh oh!

nodejs-github-bot commented Feb 1, 2021

Uh oh!

juanarbol left a comment •

edited

Loading

Uh oh!

aduh95 commented Feb 1, 2021

Uh oh!

jasnell commented Feb 1, 2021

Uh oh!

Uh oh!

mcollina left a comment

Uh oh!

benjamingr left a comment

Uh oh!

nodejs-github-bot commented Feb 3, 2021

Uh oh!

nodejs-github-bot commented Feb 3, 2021

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 4, 2021

Uh oh!

jasnell commented Feb 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Uh oh!

Conversation

jasnell commented Feb 1, 2021

Uh oh!

nodejs-github-bot commented Feb 1, 2021

Uh oh!

juanarbol left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Feb 1, 2021

Uh oh!

jasnell commented Feb 1, 2021

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 3, 2021

Uh oh!

nodejs-github-bot commented Feb 3, 2021

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 4, 2021

Uh oh!

jasnell commented Feb 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

juanarbol left a comment •

edited

Loading