fs: throw on invalid callbacks for async functions by thefourtheye · Pull Request #12562 · nodejs/node

thefourtheye · 2017-04-21T04:54:19Z

If an asynchronous function is passed no callback function, there is no
way to return the result. This patch throws an error if the callback
passed is not valid or none passed at all.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

fs

@nodejs/ctc

mscdex · 2017-04-21T04:58:09Z

lib/fs.js

This function's implementation should be changed too. In fact, I'd rather just get rid of rethrow() altogether and just copy the throw ... where needed as I don't see the value in having a function that all it does is throw an exception (+ it avoids a potential function call).

Might be worth benchmarking. I thought using throw directly could de-opt a function? I had some fairly mysterious regressions in performance when doing some tiny refactors like this with an old experiment.

@sam-github Not throwing, but try-catch/try-finally used to. However I think with the V8 in node v7.x, functions containing try-catch/try-finally are optimizable, but are still not inlineable (not sure if the inlineability limit was with Crankshaft or TurboFan or both though).

Definite +1 to getting rid of rethrow() if we can

I refactored it a little bit to get rid of rethrow. PTAL.

jasnell · 2017-04-21T16:35:12Z

This would require a documentation change both fs.md and deprecations.md (Item id DEP0013). The deprecation description would need to be updated to show that not passing a callback is "end-of-life" and that an error will now be thrown. (The entry must not be removed from deprecations.md tho)

jasnell · 2017-04-21T16:36:36Z

lib/fs.js

Wherever this error ends up being thrown, please include a comment that indicates that this previously was a deprecation with ID code DEP0013

Since this is semver-major already, please migrate the error to use the new internal/errors.js API.

jasnell · 2017-04-21T16:37:35Z

lib/fs.js

Definite +1 to getting rid of rethrow() if we can

jasnell

Almost there!

jasnell · 2017-04-21T17:47:48Z

test/parallel/test-fs-access.js

These can be changed to use common.expectsError({code: 'ERR_INVALID_CALLBACK'})

jasnell · 2017-04-21T17:48:27Z

test/parallel/test-fs-chmod.js

alternatively this could be fs.close(fd, common.noop), but this works also.

jasnell · 2017-04-21T17:48:58Z

test/parallel/test-fs-make-callback.js

const cbTypeError = common.expectsError({code: 'ERR_INVALID_CALLBACK'});

jasnell · 2017-04-21T17:49:25Z

test/parallel/test-fs-makeStatsCallback.js

ditto ... I'll stop pointing these out.

thefourtheye · 2017-04-21T18:01:15Z

Thanks @jasnell :-) Updated the PR as per your suggestions.

mscdex · 2017-04-21T18:08:41Z

lib/fs.js

Can the conditionals' bodies be placed on separate lines please?

mscdex · 2017-04-21T18:10:50Z

FWIW I still prefer to see the check/throw copied where needed instead of making maybeCallback() the new rethrow().

thefourtheye · 2017-04-21T18:40:59Z

@mscdex Addressed your comments now. PTAL.

mscdex · 2017-04-21T18:46:23Z

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

addaleax · 2017-04-21T20:55:30Z

doc/api/fs.md

I would still prefer to stick with REPLACEME for the versions themselves (unless you’re reasonably sure that this doesn’t need to be fixed up again 😄 )

Sure, updated to REPLACEME now. PTAL.

If an asynchronous function is passed no callback function, there is no way to return the result. This patch throws an error if the callback passed is not valid or none passed at all.

thefourtheye · 2017-05-09T16:05:29Z

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

thefourtheye · 2017-05-09T16:45:51Z

CI is green. I'll land this tomorrow, if there are no objections.

thefourtheye · 2017-05-09T18:03:00Z

Oh boy, as per #12220 today is the semver-major cut-off day for version 8.

@jasnell @addaleax kindly take a look and confirm if this is okay to land now.

thefourtheye · 2017-05-09T18:39:00Z

Landed in 4cb5f3d. Thanks for the quick review @addaleax :-)

If an asynchronous function is passed no callback function, there is no way to return the result. This patch throws an error if the callback passed is not valid or none passed at all. PR-URL: #12562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This reverts 4cb5f3d Based on community feedback I think we should consider reverting this change. We should explore how this could be solved via linting rules. Refs: #12562 PR-URL: #12976 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

medikoo · 2017-07-21T07:58:06Z

Not providing callback to async function, is similar to not providing an error handler to event listener. In latter case node.js throws since very early days, and it seems as great pattern that works for most developers.

