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> }" });
nodejs · Jun 6, 2012 · 463d6ba · 463d6ba
1 parent c381662
commit 463d6ba
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 38 deletions.
diff --git 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,31 +395,31 @@ 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) {
   return binding.truncate(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) {
   return binding.rmdir(pathModule._makeLong(path));
 };
 
 fs.fdatasync = function(fd, callback) {
-  binding.fdatasync(fd, callback || noop);
+  binding.fdatasync(fd, makeCallback(callback));
 };
 
 fs.fdatasyncSync = function(fd) {
   return binding.fdatasync(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,31 +439,31 @@ 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) {
   return binding.sendfile(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) {
   return binding.readdir(pathModule._makeLong(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,15 +519,15 @@ 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) {
   return binding.unlink(pathModule._makeLong(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,15 +603,15 @@ 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) {
   return binding.fchown(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) {

diff --git 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