lukechilds
/
node
mirror of https://github.com/lukechilds/node.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity
Browse Source

Revert "fs: make callback mandatory to all async functions" 

This reverts commit 9359de9dd2.

Original Commit Message:

    The "fs" module has two functions called `maybeCallback` and
    `makeCallback`, as of now.

    The `maybeCallback` creates a default function to report errors, if the
    parameter passed is not a function object. Basically, if the callback
    is omitted in some cases, this function is used to create a default
    callback function.

    The `makeCallback`, OTOH, creates a default function only if the
    parameter passed is `undefined`, and if it is not a function object it
    will throw an `Error`.

    This patch removes the `maybeCallback` function and makes the callback
    function argument mandatory for all the async functions.

    PR-URL: https://github.com/nodejs/node/pull/7168
    Reviewed-By: Trevor Norris <trev.norris@gmail.com>

PR-URL: https://github.com/nodejs/node/pull/7846
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
v7.x
Myles Borins 9 years ago
committed by Anna Henningsen
parent
1a9e247c79
commit
21b0a27af8
No known key found for this signature in database GPG Key ID: D8B9F5AEAE84E4CF
10 changed files with 143 additions and 73 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 142
      lib/fs.js
  2. 1
      test/fixtures/test-fs-readfile-error.js
  3. 2
      test/parallel/test-fs-chmod.js
  4. 4
      test/parallel/test-fs-link.js
  5. 3
      test/parallel/test-fs-make-callback.js
  6. 5
      test/parallel/test-fs-mkdtemp.js
  7. 17
      test/parallel/test-fs-no-callback-errors.js
  8. 34
      test/parallel/test-fs-readfile-error.js
  9. 4
      test/parallel/test-fs-stat.js
  10. 4
      test/parallel/test-fs-watchfile.js

142
lib/fs.js
View File

