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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions doc/api/deprecations.md
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,10 @@ explicitly via error event handlers set on the domain instead.
<a id="DEP0013"></a>
### DEP0013: fs async function without callback

Type: Runtime
Type: End-of-Life

Calling an asynchronous function without a callback is deprecated.
Calling an asynchronous function without a callback will throw a `TypeError`
v8.0.0 onwards. Refer: [PR 12562](https://github.com/nodejs/node/pull/12562)

<a id="DEP0014"></a>
### DEP0014: fs.read legacy String interface
Expand Down
180 changes: 150 additions & 30 deletions doc/api/fs.md

Large diffs are not rendered by default.

60 changes: 16 additions & 44 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ const kMaxLength = require('buffer').kMaxLength;

const isWindows = process.platform === 'win32';

const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG);
const errnoException = util._errnoException;

function getOptions(options, defaultOptions) {
Expand Down Expand Up @@ -88,48 +87,26 @@ function copyObject(source) {
return target;
}

function rethrow() {
// TODO(thefourtheye) Throw error instead of warning in major version > 7
process.emitWarning(
'Calling an asynchronous function without callback is deprecated.',
'DeprecationWarning', 'DEP0013', rethrow
);

// Only enable in debug mode. A backtrace uses ~1000 bytes of heap space and
// is fairly slow to generate.
if (DEBUG) {
var backtrace = new Error();
return function(err) {
if (err) {
backtrace.stack = err.name + ': ' + err.message +
backtrace.stack.substr(backtrace.name.length);
throw backtrace;
}
};
}

return function(err) {
if (err) {
throw err; // Forgot a callback but don't know where? Use NODE_DEBUG=fs
}
};
var internalErrors;
function lazyErrors() {
if (!internalErrors)
internalErrors = require('internal/errors');
return internalErrors;
}

function maybeCallback(cb) {
return typeof cb === 'function' ? cb : rethrow();
if (typeof cb === 'function')
return cb;
else
throw new (lazyErrors().TypeError)('ERR_INVALID_CALLBACK');
}

// Ensure that callbacks run in the global context. Only use this function
// for callbacks that are passed to the binding layer, callbacks that are
// invoked from JS already run in the proper scope.
function makeCallback(cb) {
if (cb === undefined) {
return rethrow();
}

if (typeof cb !== 'function') {
throw new TypeError('"callback" argument must be a function');
}
if (typeof cb !== 'function')
throw new (lazyErrors().TypeError)('ERR_INVALID_CALLBACK');

return function() {
return cb.apply(null, arguments);
Expand All @@ -140,13 +117,8 @@ function makeCallback(cb) {
// an optimization, since the data passed back to the callback needs to be
// transformed anyway.
function makeStatsCallback(cb) {
if (cb === undefined) {
return rethrow();
}

if (typeof cb !== 'function') {
throw new TypeError('"callback" argument must be a function');
}
if (typeof cb !== 'function')
throw new (lazyErrors().TypeError)('ERR_INVALID_CALLBACK');

return function(err) {
if (err) return cb(err);
Expand Down Expand Up @@ -268,10 +240,10 @@ fs.access = function(path, mode, callback) {
if (typeof mode === 'function') {
callback = mode;
mode = fs.F_OK;
} else if (typeof callback !== 'function') {
throw new TypeError('"callback" argument must be a function');
}

callback = makeCallback(callback);

if (handleError((path = getPathFromURL(path)), callback))
return;

Expand All @@ -280,7 +252,7 @@ fs.access = function(path, mode, callback) {

mode = mode | 0;
var req = new FSReqWrap();
req.oncomplete = makeCallback(callback);
req.oncomplete = callback;
binding.access(pathModule._makeLong(path), mode, req);
};

Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/test-fs-readfile-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,4 @@
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

require('fs').readFile('/'); // throws EISDIR
require('fs').readFileSync('/'); // throws EISDIR
4 changes: 2 additions & 2 deletions test/parallel/test-fs-access.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,11 @@ assert.throws(() => {

assert.throws(() => {
fs.access(__filename, fs.F_OK);
}, /^TypeError: "callback" argument must be a function$/);
}, common.expectsError({code: 'ERR_INVALID_CALLBACK'}));

assert.throws(() => {
fs.access(__filename, fs.F_OK, {});
}, /^TypeError: "callback" argument must be a function$/);
}, common.expectsError({code: 'ERR_INVALID_CALLBACK'}));

assert.doesNotThrow(() => {
fs.accessSync(__filename);
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-fs-link.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,14 @@ fs.link(srcPath, dstPath, common.mustCall(callback));

assert.throws(
function() {
fs.link();
fs.link(undefined, undefined, common.mustNotCall());
},
/src must be a string or Buffer/
);

assert.throws(
function() {
fs.link('abc');
fs.link('abc', undefined, common.mustNotCall());
},
/dest must be a string or Buffer/
);
8 changes: 1 addition & 7 deletions test/parallel/test-fs-make-callback.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,10 @@
const common = require('../common');
const assert = require('assert');
const fs = require('fs');
const cbTypeError = /^TypeError: "callback" argument must be a function$/;
const cbTypeError = common.expectsError({code: 'ERR_INVALID_CALLBACK'});
const callbackThrowValues = [null, true, false, 0, 1, 'foo', /foo/, [], {}];

const { sep } = require('path');
const warn = 'Calling an asynchronous function without callback is deprecated.';

common.refreshTmpDir();

Expand All @@ -17,11 +16,6 @@ function testMakeCallback(cb) {
};
}

common.expectWarning('DeprecationWarning', warn);

// Passing undefined/nothing calls rethrow() internally, which emits a warning
assert.doesNotThrow(testMakeCallback());

function invalidCallbackThrowsTests() {
callbackThrowValues.forEach((value) => {
assert.throws(testMakeCallback(value), cbTypeError);
Expand Down
8 changes: 1 addition & 7 deletions test/parallel/test-fs-makeStatsCallback.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@
const common = require('../common');
const assert = require('assert');
const fs = require('fs');
const cbTypeError = /^TypeError: "callback" argument must be a function$/;
const cbTypeError = common.expectsError({code: 'ERR_INVALID_CALLBACK'});
const callbackThrowValues = [null, true, false, 0, 1, 'foo', /foo/, [], {}];
const warn = 'Calling an asynchronous function without callback is deprecated.';

function testMakeStatsCallback(cb) {
return function() {
Expand All @@ -13,14 +12,9 @@ function testMakeStatsCallback(cb) {
};
}

common.expectWarning('DeprecationWarning', warn);

// Verify the case where a callback function is provided
assert.doesNotThrow(testMakeStatsCallback(common.noop));

// Passing undefined/nothing calls rethrow() internally, which emits a warning
assert.doesNotThrow(testMakeStatsCallback());

function invalidCallbackThrowsTests() {
callbackThrowValues.forEach((value) => {
assert.throws(testMakeStatsCallback(value), cbTypeError);
Expand Down
5 changes: 0 additions & 5 deletions test/parallel/test-fs-mkdtemp.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,3 @@ fs.mkdtemp(path.join(common.tmpDir, 'bar.'), common.mustCall(handler));
// Same test as above, but making sure that passing an options object doesn't
// affect the way the callback function is handled.
fs.mkdtemp(path.join(common.tmpDir, 'bar.'), {}, common.mustCall(handler));

// Making sure that not passing a callback doesn't crash, as a default function
// is passed internally.
assert.doesNotThrow(() => fs.mkdtemp(path.join(common.tmpDir, 'bar-')));
assert.doesNotThrow(() => fs.mkdtemp(path.join(common.tmpDir, 'bar-'), {}));
2 changes: 1 addition & 1 deletion test/parallel/test-fs-readfile-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ function test(env, cb) {

test({ NODE_DEBUG: '' }, common.mustCall((data) => {
assert(/EISDIR/.test(data));
assert(!/test-fs-readfile-error/.test(data));
assert(/test-fs-readfile-error/.test(data));
}));

test({ NODE_DEBUG: 'fs' }, common.mustCall((data) => {
Expand Down
6 changes: 3 additions & 3 deletions test/parallel/test-fs-write-no-fd.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
'use strict';
require('../common');
const common = require('../common');
const fs = require('fs');
const assert = require('assert');

assert.throws(function() {
fs.write(null, Buffer.allocUnsafe(1), 0, 1);
fs.write(null, Buffer.allocUnsafe(1), 0, 1, common.mustNotCall());
}, /TypeError/);

assert.throws(function() {
fs.write(null, '1', 0, 1);
fs.write(null, '1', 0, 1, common.mustNotCall());
}, /TypeError/);