fs: make callbacks run in global context

Callbacks that were passed to the binding layer ran in the context of the (internal) binding object. Make sure they run in the global context. Before: fs.symlink('a', 'b', function() { console.log(this); // prints "{ oncomplete: [Function] }" }); After: fs.symlink('a', 'b', function() { console.log(this); // prints "{ <global object> }" });
13 years ago · 463d6bac8b
2 changed files with 53 additions and 38 deletions
--- a/lib/fs.js
+++ b/lib/fs.js
@ -104,7 +104,7 @@ fs.existsSync = function(path) {
 fs.readFile = function(path, encoding_) {
  var encoding = typeof(encoding_) === 'string' ? encoding_ : null;
  var callback = arguments[arguments.length - 1];
-  if (typeof(callback) !== 'function') callback = noop;
+  if (typeof(callback) !== 'function') callback = function() {};

  // first, stat the file, so we know the size.
  var size;
@ -237,13 +237,27 @@ Object.defineProperty(exports, '_stringToFlags', {
  value: stringToFlags
 });

-function noop() {}
+
+// 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') {
+    // faster than returning a ref to a global no-op function
+    return function() {};
+  }
+
+  return function() {
+    return cb.apply(null, arguments);
+  };
+}
+

 // Yes, the follow could be easily DRYed up but I provide the explicit
 // list to make the arguments clear.

 fs.close = function(fd, callback) {
-  binding.close(fd, callback || noop);
+  binding.close(fd, makeCallback(callback));
 };

 fs.closeSync = function(fd) {
@ -264,11 +278,7 @@ function modeNum(m, def) {
 }

 fs.open = function(path, flags, mode, callback) {
-  callback = arguments[arguments.length - 1];
-  if (typeof(callback) !== 'function') {
-    callback = noop;
-  }
-
+  callback = makeCallback(arguments[arguments.length - 1]);
  mode = modeNum(mode, 438 /*=0666*/);

  binding.open(pathModule._makeLong(path),
@ -376,7 +386,7 @@ fs.writeSync = function(fd, buffer, offset, length, position) {
 fs.rename = function(oldPath, newPath, callback) {
  binding.rename(pathModule._makeLong(oldPath),
                 pathModule._makeLong(newPath),
-                 callback || noop);
+                 makeCallback(callback));
 };

 fs.renameSync = function(oldPath, newPath) {
@ -385,7 +395,7 @@ fs.renameSync = function(oldPath, newPath) {
 };

 fs.truncate = function(fd, len, callback) {
-  binding.truncate(fd, len, callback || noop);
+  binding.truncate(fd, len, makeCallback(callback));
 };

 fs.truncateSync = function(fd, len) {
@ -393,7 +403,7 @@ fs.truncateSync = function(fd, len) {
 };

 fs.rmdir = function(path, callback) {
-  binding.rmdir(pathModule._makeLong(path), callback || noop);
+  binding.rmdir(pathModule._makeLong(path), makeCallback(callback));
 };

 fs.rmdirSync = function(path) {
@ -401,7 +411,7 @@ fs.rmdirSync = function(path) {
 };

 fs.fdatasync = function(fd, callback) {
-  binding.fdatasync(fd, callback || noop);
+  binding.fdatasync(fd, makeCallback(callback));
 };

 fs.fdatasyncSync = function(fd) {
@ -409,7 +419,7 @@ fs.fdatasyncSync = function(fd) {
 };

 fs.fsync = function(fd, callback) {
-  binding.fsync(fd, callback || noop);
+  binding.fsync(fd, makeCallback(callback));
 };

 fs.fsyncSync = function(fd) {
@ -418,8 +428,9 @@ fs.fsyncSync = function(fd) {

 fs.mkdir = function(path, mode, callback) {
  if (typeof mode === 'function') callback = mode;
-  binding.mkdir(pathModule._makeLong(path), modeNum(mode, 511 /*=0777*/),
-                callback || noop);
+  binding.mkdir(pathModule._makeLong(path),
+                modeNum(mode, 511 /*=0777*/),
+                makeCallback(callback));
 };

 fs.mkdirSync = function(path, mode) {
@ -428,7 +439,7 @@ fs.mkdirSync = function(path, mode) {
 };

 fs.sendfile = function(outFd, inFd, inOffset, length, callback) {
-  binding.sendfile(outFd, inFd, inOffset, length, callback || noop);
+  binding.sendfile(outFd, inFd, inOffset, length, makeCallback(callback));
 };

 fs.sendfileSync = function(outFd, inFd, inOffset, length) {
@ -436,7 +447,7 @@ fs.sendfileSync = function(outFd, inFd, inOffset, length) {
 };

 fs.readdir = function(path, callback) {
-  binding.readdir(pathModule._makeLong(path), callback || noop);
+  binding.readdir(pathModule._makeLong(path), makeCallback(callback));
 };

 fs.readdirSync = function(path) {
@ -444,15 +455,15 @@ fs.readdirSync = function(path) {
 };

 fs.fstat = function(fd, callback) {
-  binding.fstat(fd, callback || noop);
+  binding.fstat(fd, makeCallback(callback));
 };

 fs.lstat = function(path, callback) {
-  binding.lstat(pathModule._makeLong(path), callback || noop);
+  binding.lstat(pathModule._makeLong(path), makeCallback(callback));
 };

 fs.stat = function(path, callback) {
-  binding.stat(pathModule._makeLong(path), callback || noop);
+  binding.stat(pathModule._makeLong(path), makeCallback(callback));
 };

 fs.fstatSync = function(fd) {
@ -468,7 +479,7 @@ fs.statSync = function(path) {
 };

 fs.readlink = function(path, callback) {
-  binding.readlink(pathModule._makeLong(path), callback || noop);
+  binding.readlink(pathModule._makeLong(path), makeCallback(callback));
 };

 fs.readlinkSync = function(path) {
@ -477,8 +488,7 @@ fs.readlinkSync = function(path) {

 fs.symlink = function(destination, path, type_, callback) {
  var type = (typeof(type_) == 'string' ? type_ : null);
-  var callback_ = arguments[arguments.length - 1];
-  callback = (typeof(callback_) == 'function' ? callback_ : noop);
+  var callback = makeCallback(arguments[arguments.length - 1]);

  if (isWindows && type === 'junction') {
    destination = pathModule._makeLong(destination);
@ -500,7 +510,7 @@ fs.symlinkSync = function(destination, path, type) {
 fs.link = function(srcpath, dstpath, callback) {
  binding.link(pathModule._makeLong(srcpath),
               pathModule._makeLong(dstpath),
-               callback || noop);
+               makeCallback(callback));
 };

 fs.linkSync = function(srcpath, dstpath) {
@ -509,7 +519,7 @@ fs.linkSync = function(srcpath, dstpath) {
 };

 fs.unlink = function(path, callback) {
-  binding.unlink(pathModule._makeLong(path), callback || noop);
+  binding.unlink(pathModule._makeLong(path), makeCallback(callback));
 };

 fs.unlinkSync = function(path) {
@ -517,7 +527,7 @@ fs.unlinkSync = function(path) {
 };

 fs.fchmod = function(fd, mode, callback) {
-  binding.fchmod(fd, modeNum(mode), callback || noop);
+  binding.fchmod(fd, modeNum(mode), makeCallback(callback));
 };

 fs.fchmodSync = function(fd, mode) {
@ -526,7 +536,7 @@ fs.fchmodSync = function(fd, mode) {

 if (constants.hasOwnProperty('O_SYMLINK')) {
  fs.lchmod = function(path, mode, callback) {
-    callback = callback || noop;
+    callback = callback || (function() {});
    fs.open(path, constants.O_WRONLY | constants.O_SYMLINK, function(err, fd) {
      if (err) {
        callback(err);
@ -565,7 +575,9 @@ if (constants.hasOwnProperty('O_SYMLINK')) {


 fs.chmod = function(path, mode, callback) {
-  binding.chmod(pathModule._makeLong(path), modeNum(mode), callback || noop);
+  binding.chmod(pathModule._makeLong(path),
+                modeNum(mode),
+                makeCallback(callback));
 };

 fs.chmodSync = function(path, mode) {
@ -574,7 +586,7 @@ fs.chmodSync = function(path, mode) {

 if (constants.hasOwnProperty('O_SYMLINK')) {
  fs.lchown = function(path, uid, gid, callback) {
-    callback = callback || noop;
+    callback = callback || (function() {});
    fs.open(path, constants.O_WRONLY | constants.O_SYMLINK, function(err, fd) {
      if (err) {
        callback(err);
@ -591,7 +603,7 @@ if (constants.hasOwnProperty('O_SYMLINK')) {
 }

 fs.fchown = function(fd, uid, gid, callback) {
-  binding.fchown(fd, uid, gid, callback || noop);
+  binding.fchown(fd, uid, gid, makeCallback(callback));
 };

 fs.fchownSync = function(fd, uid, gid) {
@ -599,7 +611,7 @@ fs.fchownSync = function(fd, uid, gid) {
 };

 fs.chown = function(path, uid, gid, callback) {
-  binding.chown(pathModule._makeLong(path), uid, gid, callback || noop);
+  binding.chown(pathModule._makeLong(path), uid, gid, makeCallback(callback));
 };

 fs.chownSync = function(path, uid, gid) {
@ -622,9 +634,10 @@ function toUnixTimestamp(time) {
 fs._toUnixTimestamp = toUnixTimestamp;

 fs.utimes = function(path, atime, mtime, callback) {
-  atime = toUnixTimestamp(atime);
-  mtime = toUnixTimestamp(mtime);
-  binding.utimes(pathModule._makeLong(path), atime, mtime, callback || noop);
+  binding.utimes(pathModule._makeLong(path),
+                 toUnixTimestamp(atime),
+                 toUnixTimestamp(mtime),
+                 makeCallback(callback));
 };

 fs.utimesSync = function(path, atime, mtime) {
@ -636,7 +649,7 @@ fs.utimesSync = function(path, atime, mtime) {
 fs.futimes = function(fd, atime, mtime, callback) {
  atime = toUnixTimestamp(atime);
  mtime = toUnixTimestamp(mtime);
-  binding.futimes(fd, atime, mtime, callback || noop);
+  binding.futimes(fd, atime, mtime, makeCallback(callback));
 };

 fs.futimesSync = function(fd, atime, mtime) {
--- a/test/simple/test-fs-stat.js
+++ b/test/simple/test-fs-stat.js
@ -19,9 +19,6 @@
 // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
 // USE OR OTHER DEALINGS IN THE SOFTWARE.

-
-
-
 var common = require('../common');
 var assert = require('assert');
 var fs = require('fs');
@ -36,6 +33,7 @@ fs.stat('.', function(err, stats) {
    assert.ok(stats.mtime instanceof Date);
    success_count++;
  }
+  assert(this === global);
 });

 fs.lstat('.', function(err, stats) {
@ -46,6 +44,7 @@ fs.lstat('.', function(err, stats) {
    assert.ok(stats.mtime instanceof Date);
    success_count++;
  }
+  assert(this === global);
 });

 // fstat
@ -62,7 +61,10 @@ fs.open('.', 'r', undefined, function(err, fd) {
      success_count++;
      fs.close(fd);
    }
+    assert(this === global);
  });
+
+  assert(this === global);
 });

 // fstatSync