Skip to content

Commit

Permalink
fs: Revert throw on invalid callbacks
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
MylesBorins committed May 20, 2017
1 parent 1ebdd1d commit eebae66
Show file tree
Hide file tree
Showing 11 changed files with 104 additions and 180 deletions.
5 changes: 2 additions & 3 deletions doc/api/deprecations.md
Original file line number Diff line number Diff line change
Expand Up @@ -150,10 +150,9 @@ explicitly via error event handlers set on the domain instead.
<a id="DEP0013"></a>
### DEP0013: fs async function without callback

Type: End-of-Life
Type: Runtime

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)
Calling an asynchronous function without a callback is deprecated.

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

Large diffs are not rendered by default.

60 changes: 44 additions & 16 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ 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 @@ -87,26 +88,48 @@ function copyObject(source) {
return target;
}

var internalErrors;
function lazyErrors() {
if (!internalErrors)
internalErrors = require('internal/errors');
return internalErrors;
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
}
};
}

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

// 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 (typeof cb !== 'function')
throw new (lazyErrors().TypeError)('ERR_INVALID_CALLBACK');
if (cb === undefined) {
return rethrow();
}

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

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

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

return function(err) {
if (err) return cb(err);
Expand Down Expand Up @@ -240,10 +268,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 @@ -252,7 +280,7 @@ fs.access = function(path, mode, callback) {

mode = mode | 0;
var req = new FSReqWrap();
req.oncomplete = callback;
req.oncomplete = makeCallback(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').readFileSync('/'); // throws EISDIR
require('fs').readFile('/'); // 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);
}, common.expectsError({code: 'ERR_INVALID_CALLBACK'}));
}, /^TypeError: "callback" argument must be a function$/);

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

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(undefined, undefined, common.mustNotCall());
fs.link();
},
/src must be a string or Buffer/
);

assert.throws(
function() {
fs.link('abc', undefined, common.mustNotCall());
fs.link('abc');
},
/dest must be a string or Buffer/
);
8 changes: 7 additions & 1 deletion test/parallel/test-fs-make-callback.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@
const common = require('../common');
const assert = require('assert');
const fs = require('fs');
const cbTypeError = common.expectsError({code: 'ERR_INVALID_CALLBACK'});
const cbTypeError = /^TypeError: "callback" argument must be a function$/;
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 @@ -16,6 +17,11 @@ 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: 7 additions & 1 deletion test/parallel/test-fs-makeStatsCallback.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@
const common = require('../common');
const assert = require('assert');
const fs = require('fs');
const cbTypeError = common.expectsError({code: 'ERR_INVALID_CALLBACK'});
const cbTypeError = /^TypeError: "callback" argument must be a function$/;
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 @@ -12,9 +13,14 @@ 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: 5 additions & 0 deletions test/parallel/test-fs-mkdtemp.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,8 @@ 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';
const common = require('../common');
require('../common');
const fs = require('fs');
const assert = require('assert');

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

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

0 comments on commit eebae66

Please sign in to comment.