So taking this in (not just as a depreaction warning), will just make error handling in Node.js more consistent (and for 100% coverage we also need #12010 )

This reverts commit 8250bfd. PR-URL: nodejs#18668 Refs: nodejs#12562 Refs: nodejs#12976 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>

The callback should run in the global scope and not in the FSReqWrap context. PR-URL: nodejs#18668 Refs: nodejs#12562 Refs: nodejs#12976 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>

* v10.0.0 -> Node.js 10.0.0 * Parenthetical with URL rather than "PR" as a lot of people may not know what "PR" stands for but they will know what a URL is. Plus not hiding the URL means the text is more copy/paste-able. In this particular case, "PR 12562" is not more useful or informative than nodejs#12562 so just use the URL.

* v10.0.0 -> Node.js 10.0.0 * Parenthetical with URL rather than "PR" as a lot of people may not know what "PR" stands for but they will know what a URL is. Plus not hiding the URL means the text is more copy/paste-able. In this particular case, "PR 12562" is not more useful or informative than nodejs#12562 so just use the URL. PR-URL: nodejs#20519 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

This reverts commit 8250bfd. PR-URL: nodejs#18668 Refs: nodejs#12562 Refs: nodejs#12976 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>

The callback should run in the global scope and not in the FSReqWrap context. PR-URL: nodejs#18668 Refs: nodejs#12562 Refs: nodejs#12976 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>

* v10.0.0 -> Node.js 10.0.0 * Parenthetical with URL rather than "PR" as a lot of people may not know what "PR" stands for but they will know what a URL is. Plus not hiding the URL means the text is more copy/paste-able. In this particular case, "PR 12562" is not more useful or informative than #12562 so just use the URL. PR-URL: #20519 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

As per this nodejs/node#12562 node PR callback in async fs functions is no longer optional. This commit fixes remaining usages in impress.

As per this nodejs/node#12562 node PR callback in async fs functions is no longer optional. This commit fixes remaining usages in impress. Refs: #852 PR-URL: #856

Now mandatory in NodeJS 10. See nodejs/node#12562

thefourtheye added fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 21, 2017

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Apr 21, 2017

thefourtheye force-pushed the throw-if-no-callback branch from 940a81a to 6e6aa0a Compare April 21, 2017 04:55

mscdex reviewed Apr 21, 2017

View reviewed changes

jasnell requested changes Apr 21, 2017

View reviewed changes

thefourtheye force-pushed the throw-if-no-callback branch from 6e6aa0a to 2ffd589 Compare April 21, 2017 17:33

jasnell requested changes Apr 21, 2017

View reviewed changes

thefourtheye force-pushed the throw-if-no-callback branch from 7a75ee6 to f4255cc Compare April 21, 2017 18:00

mscdex reviewed Apr 21, 2017

View reviewed changes

lib/fs.js Outdated

Copy link

Contributor

mscdex Apr 21, 2017

Choose a reason for hiding this comment

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

Can the conditionals' bodies be placed on separate lines please?

thefourtheye force-pushed the throw-if-no-callback branch from f4255cc to c9c99e5 Compare April 21, 2017 18:38

addaleax reviewed Apr 21, 2017

View reviewed changes

jasnell approved these changes Apr 24, 2017

View reviewed changes

thefourtheye force-pushed the throw-if-no-callback branch from c9c99e5 to 97c570b Compare April 27, 2017 02:02

addaleax approved these changes Apr 27, 2017

View reviewed changes

fs: throw on invalid callbacks for async functions

2096f8b

If an asynchronous function is passed no callback function, there is no way to return the result. This patch throws an error if the callback passed is not valid or none passed at all.

thefourtheye force-pushed the throw-if-no-callback branch from 97c570b to 2096f8b Compare May 9, 2017 16:03

addaleax approved these changes May 9, 2017

View reviewed changes

thefourtheye closed this May 9, 2017

thefourtheye deleted the throw-if-no-callback branch May 9, 2017 18:39

toddself mentioned this pull request May 31, 2017

Fixes deprecation warning juliangruber/browser-run#139

Closed

BridgeAR mentioned this pull request Feb 9, 2018

Revert "fs: Revert throw on invalid callbacks" #18668

Closed

4 tasks

lundibundi added a commit to metarhia/impress that referenced this pull request Jul 28, 2018

Update fs usages according to node v10 deprecations

1fb5b19

As per this nodejs/node#12562 node PR callback in async fs functions is no longer optional. This commit fixes remaining usages in impress.

lundibundi mentioned this pull request Jul 28, 2018

Update fs usages according to node v10 deprecations metarhia/impress#856

Closed

stef-levesque added a commit to stef-levesque/HlslTools that referenced this pull request Mar 30, 2019

Fix DEP0013: fs async function without callback

9ec5ce0

Now mandatory in NodeJS 10. See nodejs/node#12562

aduh95 mentioned this pull request Feb 1, 2021

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

Closed

Uh oh!

Conversation

thefourtheye commented Apr 21, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Apr 21, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Apr 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Apr 21, 2017

Uh oh!

thefourtheye commented Apr 21, 2017

Uh oh!

mscdex commented Apr 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented May 9, 2017

Uh oh!

thefourtheye commented May 9, 2017

Uh oh!

thefourtheye commented May 9, 2017

Uh oh!

thefourtheye commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

medikoo commented Jul 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

17 participants

mscdex Apr 21, 2017 •

edited

Loading

thefourtheye commented May 9, 2017 •

edited

Loading