From a087ed9b72f71901e02a0070b9ab8d19c8765d0e Mon Sep 17 00:00:00 2001 From: Sakthipriyan Vairamani Date: Tue, 2 Jun 2015 20:43:46 +0530 Subject: [PATCH 1/5] fs: better error messages 1. Changing `Bad arguments` error messages to a more helpful message `options should either be an object or a string`. 2. Made braces consistent. 3. Returning meaningful error message from `fs_event_wrap`'s `FSEvent`'s `Start` function. --- lib/fs.js | 23 ++++++++++++++--------- src/fs_event_wrap.cc | 2 +- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 4cdd0ef5e31456..6ecdb627b822e9 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -35,6 +35,10 @@ const isWindows = process.platform === 'win32'; const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG); const errnoException = util._errnoException; +function throwOptionsError(options) { + throw new TypeError('Expected options to be either an object or a string, ' + + 'but got ' + typeof options + ' instead'); +} function rethrow() { // Only enable in debug mode. A backtrace uses ~1000 bytes of heap space and @@ -226,12 +230,13 @@ fs.existsSync = function(path) { fs.readFile = function(path, options, callback_) { var callback = maybeCallback(arguments[arguments.length - 1]); - if (!options || typeof options === 'function') + if (!options || typeof options === 'function') { options = { encoding: null, flag: 'r' }; - else if (typeof options === 'string') + } else if (typeof options === 'string') { options = { encoding: options, flag: 'r' }; - else if (typeof options !== 'object') - throw new TypeError('Bad arguments'); + } else if (typeof options !== 'object') { + throwOptionsError(options); + } var encoding = options.encoding; assertEncoding(encoding); @@ -389,7 +394,7 @@ fs.readFileSync = function(path, options) { } else if (typeof options === 'string') { options = { encoding: options, flag: 'r' }; } else if (typeof options !== 'object') { - throw new TypeError('Bad arguments'); + throwOptionsError(options); } var encoding = options.encoding; @@ -1119,7 +1124,7 @@ fs.writeFile = function(path, data, options, callback) { } else if (typeof options === 'string') { options = { encoding: options, mode: 0o666, flag: 'w' }; } else if (typeof options !== 'object') { - throw new TypeError('Bad arguments'); + throwOptionsError(options); } assertEncoding(options.encoding); @@ -1143,7 +1148,7 @@ fs.writeFileSync = function(path, data, options) { } else if (typeof options === 'string') { options = { encoding: options, mode: 0o666, flag: 'w' }; } else if (typeof options !== 'object') { - throw new TypeError('Bad arguments'); + throwOptionsError(options); } assertEncoding(options.encoding); @@ -1178,7 +1183,7 @@ fs.appendFile = function(path, data, options, callback_) { } else if (typeof options === 'string') { options = { encoding: options, mode: 0o666, flag: 'a' }; } else if (typeof options !== 'object') { - throw new TypeError('Bad arguments'); + throwOptionsError(options); } if (!options.flag) @@ -1192,7 +1197,7 @@ fs.appendFileSync = function(path, data, options) { } else if (typeof options === 'string') { options = { encoding: options, mode: 0o666, flag: 'a' }; } else if (typeof options !== 'object') { - throw new TypeError('Bad arguments'); + throwOptionsError(options); } if (!options.flag) options = util._extend({ flag: 'a' }, options); diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc index 1719942ce3e099..a6ceff2776db92 100644 --- a/src/fs_event_wrap.cc +++ b/src/fs_event_wrap.cc @@ -86,7 +86,7 @@ void FSEventWrap::Start(const FunctionCallbackInfo& args) { FSEventWrap* wrap = Unwrap(args.Holder()); if (args.Length() < 1 || !args[0]->IsString()) { - return env->ThrowTypeError("Bad arguments"); + return env->ThrowTypeError("filename must be a valid string"); } node::Utf8Value path(env->isolate(), args[0]); From f53c0829eab3ffd820c70491ffd1a48990c65a08 Mon Sep 17 00:00:00 2001 From: Sakthipriyan Vairamani Date: Tue, 2 Jun 2015 20:45:21 +0530 Subject: [PATCH 2/5] fs: removing unnecessary nullCheckCallNT `nullCheckCallNT` function is not necessary, as we can directly pass `callback` and `er` to `process.nextTick` --- lib/fs.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 6ecdb627b822e9..7ccf44b350517c 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -95,16 +95,12 @@ function nullCheck(path, callback) { er.code = 'ENOENT'; if (typeof callback !== 'function') throw er; - process.nextTick(nullCheckCallNT, callback, er); + process.nextTick(callback, er); return false; } return true; } -function nullCheckCallNT(callback, er) { - callback(er); -} - // Static method to set the stats properties on a Stats object. fs.Stats = function( dev, From 73d011e221c5e876d5751aa7d3dbe6a8cbeaa6aa Mon Sep 17 00:00:00 2001 From: Sakthipriyan Vairamani Date: Tue, 2 Jun 2015 22:32:15 +0530 Subject: [PATCH 3/5] fs: Removing inStatWatchers & using Map for lookup We are getting rid of `inStatWatchers` function and making `statWatchers` a `Map`. --- lib/fs.js | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 7ccf44b350517c..26fec55b47d6e5 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1301,12 +1301,7 @@ StatWatcher.prototype.stop = function() { }; -var statWatchers = {}; -function inStatWatchers(filename) { - return Object.prototype.hasOwnProperty.call(statWatchers, filename) && - statWatchers[filename]; -} - +const statWatchers = new Map(); fs.watchFile = function(filename) { nullCheck(filename); @@ -1333,12 +1328,14 @@ fs.watchFile = function(filename) { throw new Error('watchFile requires a listener function'); } - if (inStatWatchers(filename)) { - stat = statWatchers[filename]; - } else { - stat = statWatchers[filename] = new StatWatcher(); + stat = statWatchers.get(filename); + + if (stat === undefined) { + stat = new StatWatcher(); stat.start(filename, options.persistent, options.interval); + statWatchers.set(filename, stat); } + stat.addListener('change', listener); return stat; }; @@ -1346,9 +1343,9 @@ fs.watchFile = function(filename) { fs.unwatchFile = function(filename, listener) { nullCheck(filename); filename = pathModule.resolve(filename); - if (!inStatWatchers(filename)) return; + var stat = statWatchers.get(filename); - var stat = statWatchers[filename]; + if (stat === undefined) return; if (typeof listener === 'function') { stat.removeListener('change', listener); @@ -1358,7 +1355,7 @@ fs.unwatchFile = function(filename, listener) { if (EventEmitter.listenerCount(stat, 'change') === 0) { stat.stop(); - statWatchers[filename] = undefined; + statWatchers.delete(filename); } }; From 08f2691bcdd6a8b282062cc58b7d91615b6f1bef Mon Sep 17 00:00:00 2001 From: Sakthipriyan Vairamani Date: Wed, 3 Jun 2015 03:41:04 +0530 Subject: [PATCH 4/5] fs: minor refactoring 1. Removed a few unnecessary variables to reduce LoC 2. Removed redundant `var` definitions of variables in same function 3. Refactored variables which are defined inside a block and used outside as well 4. Refactored effect-less code 5. In `rethrow` function, instead of assigning to `err` and throwing `err`, we can directly throw `backtrace` object itself. 6. Reassigning a defined parameter while also mentioning arguments in the body is one of the optimization killers. So, changing `callback` to `callback_` and declaring a new variable called `callback` in the body. --- lib/fs.js | 57 ++++++++++++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 26fec55b47d6e5..90554eab484a9a 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -49,8 +49,7 @@ function rethrow() { if (err) { backtrace.stack = err.name + ': ' + err.message + backtrace.stack.substr(backtrace.name.length); - err = backtrace; - throw err; + throw backtrace; } }; } @@ -267,27 +266,25 @@ function ReadFileContext(callback, encoding) { } ReadFileContext.prototype.read = function() { - var fd = this.fd; - var size = this.size; var buffer; var offset; var length; - if (size === 0) { + if (this.size === 0) { buffer = this.buffer = new SlowBuffer(kReadFileBufferLength); offset = 0; length = kReadFileBufferLength; } else { buffer = this.buffer; offset = this.pos; - length = size - this.pos; + length = this.size - this.pos; } var req = new FSReqWrap(); req.oncomplete = readFileAfterRead; req.context = this; - binding.read(fd, buffer, offset, length, -1, req); + binding.read(this.fd, buffer, offset, length, -1, req); }; ReadFileContext.prototype.close = function(err) { @@ -302,8 +299,7 @@ function readFileAfterOpen(err, fd) { var context = this.context; if (err) { - var callback = context.callback; - callback(err); + context.callback(err); return; } @@ -417,7 +413,7 @@ fs.readFileSync = function(path, options) { if (size === 0) { buffers = []; } else { - var threw = true; + threw = true; try { buffer = new Buffer(size); threw = false; @@ -427,16 +423,18 @@ fs.readFileSync = function(path, options) { } var done = false; + var bytesRead; + while (!done) { - var threw = true; + threw = true; try { if (size !== 0) { - var bytesRead = fs.readSync(fd, buffer, pos, size - pos); + bytesRead = fs.readSync(fd, buffer, pos, size - pos); } else { // the kernel lies about many files. // Go ahead and try to read some bytes. buffer = new Buffer(8192); - var bytesRead = fs.readSync(fd, buffer, 0, 8192); + bytesRead = fs.readSync(fd, buffer, 0, 8192); if (bytesRead) { buffers.push(buffer.slice(0, bytesRead)); } @@ -529,8 +527,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; @@ -585,10 +583,12 @@ fs.read = function(fd, buffer, offset, length, position, callback) { fs.readSync = function(fd, buffer, offset, length, position) { var legacy = false; + var encoding; + if (!(buffer instanceof Buffer)) { // legacy string interface (fd, length, position, encoding, callback) legacy = true; - var encoding = arguments[3]; + encoding = arguments[3]; assertEncoding(encoding); @@ -623,6 +623,7 @@ fs.write = function(fd, buffer, offset, length, position, callback) { callback(err, written || 0, buffer); } + var req = new FSReqWrap(); if (buffer instanceof Buffer) { // if no position is passed then assume null if (typeof position === 'function') { @@ -630,7 +631,6 @@ fs.write = function(fd, buffer, offset, length, position, callback) { position = null; } callback = maybeCallback(callback); - var req = new FSReqWrap(); req.oncomplete = strWrapper; return binding.writeBuffer(fd, buffer, offset, length, position, req); } @@ -647,7 +647,6 @@ fs.write = function(fd, buffer, offset, length, position, callback) { length = 'utf8'; } callback = maybeCallback(position); - var req = new FSReqWrap(); req.oncomplete = bufWrapper; return binding.writeString(fd, buffer, offset, length, req); }; @@ -721,8 +720,10 @@ fs.truncateSync = function(path, len) { } // allow error to be thrown, but still close fd. var fd = fs.openSync(path, 'r+'); + var ret; + try { - var ret = fs.ftruncateSync(fd, len); + ret = fs.ftruncateSync(fd, len); } finally { fs.closeSync(fd); } @@ -875,7 +876,7 @@ function preprocessSymlinkDestination(path, type, linkPath) { } } -fs.symlink = function(destination, path, type_, callback) { +fs.symlink = function(destination, path, type_, callback_) { var type = (typeof type_ === 'string' ? type_ : null); var callback = makeCallback(arguments[arguments.length - 1]); @@ -968,9 +969,9 @@ if (constants.hasOwnProperty('O_SYMLINK')) { // prefer to return the chmod error, if one occurs, // but still try to close, and report closing errors if they occur. - var err, err2; + var err, err2, ret; try { - var ret = fs.fchmodSync(fd, mode); + ret = fs.fchmodSync(fd, mode); } catch (er) { err = er; } @@ -1088,8 +1089,8 @@ fs.futimesSync = function(fd, atime, mtime) { binding.futimes(fd, atime, mtime); }; -function writeAll(fd, buffer, offset, length, position, callback) { - callback = maybeCallback(arguments[arguments.length - 1]); +function writeAll(fd, 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) { @@ -1112,7 +1113,7 @@ function writeAll(fd, buffer, offset, length, position, callback) { }); } -fs.writeFile = function(path, data, options, callback) { +fs.writeFile = function(path, data, options, callback_) { var callback = maybeCallback(arguments[arguments.length - 1]); if (!options || typeof options === 'function') { @@ -1627,8 +1628,8 @@ function ReadStream(path, options) { this.flags = options.flags === undefined ? 'r' : options.flags; this.mode = options.mode === undefined ? 0o666 : options.mode; - this.start = options.start === undefined ? undefined : options.start; - this.end = options.end === undefined ? undefined : options.end; + this.start = options.start; + this.end = options.end; this.autoClose = options.autoClose === undefined ? true : options.autoClose; this.pos = undefined; @@ -1790,7 +1791,7 @@ function WriteStream(path, options) { this.flags = options.flags === undefined ? 'w' : options.flags; this.mode = options.mode === undefined ? 0o666 : options.mode; - this.start = options.start === undefined ? undefined : options.start; + this.start = options.start; this.pos = undefined; this.bytesWritten = 0; From 365bc7255fd100f5f44095dbcca34632d0a0eb85 Mon Sep 17 00:00:00 2001 From: Sakthipriyan Vairamani Date: Wed, 3 Jun 2015 20:27:44 +0530 Subject: [PATCH 5/5] fs: Making SyncWriteStream non-enumerable As `SyncWriteStream` is only for internal use, it would be better if it is non-enumerable, so that a simple `console.log(require('fs'))` will not reveal it. --- lib/fs.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 90554eab484a9a..60c74c4a10a256 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1878,8 +1878,11 @@ util.inherits(SyncWriteStream, Stream); // Export -fs.SyncWriteStream = SyncWriteStream; - +Object.defineProperty(fs, 'SyncWriteStream', { + configurable: true, + writable: true, + value: SyncWriteStream +}); SyncWriteStream.prototype.write = function(data, arg1, arg2) { var encoding, cb;