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.

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

fs: throw on invalid callbacks for async functions #12562

Closed

Conversation

thefourtheye
Copy link
Contributor

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

@thefourtheye 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 nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Apr 21, 2017
lib/fs.js Outdated
throw err; // Forgot a callback but don't know where? Use NODE_DEBUG=fs
}
};
throw new TypeError('"callback" argument must be a function');
}

function maybeCallback(cb) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@mscdex mscdex Apr 21, 2017

Choose a reason for hiding this comment

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

@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).

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@jasnell
Copy link
Member

jasnell commented Apr 21, 2017

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)

lib/fs.js Outdated
throw err; // Forgot a callback but don't know where? Use NODE_DEBUG=fs
}
};
throw new TypeError('"callback" argument must be a function');
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

lib/fs.js Outdated
throw err; // Forgot a callback but don't know where? Use NODE_DEBUG=fs
}
};
throw new TypeError('"callback" argument must be a function');
}

function maybeCallback(cb) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Almost there!

@@ -87,11 +87,11 @@ assert.throws(() => {

assert.throws(() => {
fs.access(__filename, fs.F_OK);
}, /^TypeError: "callback" argument must be a function$/);
}, /^TypeError \[ERR_INVALID_CALLBACK\]: callback must be a function$/);
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -114,7 +114,7 @@ fs.open(file2, 'a', common.mustCall((err, fd) => {
assert.strictEqual(mode_sync, fs.fstatSync(fd).mode & 0o777);
}

fs.close(fd);
fs.closeSync(fd);
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -2,11 +2,11 @@
const common = require('../common');
const assert = require('assert');
const fs = require('fs');
const cbTypeError = /^TypeError: "callback" argument must be a function$/;
const cbTypeError =
/^TypeError \[ERR_INVALID_CALLBACK\]: callback must be a function$/;
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -2,9 +2,9 @@
const common = require('../common');
const assert = require('assert');
const fs = require('fs');
const cbTypeError = /^TypeError: "callback" argument must be a function$/;
const cbTypeError =
/^TypeError \[ERR_INVALID_CALLBACK\]: callback must be a function$/;
Copy link
Member

Choose a reason for hiding this comment

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

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

@thefourtheye
Copy link
Contributor Author

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

lib/fs.js Outdated
}

function maybeCallback(cb) {
return typeof cb === 'function' ? cb : rethrow();
if (typeof cb === 'function') return cb;
Copy link
Contributor

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?

@mscdex
Copy link
Contributor

mscdex commented Apr 21, 2017

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

@thefourtheye
Copy link
Contributor Author

@mscdex Addressed your comments now. PTAL.

@mscdex
Copy link
Contributor

mscdex commented Apr 21, 2017

doc/api/fs.md Outdated
@@ -448,10 +448,14 @@ checks fail, and does nothing otherwise.
<!-- YAML
added: v0.6.7
changes:
- version: v8.0.0
Copy link
Member

Choose a reason for hiding this comment

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

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 😄 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@thefourtheye
Copy link
Contributor Author

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

@thefourtheye
Copy link
Contributor Author

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
Copy link
Contributor Author

thefourtheye commented May 9, 2017

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

@thefourtheye thefourtheye deleted the throw-if-no-callback branch May 9, 2017 18:39
thefourtheye added a commit that referenced this pull request May 9, 2017
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins added a commit that referenced this pull request May 20, 2017
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 <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins added a commit that referenced this pull request May 20, 2017
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 <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@medikoo
Copy link

medikoo commented Jul 21, 2017

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 )

BridgeAR added a commit to BridgeAR/node that referenced this pull request Feb 26, 2018
This reverts commit 8250bfd.

PR-URL: nodejs#18668
Refs: nodejs#12562
Refs: nodejs#12976
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Feb 26, 2018
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request May 6, 2018
* 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.
Trott added a commit to Trott/io.js that referenced this pull request May 7, 2018
* 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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
This reverts commit 8250bfd.

PR-URL: nodejs#18668
Refs: nodejs#12562
Refs: nodejs#12976
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 8, 2018
* 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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 8, 2018
* 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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 9, 2018
* 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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
lundibundi added a commit to metarhia/impress that referenced this pull request Jul 28, 2018
As per this nodejs/node#12562 node PR callback
in async fs functions is no longer optional. This commit fixes
remaining usages in impress.
tshemsedinov pushed a commit to metarhia/impress that referenced this pull request Jul 28, 2018
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
stef-levesque added a commit to stef-levesque/HlslTools that referenced this pull request Mar 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.