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

fs: make callback mandatory to all async functions 

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>
v7.x
Sakthipriyan Vairamani 9 years ago
parent
9983af0347
commit
9359de9dd2
10 changed files with 73 additions and 143 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,7 +38,6 @@ 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;
@ -82,39 +81,10 @@ 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');
}
@ -221,11 +191,9 @@ 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))
@ -233,7 +201,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);
};
@ -249,12 +217,13 @@ 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) {
if (callback) callback(err ? false : true);
callback(err ? false : true);
}
};
@ -268,8 +237,8 @@ fs.existsSync = function(path) {
}
};
fs.readFile = function(path, options, callback_) {
var callback = maybeCallback(arguments[arguments.length - 1]);
fs.readFile = function(path, options, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
if (!options || typeof options === 'function') {
options = { encoding: null, flag: 'r' };
@ -601,7 +570,7 @@ Object.defineProperty(exports, '_stringToFlags', {
fs.close = function(fd, callback) {
var req = new FSReqWrap();
req.oncomplete = makeCallback(callback);
req.oncomplete = makeCallback(arguments[arguments.length - 1]);
binding.close(fd, req);
};
@ -619,8 +588,8 @@ function modeNum(m, def) {
return undefined;
}
fs.open = function(path, flags, mode, callback_) {
var callback = makeCallback(arguments[arguments.length - 1]);
fs.open = function(path, flags, mode, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
mode = modeNum(mode, 0o666);
if (!nullCheck(path, callback)) return;
@ -642,6 +611,7 @@ 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 ' +
@ -665,20 +635,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 && callback(null, 0, buffer);
callback(null, 0, buffer);
});
}
function wrapper(err, bytesRead) {
// Retain a reference to buffer so that it can't be GC'ed too soon.
callback && callback(err, bytesRead || 0, buffer);
callback(err, bytesRead || 0, buffer);
}
var req = new FSReqWrap();
@ -742,6 +712,7 @@ 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);
@ -753,10 +724,8 @@ 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);
}
@ -771,7 +740,6 @@ fs.write = function(fd, buffer, offset, length, position, callback) {
}
length = 'utf8';
}
callback = maybeCallback(position);
return binding.writeString(fd, buffer, offset, length, req);
};
@ -793,7 +761,7 @@ fs.writeSync = function(fd, buffer, offset, length, position) {
};
fs.rename = function(oldPath, newPath, callback) {
callback = makeCallback(callback);
callback = makeCallback(arguments[arguments.length - 1]);
if (!nullCheck(oldPath, callback)) return;
if (!nullCheck(newPath, callback)) return;
var req = new FSReqWrap();
@ -814,14 +782,13 @@ fs.truncate = function(path, len, callback) {
if (typeof path === 'number') {
return fs.ftruncate(path, len, callback);
}
if (typeof len === 'function') {
callback = len;
len = 0;
} else if (len === undefined) {
callback = makeCallback(arguments[arguments.length - 1]);
if (typeof len === 'function' || len === undefined) {
len = 0;
}
callback = maybeCallback(callback);
fs.open(path, 'r+', function(er, fd) {
if (er) return callback(er);
var req = new FSReqWrap();
@ -855,14 +822,11 @@ fs.truncateSync = function(path, len) {
};
fs.ftruncate = function(fd, len, callback) {
if (typeof len === 'function') {
callback = len;
len = 0;
} else if (len === undefined) {
if (typeof len === 'function' || len === undefined) {
len = 0;
}
var req = new FSReqWrap();
req.oncomplete = makeCallback(callback);
req.oncomplete = makeCallback(arguments[arguments.length - 1]);
binding.ftruncate(fd, len, req);
};
@ -874,7 +838,7 @@ fs.ftruncateSync = function(fd, len) {
};
fs.rmdir = function(path, callback) {
callback = maybeCallback(callback);
callback = makeCallback(arguments[arguments.length - 1]);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -898,7 +862,7 @@ fs.fdatasyncSync = function(fd) {
fs.fsync = function(fd, callback) {
var req = new FSReqWrap();
req.oncomplete = makeCallback(callback);
req.oncomplete = makeCallback(arguments[arguments.length - 1]);
binding.fsync(fd, req);
};
@ -907,8 +871,7 @@ fs.fsyncSync = function(fd) {
};
fs.mkdir = function(path, mode, callback) {
if (typeof mode === 'function') callback = mode;
callback = makeCallback(callback);
callback = makeCallback(arguments[arguments.length - 1]);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -924,9 +887,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};
@ -934,7 +897,6 @@ 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;
@ -953,12 +915,12 @@ fs.readdirSync = function(path, options) {
fs.fstat = function(fd, callback) {
var req = new FSReqWrap();
req.oncomplete = makeCallback(callback);
req.oncomplete = makeCallback(arguments[arguments.length - 1]);
binding.fstat(fd, req);
};
fs.lstat = function(path, callback) {
callback = makeCallback(callback);
callback = makeCallback(arguments[arguments.length - 1]);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -966,7 +928,7 @@ fs.lstat = function(path, callback) {
};
fs.stat = function(path, callback) {
callback = makeCallback(callback);
callback = makeCallback(arguments[arguments.length - 1]);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -988,16 +950,15 @@ 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;
@ -1057,7 +1018,7 @@ fs.symlinkSync = function(target, path, type) {
};
fs.link = function(srcpath, dstpath, callback) {
callback = makeCallback(callback);
callback = makeCallback(arguments[arguments.length - 1]);
if (!nullCheck(srcpath, callback)) return;
if (!nullCheck(dstpath, callback)) return;
@ -1077,7 +1038,7 @@ fs.linkSync = function(srcpath, dstpath) {
};
fs.unlink = function(path, callback) {
callback = makeCallback(callback);
callback = makeCallback(arguments[arguments.length - 1]);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -1091,7 +1052,7 @@ fs.unlinkSync = function(path) {
fs.fchmod = function(fd, mode, callback) {
var req = new FSReqWrap();
req.oncomplete = makeCallback(callback);
req.oncomplete = makeCallback(arguments[arguments.length - 1]);
binding.fchmod(fd, modeNum(mode), req);
};
@ -1101,7 +1062,7 @@ fs.fchmodSync = function(fd, mode) {
if (constants.hasOwnProperty('O_SYMLINK')) {
fs.lchmod = function(path, mode, callback) {
callback = maybeCallback(callback);
callback = makeCallback(arguments[arguments.length - 1]);
fs.open(path, constants.O_WRONLY | constants.O_SYMLINK, function(err, fd) {
if (err) {
callback(err);
@ -1140,7 +1101,7 @@ if (constants.hasOwnProperty('O_SYMLINK')) {
fs.chmod = function(path, mode, callback) {
callback = makeCallback(callback);
callback = makeCallback(arguments[arguments.length - 1]);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -1156,7 +1117,7 @@ fs.chmodSync = function(path, mode) {
if (constants.hasOwnProperty('O_SYMLINK')) {
fs.lchown = function(path, uid, gid, callback) {
callback = maybeCallback(callback);
callback = makeCallback(arguments[arguments.length - 1]);
fs.open(path, constants.O_WRONLY | constants.O_SYMLINK, function(err, fd) {
if (err) {
callback(err);
@ -1174,7 +1135,7 @@ if (constants.hasOwnProperty('O_SYMLINK')) {
fs.fchown = function(fd, uid, gid, callback) {
var req = new FSReqWrap();
req.oncomplete = makeCallback(callback);
req.oncomplete = makeCallback(arguments[arguments.length - 1]);
binding.fchown(fd, uid, gid, req);
};
@ -1183,7 +1144,7 @@ fs.fchownSync = function(fd, uid, gid) {
};
fs.chown = function(path, uid, gid, callback) {
callback = makeCallback(callback);
callback = makeCallback(arguments[arguments.length - 1]);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -1217,7 +1178,7 @@ function toUnixTimestamp(time) {
fs._toUnixTimestamp = toUnixTimestamp;
fs.utimes = function(path, atime, mtime, callback) {
callback = makeCallback(callback);
callback = makeCallback(arguments[arguments.length - 1]);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
@ -1235,10 +1196,11 @@ 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 = makeCallback(callback);
req.oncomplete = callback;
binding.futimes(fd, atime, mtime, req);
};
@ -1248,8 +1210,8 @@ fs.futimesSync = function(fd, atime, mtime) {
binding.futimes(fd, atime, mtime);
};
function writeAll(fd, isUserFd, buffer, offset, length, position, callback_) {
var callback = maybeCallback(arguments[arguments.length - 1]);
function writeAll(fd, isUserFd, buffer, offset, length, position, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
// write(fd, buffer, offset, length, position, callback)
fs.write(fd, buffer, offset, length, position, function(writeErr, written) {
@ -1280,8 +1242,8 @@ function writeAll(fd, isUserFd, buffer, offset, length, position, callback_) {
});
}
fs.writeFile = function(path, data, options, callback_) {
var callback = maybeCallback(arguments[arguments.length - 1]);
fs.writeFile = function(path, data, options, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
if (!options || typeof options === 'function') {
options = { encoding: 'utf8', mode: 0o666, flag: 'w' };
@ -1352,8 +1314,8 @@ fs.writeFileSync = function(path, data, options) {
}
};
fs.appendFile = function(path, data, options, callback_) {
var callback = maybeCallback(arguments[arguments.length - 1]);
fs.appendFile = function(path, data, options, callback) {
callback = makeCallback(arguments[arguments.length - 1]);
if (!options || typeof options === 'function') {
options = { encoding: 'utf8', mode: 0o666, flag: 'a' };
@ -1508,6 +1470,7 @@ 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;
@ -1523,14 +1486,9 @@ 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) {
@ -1576,17 +1534,16 @@ 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();
@ -1597,12 +1554,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};
@ -1610,7 +1567,6 @@ 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

@ -1 +0,0 @@
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.close(fd);
fs.closeSync(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();
fs.link(undefined, undefined, () => {});
},
/src must be a string or Buffer/
);
assert.throws(
function() {
fs.link('abc');
fs.link('abc', undefined, () => {});
},
/dest must be a string or Buffer/
);

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

@ -13,9 +13,6 @@ 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,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-'), {}));

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

@ -0,0 +1,17 @@
'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

@ -1,34 +0,0 @@
'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.close(fd);
fs.closeSync(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.close(fd);
fs.closeSync(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');
}, /"watchFile\(\)" requires a listener function/);
}, /"callback" argument must be a function/);
assert.throws(function() {
fs.watchFile('./another-file', {}, 'bad listener');
}, /"watchFile\(\)" requires a listener function/);
}, /"callback" argument must be a function/);
assert.throws(function() {
fs.watchFile(new Object(), function() {});

Write Preview
Loading…
Cancel
Save