@ -38,6 +38,7 @@ const O_WRONLY = constants.O_WRONLY || 0;
const isWindows = process.platform === 'win32';
const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG);
const errnoException = util._errnoException;
var printDeprecation;
@ -81,10 +82,39 @@ function throwOptionsError(options) {
'but got ' + typeof options + ' instead');
}
function 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) {
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 (cb === undefined) {
return rethrow();
}
if (typeof cb !== 'function') {
throw new TypeError('"callback" argument must be a function');
}
@ -191,9 +221,11 @@ fs.Stats.prototype.isSocket = function() {
});
fs.access = function(path, mode, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
if (typeof mode === 'function') {
callback = mode;
mode = fs.F_OK;
} else if (typeof callback !== 'function') {
throw new TypeError('"callback" argument must be a function');
}
if (!nullCheck(path, callback))
@ -201,7 +233,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);
};
@ -217,13 +249,12 @@ fs.accessSync = function(path, mode) {
};
fs.exists = function(path, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
if (!nullCheck(path, cb)) return;
var req = new FSReqWrap();
req.oncomplete = cb;
binding.stat(pathModule._makeLong(path), req);
function cb(err, stats) {
callback(err ? false : true);
if (callback) callback(err ? false : true);
}
};
@ -237,8 +268,8 @@ fs.existsSync = function(path) {
}
};
fs.readFile = function(path, options, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
fs.readFile = function(path, options, callback_) {
var callback = maybeCallback(arguments[arguments.length - 1]);
if (!options || typeof options === 'function') {
options = { encoding: null, flag: 'r' };
@ -570,7 +601,7 @@ Object.defineProperty(exports, '_stringToFlags', {
fs.close = function(fd, callback) {
var req = new FSReqWrap();
req.oncomplete = makeCallback(arguments[arguments.length - 1]);
req.oncomplete = makeCallback(callback);
binding.close(fd, req);
};
@ -588,8 +619,8 @@ function modeNum(m, def) {
return undefined;
}
fs.open = function(path, flags, mode, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
fs.open = function(path, flags, mode, callback_) {
var callback = makeCallback(arguments[arguments.length - 1]);
mode = modeNum(mode, 0o666);
if (!nullCheck(path, callback)) return;
@ -611,7 +642,6 @@ fs.openSync = function(path, flags, mode) {
var readWarned = false;
fs.read = function(fd, buffer, offset, length, position, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
if (!(buffer instanceof Buffer)) {
// legacy string interface (fd, length, position, encoding, callback)
readWarned = printDeprecation('fs.read\'s legacy String interface ' +
@ -635,20 +665,20 @@ fs.read = function(fd, buffer, offset, length, position, callback) {
if (bytesRead > 0) {
tryToStringWithEnd(buffer, encoding, bytesRead, cb);
} else {
cb(err, '', bytesRead);
(cb)(err, '', bytesRead);
}
};
}
if (length === 0) {
return process.nextTick(function() {
callback(null, 0, buffer);
callback && callback(null, 0, buffer);
});
}
function wrapper(err, bytesRead) {
// Retain a reference to buffer so that it can't be GC'ed too soon.
callback(err, bytesRead || 0, buffer);
callback && callback(err, bytesRead || 0, buffer);
}
var req = new FSReqWrap();
@ -712,7 +742,6 @@ fs.readSync = function(fd, buffer, offset, length, position) {
// OR
// fs.write(fd, string[, position[, encoding]], callback);
fs.write = function(fd, buffer, offset, length, position, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
function wrapper(err, written) {
// Retain a reference to buffer so that it can't be GC'ed too soon.
callback(err, written || 0, buffer);
@ -724,8 +753,10 @@ fs.write = function(fd, buffer, offset, length, position, callback) {
if (buffer instanceof Buffer) {
// if no position is passed then assume null
if (typeof position === 'function') {
callback = position;
position = null;
}
callback = maybeCallback(callback);
return binding.writeBuffer(fd, buffer, offset, length, position, req);
}
@ -740,6 +771,7 @@ fs.write = function(fd, buffer, offset, length, position, callback) {
}
length = 'utf8';
}
callback = maybeCallback(position);
return binding.writeString(fd, buffer, offset, length, req);
};
@ -761,7 +793,7 @@ fs.writeSync = function(fd, buffer, offset, length, position) {
};
fs.rename = function(oldPath, newPath, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
callback = makeCallback(callback);
if (!nullCheck(oldPath, callback)) return;
if (!nullCheck(newPath, callback)) return;
var req = new FSReqWrap();
@ -782,13 +814,14 @@ fs.truncate = function(path, len, callback) {
if (typeof path === 'number') {
return fs.ftruncate(path, len, callback);
}
callback = makeCallback(arguments[arguments.length - 1]);
if (typeof len === 'function' || len === undefined) {
if (typeof len === 'function') {
callback = len;
len = 0;
} else if (len === undefined) {
len = 0;
}
callback = maybeCallback(callback);
fs.open(path, 'r+', function(er, fd) {
if (er) return callback(er);
var req = new FSReqWrap();
@ -822,11 +855,14 @@ fs.truncateSync = function(path, len) {
};
fs.ftruncate = function(fd, len, callback) {
if (typeof len === 'function' || len === undefined) {
if (typeof len === 'function') {
callback = len;
len = 0;
} else if (len === undefined) {
len = 0;
}
var req = new FSReqWrap();
req.oncomplete = makeCallback(arguments[arguments.length - 1]);
req.oncomplete = makeCallback(callback);
binding.ftruncate(fd, len, req);
};
@ -838,7 +874,7 @@ fs.ftruncateSync = function(fd, len) {
};
fs.rmdir = function(path, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
callback = maybeCallback(callback);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -862,7 +898,7 @@ fs.fdatasyncSync = function(fd) {
fs.fsync = function(fd, callback) {
var req = new FSReqWrap();
req.oncomplete = makeCallback(arguments[arguments.length - 1]);
req.oncomplete = makeCallback(callback);
binding.fsync(fd, req);
};
@ -871,7 +907,8 @@ fs.fsyncSync = function(fd) {
};
fs.mkdir = function(path, mode, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
if (typeof mode === 'function') callback = mode;
callback = makeCallback(callback);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -887,9 +924,9 @@ fs.mkdirSync = function(path, mode) {
};
fs.readdir = function(path, options, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
options = options || {};
if (typeof options === 'function') {
callback = options;
options = {};
} else if (typeof options === 'string') {
options = {encoding: options};
@ -897,6 +934,7 @@ fs.readdir = function(path, options, callback) {
if (typeof options !== 'object')
throw new TypeError('"options" must be a string or an object');
callback = makeCallback(callback);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -915,12 +953,12 @@ fs.readdirSync = function(path, options) {
fs.fstat = function(fd, callback) {
var req = new FSReqWrap();
req.oncomplete = makeCallback(arguments[arguments.length - 1]);
req.oncomplete = makeCallback(callback);
binding.fstat(fd, req);
};
fs.lstat = function(path, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
callback = makeCallback(callback);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -928,7 +966,7 @@ fs.lstat = function(path, callback) {
};
fs.stat = function(path, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
callback = makeCallback(callback);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -950,15 +988,16 @@ fs.statSync = function(path) {
};
fs.readlink = function(path, options, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
options = options || {};
if (typeof options === 'function') {
callback = options;
options = {};
} else if (typeof options === 'string') {
options = {encoding: options};
}
if (typeof options !== 'object')
throw new TypeError('"options" must be a string or an object');
callback = makeCallback(callback);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -1018,7 +1057,7 @@ fs.symlinkSync = function(target, path, type) {
};
fs.link = function(srcpath, dstpath, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
callback = makeCallback(callback);
if (!nullCheck(srcpath, callback)) return;
if (!nullCheck(dstpath, callback)) return;
@ -1038,7 +1077,7 @@ fs.linkSync = function(srcpath, dstpath) {
};
fs.unlink = function(path, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
callback = makeCallback(callback);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -1052,7 +1091,7 @@ fs.unlinkSync = function(path) {
fs.fchmod = function(fd, mode, callback) {
var req = new FSReqWrap();
req.oncomplete = makeCallback(arguments[arguments.length - 1]);
req.oncomplete = makeCallback(callback);
binding.fchmod(fd, modeNum(mode), req);
};
@ -1062,7 +1101,7 @@ fs.fchmodSync = function(fd, mode) {
if (constants.hasOwnProperty('O_SYMLINK')) {
fs.lchmod = function(path, mode, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
callback = maybeCallback(callback);
fs.open(path, constants.O_WRONLY | constants.O_SYMLINK, function(err, fd) {
if (err) {
callback(err);
@ -1101,7 +1140,7 @@ if (constants.hasOwnProperty('O_SYMLINK')) {
fs.chmod = function(path, mode, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
callback = makeCallback(callback);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -1117,7 +1156,7 @@ fs.chmodSync = function(path, mode) {
if (constants.hasOwnProperty('O_SYMLINK')) {
fs.lchown = function(path, uid, gid, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
callback = maybeCallback(callback);
fs.open(path, constants.O_WRONLY | constants.O_SYMLINK, function(err, fd) {
if (err) {
callback(err);
@ -1135,7 +1174,7 @@ if (constants.hasOwnProperty('O_SYMLINK')) {
fs.fchown = function(fd, uid, gid, callback) {
var req = new FSReqWrap();
req.oncomplete = makeCallback(arguments[arguments.length - 1]);
req.oncomplete = makeCallback(callback);
binding.fchown(fd, uid, gid, req);
};
@ -1144,7 +1183,7 @@ fs.fchownSync = function(fd, uid, gid) {
};
fs.chown = function(path, uid, gid, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
callback = makeCallback(callback);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -1178,7 +1217,7 @@ function toUnixTimestamp(time) {
fs._toUnixTimestamp = toUnixTimestamp;
fs.utimes = function(path, atime, mtime, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
callback = makeCallback(callback);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -1196,11 +1235,10 @@ fs.utimesSync = function(path, atime, mtime) {
};
fs.futimes = function(fd, atime, mtime, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
atime = toUnixTimestamp(atime);
mtime = toUnixTimestamp(mtime);
var req = new FSReqWrap();
req.oncomplete = callback;
req.oncomplete = makeCallback(callback);
binding.futimes(fd, atime, mtime, req);
};
@ -1210,8 +1248,8 @@ fs.futimesSync = function(fd, atime, mtime) {
binding.futimes(fd, atime, mtime);
};
function writeAll(fd, isUserFd, buffer, offset, length, position, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
function writeAll(fd, isUserFd, buffer, offset, length, position, callback_) {
var callback = maybeCallback(arguments[arguments.length - 1]);
// write(fd, buffer, offset, length, position, callback)
fs.write(fd, buffer, offset, length, position, function(writeErr, written) {
@ -1242,8 +1280,8 @@ function writeAll(fd, isUserFd, buffer, offset, length, position, callback) {
});
}
fs.writeFile = function(path, data, options, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
fs.writeFile = function(path, data, options, callback_) {
var callback = maybeCallback(arguments[arguments.length - 1]);
if (!options || typeof options === 'function') {
options = { encoding: 'utf8', mode: 0o666, flag: 'w' };
@ -1314,8 +1352,8 @@ fs.writeFileSync = function(path, data, options) {
}
};
fs.appendFile = function(path, data, options, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
fs.appendFile = function(path, data, options, callback_) {
var callback = maybeCallback(arguments[arguments.length - 1]);
if (!options || typeof options === 'function') {
options = { encoding: 'utf8', mode: 0o666, flag: 'a' };
@ -1470,7 +1508,6 @@ StatWatcher.prototype.stop = function() {
const statWatchers = new Map();
fs.watchFile = function(filename, options, listener) {
listener = makeCallback(arguments[arguments.length - 1]);
nullCheck(filename);
filename = pathModule.resolve(filename);
var stat;
@ -1486,9 +1523,14 @@ fs.watchFile = function(filename, options, listener) {
if (options !== null && typeof options === 'object') {
options = util._extend(defaults, options);
} else {
listener = options;
options = defaults;
}
if (typeof listener !== 'function') {
throw new Error('"watchFile()" requires a listener function');
}
stat = statWatchers.get(filename);
if (stat === undefined) {
@ -1534,16 +1576,17 @@ fs.realpathSync = function realpathSync(path, options) {
fs.realpath = function realpath(path, options, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
if (!options) {
options = {};
} else if (typeof options === 'function') {
callback = options;
options = {};
} else if (typeof options === 'string') {
options = {encoding: options};
} else if (typeof options !== 'object') {
throw new TypeError('"options" must be a string or an object');
}
callback = makeCallback(callback);
if (!nullCheck(path, callback))
return;
var req = new FSReqWrap();
@ -1554,12 +1597,12 @@ fs.realpath = function realpath(path, options, callback) {
fs.mkdtemp = function(prefix, options, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
if (!prefix || typeof prefix !== 'string')
throw new TypeError('filename prefix is required');
options = options || {};
if (typeof options === 'function') {
callback = options;
options = {};
} else if (typeof options === 'string') {
options = {encoding: options};
@ -1567,6 +1610,7 @@ fs.mkdtemp = function(prefix, options, callback) {
if (typeof options !== 'object')
throw new TypeError('"options" must be a string or an object');
callback = makeCallback(callback);
if (!nullCheck(prefix, callback)) {
return;
}

1
test/fixtures/test-fs-readfile-error.js
View File

@ -0,0 +1 @@
require('fs').readFile('/'); // throws EISDIR

2
test/parallel/test-fs-chmod.js
View File

@ -101,7 +101,7 @@ fs.open(file2, 'a', function(err, fd) {
assert.equal(mode_sync, fs.fstatSync(fd).mode & 0o777);
}
success_count++;
fs.closeSync(fd);
fs.close(fd);
}
});
});

4
test/parallel/test-fs-link.js
View File

@ -23,14 +23,14 @@ fs.link(srcPath, dstPath, common.mustCall(callback));
assert.throws(
function() {
fs.link(undefined, undefined, () => {});
fs.link();
},
/src must be a string or Buffer/
);
assert.throws(
function() {
fs.link('abc', undefined, () => {});
fs.link('abc');
},
/dest must be a string or Buffer/
);

3
test/parallel/test-fs-make-callback.js
View File

@ -13,6 +13,9 @@ function test(cb) {
// Verify the case where a callback function is provided
assert.doesNotThrow(test(function() {}));
// Passing undefined calls rethrow() internally, which is fine
assert.doesNotThrow(test(undefined));
// Anything else should throw
assert.throws(test(null));
assert.throws(test(true));

5
test/parallel/test-fs-mkdtemp.js
View File

@ -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-'), {}));

17
test/parallel/test-fs-no-callback-errors.js
View File

@ -1,17 +0,0 @@
'use strict';
require('../common');
const fs = require('fs');
const assert = require('assert');
Object.getOwnPropertyNames(fs).filter(
(d) => !d.endsWith('Stream') && // ignore stream creation functions
!d.endsWith('Sync') && // ignore synchronous functions
typeof fs[d] === 'function' && // ignore anything other than functions
d.indexOf('_') === -1 && // ignore internal use functions
!/^[A-Z]/.test(d) && // ignore conventional constructors
d !== 'watch' && // watch's callback is optional
d !== 'unwatchFile' // unwatchFile's callback is optional
).forEach(
(d) => assert.throws(() => fs[d](), /"callback" argument must be a function/)
);

34
test/parallel/test-fs-readfile-error.js
View File

@ -0,0 +1,34 @@
'use strict';
var common = require('../common');
var assert = require('assert');
var exec = require('child_process').exec;
var path = require('path');
// `fs.readFile('/')` does not fail on FreeBSD, because you can open and read
// the directory there.
if (process.platform === 'freebsd') {
common.skip('platform not supported.');
return;
}
function test(env, cb) {
var filename = path.join(common.fixturesDir, 'test-fs-readfile-error.js');
var execPath = '"' + process.execPath + '" "' + filename + '"';
var options = { env: Object.assign(process.env, env) };
exec(execPath, options, function(err, stdout, stderr) {
assert(err);
assert.equal(stdout, '');
assert.notEqual(stderr, '');
cb('' + stderr);
});
}
test({ NODE_DEBUG: '' }, common.mustCall(function(data) {
assert(/EISDIR/.test(data));
assert(!/test-fs-readfile-error/.test(data));
}));
test({ NODE_DEBUG: 'fs' }, common.mustCall(function(data) {
assert(/EISDIR/.test(data));
assert(/test-fs-readfile-error/.test(data));
}));

4
test/parallel/test-fs-stat.js
View File

@ -28,7 +28,7 @@ fs.open('.', 'r', undefined, common.mustCall(function(err, fd) {
fs.fstat(fd, common.mustCall(function(err, stats) {
assert.ifError(err);
assert.ok(stats.mtime instanceof Date);
fs.closeSync(fd);
fs.close(fd);
assert(this === global);
}));
@ -47,7 +47,7 @@ fs.open('.', 'r', undefined, common.mustCall(function(err, fd) {
console.dir(stats);
assert.ok(stats.mtime instanceof Date);
}
fs.closeSync(fd);
fs.close(fd);
}));
console.log(`stating: ${__filename}`);

4
test/parallel/test-fs-watchfile.js
View File

@ -8,11 +8,11 @@ const assert = require('assert');
// Basic usage tests.
assert.throws(function() {
fs.watchFile('./some-file');
}, /"callback" argument must be a function/);
}, /"watchFile\(\)" requires a listener function/);
assert.throws(function() {
fs.watchFile('./another-file', {}, 'bad listener');
}, /"callback" argument must be a function/);
}, /"watchFile\(\)" requires a listener function/);
assert.throws(function() {
fs.watchFile(new Object(), function() {});

Write Preview
Loading…
Cancel
Save