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

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation60 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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)

@nodejs/ctc

addaleax, cryptoquick, thelostone-mc, BridgeAR, sonicdoe, ChALkeR, and medikoo reacted with thumbs up emoji elvizcacho, WebReflection, AlexGalays, gildean, toddself, nickpeihl, kentor, nickleefly, and armordog reacted with thumbs down emoji

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

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

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

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.

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

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

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.

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)

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

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

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.

Choose a reason for hiding this comment

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

Done!

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

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

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$/);

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

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$/;

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$/;

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.

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

}

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

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?

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

@mscdex Addressed your comments now. PTAL.

@@ -448,10 +448,14 @@ checks fail, and does nothing otherwise.
<!-- YAML
added: v0.6.7
changes:
- version: v8.0.0

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

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.

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

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 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 jasnell@gmail.com Reviewed-By: Anna Henningsen anna@addaleax.net

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 thechargingvolcano@gmail.com Reviewed-By: Evan Lucas evanlucas@me.com Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Rich Trott rtrott@gmail.com

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.

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

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.

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

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request

May 8, 2018

This reverts commit 8250bfd.

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.

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

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.

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.

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

Labels

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.