Skip to content

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

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
Closed
jasnell wants to merge 3 commits into from
Closed

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

jasnell wants to merge 3 commits into from

Conversation

@jasnell
Copy link
Member

@jasnelljasnell 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 tofs.close(fd,(err)=>{if(err!=null)throwerr;});

/cc @mcollina

Signed-off-by: James M Snell [email protected]

@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 <[email protected]>
@nodejs-github-botnodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Feb 1, 2021
@jasnelljasnell 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

nodejs-github-bot commented Feb 1, 2021

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

@juanarboljuanarbol 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

@jasnelljasnell 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
MemberAuthor

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
mcollina
mcollina approved these changes Feb 2, 2021
View reviewed changes
Copy link
Member

@mcollinamcollina 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

@benjamingrbenjamingr 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

nodejs-github-bot commented Feb 3, 2021

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 3, 2021

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

lpinca
lpinca approved these changes Feb 3, 2021
View reviewed changes
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 <[email protected]> PR-URL: #37174 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
jasnelland others added 2 commits February 4, 2021 10:40
@jasnell@lpinca
[Squash] nits
602d8ba
Co-authored-by: Luigi Pinca <[email protected]>
@jasnell@aduh95
[Squash] nit
e696b41
Co-authored-by: Antoine du Hamel <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 4, 2021

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

@jasnell
Copy link
MemberAuthor

jasnell commented Feb 5, 2021

Landed in 7d0f680

@jasnelljasnell 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 <[email protected]> PR-URL: #37174 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
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 <[email protected]> PR-URL: #37174 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
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
@aduh95aduh95 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 <[email protected]> PR-URL: #37174 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@danielleadamsdanielleadams mentioned this pull request May 3, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

@benjamingrbenjamingrbenjamingr approved these changes

@lpincalpincalpinca approved these changes

@LxxyxLxxyxLxxyx approved these changes

@aduh95aduh95aduh95 approved these changes

@juanarboljuanarboljuanarbol approved these changes

@RaisinTenRaisinTenRaisinTen approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.fsIssues and PRs related to the fs subsystem / file system.semver-minorPRs 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