Repository: filerjs/filer (js)

Issue:
File descriptors are not allocated safely
The algorithm in use is pretty dumb (take the next highest number), which can run out for long-lived applications that open lots of files.

First step here is to write a test case that exhibits the failure.


Pull request:
Refactor fs arg, open files management
This refactors how the file descriptors get managed.  My motivation is to move this off of the `fs` instance, and into its own private module.  I want to eventually be able to refactor `implementation.js` into separate modules per function, and will need to be able to get access to the open files without needing the `fs` instance.  This also fixes #28 by reusing old descriptors.

Patch:
diff --git a/src/filesystem/implementation.js b/src/filesystem/implementation.js
index f609da2..d1b5eca 100644
--- a/src/filesystem/implementation.js
+++ b/src/filesystem/implementation.js
@@ -33,6 +33,7 @@ var FS_NOCTIME = Constants.FS_NOCTIME;
 var Encoding = require('../encoding.js');
 var Errors = require('../errors.js');
 var DirectoryEntry = require('../directory-entry.js');
+var openFiles = require('../open-files.js');
 var OpenFileDescription = require('../open-file-description.js');
 var SuperNode = require('../super-node.js');
 var Node = require('../node.js');
@@ -1650,8 +1651,8 @@ function pathCheck(path, allowRelative, callback) {
 }
 
 
-function open(fs, context, path, flags, mode, callback) {
-  if (arguments.length < 6 ){
+function open(context, path, flags, mode, callback) {
+  if (arguments.length < 5 ){
     callback = arguments[arguments.length - 1];
     mode = 0o644;
   }
@@ -1672,7 +1673,7 @@ function open(fs, context, path, flags, mode, callback) {
         position = 0;
       }
       var openFileDescription = new OpenFileDescription(path, fileNode.id, flags, position);
-      var fd = fs.allocDescriptor(openFileDescription);
+      var fd = openFiles.allocDescriptor(openFileDescription);
       callback(null, fd);
     }
   }
@@ -1685,22 +1686,22 @@ function open(fs, context, path, flags, mode, callback) {
   open_file(context, path, flags, mode, check_result);
 }
 
-function close(fs, context, fd, callback) {
-  if(!fs.openFiles[fd]) {
+function close(context, fd, callback) {
+  if(!openFiles.getOpenFileDescription(fd)) {
     callback(new Errors.EBADF());
   } else {
-    fs.releaseDescriptor(fd);
+    openFiles.releaseDescriptor(fd);
     callback(null);
   }
 }
 
-function mknod(fs, context, path, type, callback) {
+function mknod(context, path, type, callback) {
   if(!pathCheck(path, callback)) return;
   make_node(context, path, type, callback);
 }
 
-function mkdir(fs, context, path, mode, callback) {
-  if (arguments.length < 5) {
+function mkdir(context, path, mode, callback) {
+  if (arguments.length < 4) {
     callback = mode;
     mode = FULL_READ_WRITE_EXEC_PERMISSIONS;
   } else {
@@ -1712,7 +1713,7 @@ function mkdir(fs, context, path, mode, callback) {
   make_directory(context, path, callback);
 }
 
-function access(fs, context, path, mode, callback) {
+function access(context, path, mode, callback) {
   if (typeof mode === 'function') {
     callback = mode;
     mode = Constants.fsConstants.F_OK;
@@ -1723,7 +1724,7 @@ function access(fs, context, path, mode, callback) {
   access_file(context, path, mode, callback);
 }
 
-function mkdtemp(fs, context, prefix, options, callback) { 
+function mkdtemp(context, prefix, options, callback) {
   callback = arguments[arguments.length - 1];
   if(!prefix) {
     return callback(new Error('filename prefix is required'));
@@ -1738,19 +1739,19 @@ function mkdtemp(fs, context, prefix, options, callback) {
   });  
 }
 
-function rmdir(fs, context, path, callback) {
+function rmdir(context, path, callback) {
   if(!pathCheck(path, callback)) return;
   remove_directory(context, path, callback);
 }
 
-function stat(fs, context, path, callback) {
+function stat(context, path, callback) {
   if(!pathCheck(path, callback)) return;
 
   function check_result(error, result) {
     if(error) {
       callback(error);
     } else {
-      var stats = new Stats(path, result, fs.name);
+      var stats = new Stats(path, result, context.name);
       callback(null, stats);
     }
   }
@@ -1758,17 +1759,17 @@ function stat(fs, context, path, callback) {
   stat_file(context, path, check_result);
 }
 
-function fstat(fs, context, fd, callback) {
+function fstat(context, fd, callback) {
   function check_result(error, result) {
     if(error) {
       callback(error);
     } else {
-      var stats = new Stats(ofd.path, result, fs.name);
+      var stats = new Stats(ofd.path, result, context.name);
       callback(null, stats);
     }
   }
 
-  var ofd = fs.openFiles[fd];
+  var ofd = openFiles.getOpenFileDescription(fd);
   if(!ofd) {
     callback(new Errors.EBADF());
   } else {
@@ -1776,18 +1777,18 @@ function fstat(fs, context, fd, callback) {
   }
 }
 
-function link(fs, context, oldpath, newpath, callback) {
+function link(context, oldpath, newpath, callback) {
   if(!pathCheck(oldpath, callback)) return;
   if(!pathCheck(newpath, callback)) return;
   link_node(context, oldpath, newpath, callback);
 }
 
-function unlink(fs, context, path, callback) {
+function unlink(context, path, callback) {
   if(!pathCheck(path, callback)) return;
   unlink_node(context, path, callback);
 }
 
-function read(fs, context, fd, buffer, offset, length, position, callback) {
+function read(context, fd, buffer, offset, length, position, callback) {
   // Follow how node.js does this
   function wrapped_cb(err, bytesRead) {
     // Retain a reference to buffer so that it can't be GC'ed too soon.
@@ -1798,7 +1799,7 @@ function read(fs, context, fd, buffer, offset, length, position, callback) {
   length = (undefined === length) ? buffer.length - offset : length;
   callback = arguments[arguments.length - 1];
 
-  var ofd = fs.openFiles[fd];
+  var ofd = openFiles.getOpenFileDescription(fd);
   if(!ofd) {
     callback(new Errors.EBADF());
   } else if(!ofd.flags.includes(O_READ)) {
@@ -1808,9 +1809,9 @@ function read(fs, context, fd, buffer, offset, length, position, callback) {
   }
 }
 
-function fsync(fs, context, fd, callback) {
+function fsync(context, fd, callback) {
   if(validateInteger(fd, callback) !== fd) return;
-  var ofd = fs.openFiles[fd];
+  var ofd = openFiles.getOpenFileDescription(fd);
   if(!ofd) {
     callback(new Errors.EBADF());
   } else {
@@ -1818,7 +1819,7 @@ function fsync(fs, context, fd, callback) {
   }
 }
 
-function readFile(fs, context, path, options, callback) {
+function readFile(context, path, options, callback) {
   callback = arguments[arguments.length - 1];
   options = validate_file_options(options, null, 'r');
 
@@ -1834,10 +1835,10 @@ function readFile(fs, context, path, options, callback) {
       return callback(err);
     }
     var ofd = new OpenFileDescription(path, fileNode.id, flags, 0);
-    var fd = fs.allocDescriptor(ofd);
+    var fd = openFiles.allocDescriptor(ofd);
 
     function cleanup() {
-      fs.releaseDescriptor(fd);
+      openFiles.releaseDescriptor(fd);
     }
 
     fstat_file(context, ofd, function(err, fstatResult) {
@@ -1846,7 +1847,7 @@ function readFile(fs, context, path, options, callback) {
         return callback(err);
       }
 
-      var stats = new Stats(ofd.path, fstatResult, fs.name);
+      var stats = new Stats(ofd.path, fstatResult, context.name);
 
       if(stats.isDirectory()) {
         cleanup();
@@ -1875,12 +1876,12 @@ function readFile(fs, context, path, options, callback) {
   });
 }
 
-function write(fs, context, fd, buffer, offset, length, position, callback) {
+function write(context, fd, buffer, offset, length, position, callback) {
   callback = arguments[arguments.length - 1];
   offset = (undefined === offset) ? 0 : offset;
   length = (undefined === length) ? buffer.length - offset : length;
 
-  var ofd = fs.openFiles[fd];
+  var ofd = openFiles.getOpenFileDescription(fd);
   if(!ofd) {
     callback(new Errors.EBADF());
   } else if(!ofd.flags.includes(O_WRITE)) {
@@ -1892,7 +1893,7 @@ function write(fs, context, fd, buffer, offset, length, position, callback) {
   }
 }
 
-function writeFile(fs, context, path, data, options, callback) {
+function writeFile(context, path, data, options, callback) {
   callback = arguments[arguments.length - 1];
   options = validate_file_options(options, 'utf8', 'w');
 
@@ -1916,10 +1917,10 @@ function writeFile(fs, context, path, data, options, callback) {
       return callback(err);
     }
     var ofd = new OpenFileDescription(path, fileNode.id, flags, 0);
-    var fd = fs.allocDescriptor(ofd);
+    var fd = openFiles.allocDescriptor(ofd);
 
     replace_data(context, ofd, data, 0, data.length, function(err) {
-      fs.releaseDescriptor(fd);
+      openFiles.releaseDescriptor(fd);
 
       if(err) {
         return callback(err);
@@ -1929,7 +1930,7 @@ function writeFile(fs, context, path, data, options, callback) {
   });
 }
 
-function appendFile(fs, context, path, data, options, callback) {
+function appendFile(context, path, data, options, callback) {
   callback = arguments[arguments.length - 1];
   options = validate_file_options(options, 'utf8', 'a');
 
@@ -1953,10 +1954,10 @@ function appendFile(fs, context, path, data, options, callback) {
       return callback(err);
     }
     var ofd = new OpenFileDescription(path, fileNode.id, flags, fileNode.size);
-    var fd = fs.allocDescriptor(ofd);
+    var fd = openFiles.allocDescriptor(ofd);
 
     write_data(context, ofd, data, 0, data.length, ofd.position, function(err) {
-      fs.releaseDescriptor(fd);
+      openFiles.releaseDescriptor(fd);
 
       if(err) {
         return callback(err);
@@ -1966,11 +1967,11 @@ function appendFile(fs, context, path, data, options, callback) {
   });
 }
 
-function exists(fs, context, path, callback) {
+function exists(context, path, callback) {
   function cb(err) {
     callback(err ? false : true);
   }
-  stat(fs, context, path, cb);
+  stat(context, path, cb);
 }
 
 function validateInteger(value, callback) {
@@ -2096,13 +2097,13 @@ function fchown_file(context, ofd, uid, gid, callback) {
   ofd.getNode(context, update_owner);
 }
 
-function getxattr(fs, context, path, name, callback) {
+function getxattr(context, path, name, callback) {
   if (!pathCheck(path, callback)) return;
   getxattr_file(context, path, name, callback);
 }
 
-function fgetxattr(fs, context, fd, name, callback) {
-  var ofd = fs.openFiles[fd];
+function fgetxattr(context, fd, name, callback) {
+  var ofd = openFiles.getOpenFileDescription(fd);
   if (!ofd) {
     callback(new Errors.EBADF());
   }
@@ -2111,7 +2112,7 @@ function fgetxattr(fs, context, fd, name, callback) {
   }
 }
 
-function setxattr(fs, context, path, name, value, flag, callback) {
+function setxattr(context, path, name, value, flag, callback) {
   if(typeof flag === 'function') {
     callback = flag;
     flag = null;
@@ -2121,13 +2122,13 @@ function setxattr(fs, context, path, name, value, flag, callback) {
   setxattr_file(context, path, name, value, flag, callback);
 }
 
-function fsetxattr(fs, context, fd, name, value, flag, callback) {
+function fsetxattr(context, fd, name, value, flag, callback) {
   if(typeof flag === 'function') {
     callback = flag;
     flag = null;
   }
 
-  var ofd = fs.openFiles[fd];
+  var ofd = openFiles.getOpenFileDescription(fd);
   if (!ofd) {
     callback(new Errors.EBADF());
   }
@@ -2139,13 +2140,13 @@ function fsetxattr(fs, context, fd, name, value, flag, callback) {
   }
 }
 
-function removexattr(fs, context, path, name, callback) {
+function removexattr(context, path, name, callback) {
   if (!pathCheck(path, callback)) return;
   removexattr_file(context, path, name, callback);
 }
 
-function fremovexattr(fs, context, fd, name, callback) {
-  var ofd = fs.openFiles[fd];
+function fremovexattr(context, fd, name, callback) {
+  var ofd = openFiles.getOpenFileDescription(fd);
   if (!ofd) {
     callback(new Errors.EBADF());
   }
@@ -2157,7 +2158,7 @@ function fremovexattr(fs, context, fd, name, callback) {
   }
 }
 
-function lseek(fs, context, fd, offset, whence, callback) {
+function lseek(context, fd, offset, whence, callback) {
   function update_descriptor_position(error, stats) {
     if(error) {
       callback(error);
@@ -2171,7 +2172,7 @@ function lseek(fs, context, fd, offset, whence, callback) {
     }
   }
 
-  var ofd = fs.openFiles[fd];
+  var ofd = openFiles.getOpenFileDescription(fd);
   if(!ofd) {
     callback(new Errors.EBADF());
   }
@@ -2197,7 +2198,7 @@ function lseek(fs, context, fd, offset, whence, callback) {
   }
 }
 
-function readdir(fs, context, path, callback) {
+function readdir(context, path, callback) {
   if(!pathCheck(path, callback)) return;
   read_directory(context, path, callback);
 }
@@ -2211,7 +2212,7 @@ function toUnixTimestamp(time) {
   }
 }
 
-function utimes(fs, context, path, atime, mtime, callback) {
+function utimes(context, path, atime, mtime, callback) {
   if(!pathCheck(path, callback)) return;
 
   var currentTime = Date.now();
@@ -2221,12 +2222,12 @@ function utimes(fs, context, path, atime, mtime, callback) {
   utimes_file(context, path, atime, mtime, callback);
 }
 
-function futimes(fs, context, fd, atime, mtime, callback) {
+function futimes(context, fd, atime, mtime, callback) {
   var currentTime = Date.now();
   atime = (atime) ? toUnixTimestamp(atime) : toUnixTimestamp(currentTime);
   mtime = (mtime) ? toUnixTimestamp(mtime) : toUnixTimestamp(currentTime);
 
-  var ofd = fs.openFiles[fd];
+  var ofd = openFiles.getOpenFileDescription(fd);
   if(!ofd) {
     callback(new Errors.EBADF());
   } else if(!ofd.flags.includes(O_WRITE)) {
@@ -2236,7 +2237,7 @@ function futimes(fs, context, fd, atime, mtime, callback) {
   }
 }
 
-function chmod(fs, context, path, mode, callback) {
+function chmod(context, path, mode, callback) {
   if(!pathCheck(path, callback)) return;
   mode = validateAndMaskMode(mode, callback);
   if(!mode) return;
@@ -2244,11 +2245,11 @@ function chmod(fs, context, path, mode, callback) {
   chmod_file(context, path, mode, callback);
 }
 
-function fchmod(fs, context, fd, mode, callback) {
+function fchmod(context, fd, mode, callback) {
   mode = validateAndMaskMode(mode, callback);
   if(!mode) return;
 
-  var ofd = fs.openFiles[fd];
+  var ofd = openFiles.getOpenFileDescription(fd);
   if(!ofd) {
     callback(new Errors.EBADF());
   } else if(!ofd.flags.includes(O_WRITE)) {
@@ -2258,7 +2259,7 @@ function fchmod(fs, context, fd, mode, callback) {
   }
 }
 
-function chown(fs, context, path, uid, gid, callback) {
+function chown(context, path, uid, gid, callback) {
   if(!pathCheck(path, callback)) return;
   if(!isUint32(uid)) {
     return callback(new Errors.EINVAL('uid must be a valid integer', uid));
@@ -2270,7 +2271,7 @@ function chown(fs, context, path, uid, gid, callback) {
   chown_file(context, path, uid, gid, callback);
 }
 
-function fchown(fs, context, fd, uid, gid, callback) {
+function fchown(context, fd, uid, gid, callback) {
   if(!isUint32(uid)) {
     return callback(new Errors.EINVAL('uid must be a valid integer', uid));
   }
@@ -2278,7 +2279,7 @@ function fchown(fs, context, fd, uid, gid, callback) {
     return callback(new Errors.EINVAL('gid must be a valid integer', gid));
   }
 
-  var ofd = fs.openFiles[fd];
+  var ofd = openFiles.getOpenFileDescription(fd);
   if(!ofd) {
     callback(new Errors.EBADF());
   } else if(!ofd.flags.includes(O_WRITE)) {
@@ -2288,7 +2289,7 @@ function fchown(fs, context, fd, uid, gid, callback) {
   }
 }
 
-function rename(fs, context, oldpath, newpath, callback) {
+function rename(context, oldpath, newpath, callback) {
   if(!pathCheck(oldpath, callback)) return;
   if(!pathCheck(newpath, callback)) return;
 
@@ -2403,7 +2404,7 @@ function rename(fs, context, oldpath, newpath, callback) {
   find_node(context, oldpath, check_node_type);
 }
 
-function symlink(fs, context, srcpath, dstpath, type, callback) {
+function symlink(context, srcpath, dstpath, type, callback) {
   // NOTE: we support passing the `type` arg, but ignore it.
   callback = arguments[arguments.length - 1];
 
@@ -2415,19 +2416,19 @@ function symlink(fs, context, srcpath, dstpath, type, callback) {
   make_symbolic_link(context, srcpath, dstpath, callback);
 }
 
-function readlink(fs, context, path, callback) {
+function readlink(context, path, callback) {
   if(!pathCheck(path, callback)) return;
   read_link(context, path, callback);
 }
 
-function lstat(fs, context, path, callback) {
+function lstat(context, path, callback) {
   if(!pathCheck(path, callback)) return;
 
   function check_result(error, result) {
     if(error) {
       callback(error);
     } else {
-      var stats = new Stats(path, result, fs.name);
+      var stats = new Stats(path, result, context.name);
       callback(null, stats);
     }
   }
@@ -2435,7 +2436,7 @@ function lstat(fs, context, path, callback) {
   lstat_file(context, path, check_result);
 }
 
-function truncate(fs, context, path, length, callback) {
+function truncate(context, path, length, callback) {
   // NOTE: length is optional
   callback = arguments[arguments.length - 1];
   length = length || 0;
@@ -2446,12 +2447,12 @@ function truncate(fs, context, path, length, callback) {
   truncate_file(context, path, length, callback);
 }
 
-function ftruncate(fs, context, fd, length, callback) {
+function ftruncate(context, fd, length, callback) {
   // NOTE: length is optional
   callback = arguments[arguments.length - 1];
   length = length || 0;
 
-  var ofd = fs.openFiles[fd];
+  var ofd = openFiles.getOpenFileDescription(fd);
   if(!ofd) {
     callback(new Errors.EBADF());
   } else if(!ofd.flags.includes(O_WRITE)) {
diff --git a/src/filesystem/interface.js b/src/filesystem/interface.js
index 901212f..9b9130c 100644
--- a/src/filesystem/interface.js
+++ b/src/filesystem/interface.js
@@ -22,7 +22,6 @@ var defaultGuidFn = require('../shared.js').guid;
 var STDIN = Constants.STDIN;
 var STDOUT = Constants.STDOUT;
 var STDERR = Constants.STDERR;
-var FIRST_DESCRIPTOR = Constants.FIRST_DESCRIPTOR;
 
 // The core fs operations live on impl
 var impl = require('./implementation.js');
@@ -101,22 +100,6 @@ function FileSystem(options, callback) {
   // Expose Shell constructor
   this.Shell = Shell.bind(undefined, this);
 
-  // Safely expose the list of open files and file
-  // descriptor management functions
-  var openFiles = {};
-  var nextDescriptor = FIRST_DESCRIPTOR;
-  Object.defineProperty(this, 'openFiles', {
-    get: function() { return openFiles; }
-  });
-  this.allocDescriptor = function(openFileDescription) {
-    var fd = nextDescriptor ++;
-    openFiles[fd] = openFileDescription;
-    return fd;
-  };
-  this.releaseDescriptor = function(fd) {
-    delete openFiles[fd];
-  };
-
   // Safely expose the operation queue
   var queue = [];
   this.queueOrRun = function(operation) {
@@ -205,6 +188,7 @@ function FileSystem(options, callback) {
     function complete(error) {
       function wrappedContext(methodName) {
         var context = provider[methodName]();
+        context.name = name;
         context.flags = flags;
         context.changes = [];
         context.guid = wrappedGuidFn(context);
@@ -348,7 +332,7 @@ function FileSystem(options, callback) {
         // Forward this call to the impl's version, using the following
         // call signature, with complete() as the callback/last-arg now:
         // fn(fs, context, arg0, arg1, ... , complete);
-        var fnArgs = [fs, context].concat(args);
+        var fnArgs = [context].concat(args);
         impl[methodName].apply(null, fnArgs);
       });
       if(error) {
diff --git a/src/open-files.js b/src/open-files.js
new file mode 100644
index 0000000..fbbe1d6
--- /dev/null
+++ b/src/open-files.js
@@ -0,0 +1,44 @@
+const { FIRST_DESCRIPTOR } = require('./constants');
+const openFiles = {};
+
+/**
+ * Start at FIRST_DESCRIPTOR and go until we find
+ * an empty file descriptor, then return it.
+ */
+const getEmptyDescriptor = () => {
+  let fd = FIRST_DESCRIPTOR;
+
+  while(getOpenFileDescription(fd)) {
+    fd++;
+  }
+
+  return fd;
+};
+
+/**
+ * Look up the open file description object for a given
+ * file descriptor.
+ */
+const getOpenFileDescription = ofd => openFiles[ofd];
+
+/**
+ * Allocate a new file descriptor for the given
+ * open file description. 
+ */
+const allocDescriptor = openFileDescription => {
+  const ofd = getEmptyDescriptor();
+  openFiles[ofd] = openFileDescription;
+  return ofd;
+};
+
+/**
+ * Release the given existing file descriptor created
+ * with allocDescriptor(). 
+ */
+const releaseDescriptor = ofd => delete openFiles[ofd];
+
+module.exports = {
+  allocDescriptor,
+  releaseDescriptor,
+  getOpenFileDescription
+};


Test patch:
diff --git a/tests/spec/fs.chown.spec.js b/tests/spec/fs.chown.spec.js
index dd1f6df..e141321 100644
--- a/tests/spec/fs.chown.spec.js
+++ b/tests/spec/fs.chown.spec.js
@@ -36,7 +36,7 @@ describe('fs.chown, fs.fchown', function() {
       fs.fchown(fd, '1001', 1001, function(err) {
         expect(err).to.exist;
         expect(err.code).to.equal('EINVAL');
-        done();
+        fs.close(fd, done);
       });
     });
   });
@@ -64,7 +64,7 @@ describe('fs.chown, fs.fchown', function() {
       fs.fchown(fd, 1001, '1001', function(err) {
         expect(err).to.exist;
         expect(err.code).to.equal('EINVAL');
-        done();
+        fs.close(fd, done);
       });
     });
   });
diff --git a/tests/spec/fs.close.spec.js b/tests/spec/fs.close.spec.js
index 0a94992..33e1808 100644
--- a/tests/spec/fs.close.spec.js
+++ b/tests/spec/fs.close.spec.js
@@ -23,6 +23,7 @@ describe('fs.close', function() {
 
         fs.read(fd, buffer, 0, buffer.length, undefined, function(error, result) {
           expect(error).to.exist;
+          expect(error.code).to.equal('EBADF');
           expect(result).not.to.exist;
           done();
         });
diff --git a/tests/spec/fs.lseek.spec.js b/tests/spec/fs.lseek.spec.js
index 4d8f9ce..97030cc 100644
--- a/tests/spec/fs.lseek.spec.js
+++ b/tests/spec/fs.lseek.spec.js
@@ -77,7 +77,7 @@ describe('fs.lseek', function() {
                 expect(result.size).to.equal(offset + buffer.length);
                 var expected = Buffer.from([1, 2, 3, 1, 2, 3, 4, 5, 6, 7, 8]);
                 expect(result_buffer).to.deep.equal(expected);
-                done();
+                fs.close(fd, done);
               });
             });
           });
@@ -117,7 +117,7 @@ describe('fs.lseek', function() {
                 expect(result.size).to.equal(offset + 2 * buffer.length);
                 var expected = Buffer.from([1, 2, 3, 4, 5, 6, 1, 2, 3, 4, 5, 6, 7, 8]);
                 expect(result_buffer).to.deep.equal(expected);
-                done();
+                fs.close(fd, done);
               });
             });
           });
@@ -163,7 +163,12 @@ describe('fs.lseek', function() {
 
                   var expected = Buffer.from([1, 2, 3, 4, 5, 6, 7, 8, 0, 0, 0, 0, 0, 1, 2, 3, 4, 5, 6, 7, 8]);
                   expect(result_buffer).to.deep.equal(expected);
-                  done();
+
+                  fs.close(fd1, function(error) {
+                    if(error) throw error;
+
+                    fs.close(fd2, done);
+                  });
                 });
               });
             });
diff --git a/tests/spec/fs.open.spec.js b/tests/spec/fs.open.spec.js
index af9aed8..edb7bc8 100644
--- a/tests/spec/fs.open.spec.js
+++ b/tests/spec/fs.open.spec.js
@@ -1,6 +1,6 @@
 var util = require('../lib/test-utils.js');
 var expect = require('chai').expect;
-var constants = require('../../src/constants.js');
+var { FIRST_DESCRIPTOR } = require('../../src/constants.js');
 
 describe('fs.open', function() {
   beforeEach(util.setup);
@@ -63,45 +63,68 @@ describe('fs.open', function() {
 
   it('should return a unique file descriptor', function(done) {
     var fs = util.fs();
-    var fd1;
 
     fs.open('/file1', 'w+', function(error, fd) {
       if(error) throw error;
       expect(error).not.to.exist;
       expect(fd).to.be.a('number');
 
-      fs.open('/file2', 'w+', function(error, fd) {
+      fs.open('/file2', 'w+', function(error, fd1) {
         if(error) throw error;
         expect(error).not.to.exist;
-        expect(fd).to.be.a('number');
-        expect(fd).not.to.equal(fd1);
-        done();
+        expect(fd1).to.be.a('number');
+        expect(fd1).not.to.equal(fd);
+
+        fs.close(fd, function(error) {
+          if(error) throw error;
+
+          fs.close(fd1, done);
+        });
       });
     });
   });
 
-  it('should return the argument value of the file descriptor index matching the value set by the first useable file descriptor constant', function(done) {
+  it('should return the argument value of the file descriptor index greater than or equal to the value set by the first useable file descriptor constant', function(done) {
     var fs = util.fs();
-    var firstFD = constants.FIRST_DESCRIPTOR;
 
     fs.open('/file1', 'w+', function(error, fd) {
       if(error) throw error;
-      expect(fd).to.equal(firstFD);
-      done();
+      expect(fd).to.equal(FIRST_DESCRIPTOR);
+      fs.close(fd, done);
+    });
+  });
+
+  it('should reuse file descriptors after closing', function(done) {
+    var fs = util.fs();
+
+    fs.open('/file1', 'w+', function(error, fd) {
+      if(error) throw error;
+      expect(fd).to.equal(FIRST_DESCRIPTOR);
+
+      fs.close(fd, function(error) {
+        if(error) throw error;
+
+        fs.open('/file1', 'w+', function(error, fd) {
+          if(error) throw error;
+          expect(fd).to.equal(FIRST_DESCRIPTOR);
+  
+          fs.close(fd, done);
+        });
+      });
     });
   });
 
   it('should create a new file when flagged for write', function(done) {
     var fs = util.fs();
 
-    fs.open('/myfile', 'w', function(error) {
+    fs.open('/myfile', 'w', function(error, fd) {
       if(error) throw error;
       
       fs.stat('/myfile', function(error, result) {
         expect(error).not.to.exist;
         expect(result).to.exist;
         expect(result.isFile()).to.be.true;
-        done();
+        fs.close(fd, done);
       });
     });
   });
@@ -110,7 +133,7 @@ describe('fs.open', function() {
   it('should create a new file, when flagged for write, and set the mode to the passed value', function(done) {
 
     var fs = util.fs();
-    fs.open('/myfile', 'w', 0o777, function(error) {
+    fs.open('/myfile', 'w', 0o777, function(error, fd) {
       if(error) throw error;
 
       fs.stat('/myfile', function(error, result) {
@@ -118,7 +141,7 @@ describe('fs.open', function() {
         expect(result).to.exist;
         expect(result.mode).to.exist;
         expect(result.mode & 0o777).to.equal(0o777);
-        done();
+        fs.close(fd, done);
       });
     });
   });
@@ -126,7 +149,7 @@ describe('fs.open', function() {
   it('should create a new file, but no mode is passed, so  the default value of 644 should be seen', function(done) {
 
     var fs = util.fs();
-    fs.open('/myfile', 'w', function(error) {
+    fs.open('/myfile', 'w', function(error, fd) {
       if(error) throw error;
 
       fs.stat('/myfile', function(error, result) {
@@ -134,7 +157,7 @@ describe('fs.open', function() {
         expect(result).to.exist;
         expect(result.mode).to.exist;
         expect(result.mode & 0o644).to.equal(0o644);
-        done();
+        fs.close(fd, done);
       });
     });
   });
@@ -162,8 +185,7 @@ describe('fs.open', function() {
             expect(error.code).to.equal('EBADF');
             expect(result).not.to.exist;
 
-            fs.close(fd);
-            done();
+            fs.close(fd, done);
           });
         });
       });
@@ -180,22 +202,3 @@ describe('fs.open', function() {
     });
   });
 });
-
-/**
- * fsPromise.open() Tests
- **/
-describe('fsPromises.open', function() {
-  beforeEach(util.setup);
-  afterEach(util.cleanup);
-
-  it('should return an error if the parent path does not exist', function() {
-    var fsPromises = util.fs().promises;
-
-    return fsPromises.open('/tmp/myfile', 'w+')
-      .then(result => expect(result).not.to.exist)
-      .catch(error => {
-        expect(error).to.exist;
-        expect(error.code).to.equal('ENOENT');
-      });
-  });
-});
diff --git a/tests/spec/fs.read.spec.js b/tests/spec/fs.read.spec.js
index 216c3bc..29b08f4 100644
--- a/tests/spec/fs.read.spec.js
+++ b/tests/spec/fs.read.spec.js
@@ -17,6 +17,7 @@ describe('fs.read', function() {
 
     fs.open('/myfile', 'w+', function(error, fd) {
       if(error) throw error;
+
       fs.write(fd, wbuffer, 0, wbuffer.length, 0, function(error, result) {
         if(error) throw error;
         expect(result).to.equal(wbuffer.length);
@@ -25,7 +26,7 @@ describe('fs.read', function() {
           expect(error).not.to.exist;
           expect(result).to.equal(rbuffer.length);
           expect(wbuffer).to.deep.equal(rbuffer);
-          done();
+          fs.close(fd, done);
         });
       });
     });
@@ -54,7 +55,7 @@ describe('fs.read', function() {
             expect(error).not.to.exist;
             expect(_result).to.equal(rbuffer.length);
             expect(wbuffer).to.deep.equal(rbuffer);
-            done();
+            fs.close(fd, done);
           });
         });
       });
@@ -77,7 +78,7 @@ describe('fs.read', function() {
           expect(error.code).to.equal('EISDIR');
           expect(result).to.equal(0);
           expect(buf).to.deep.equal(buf2);
-          done();
+          fs.close(fd, done);
         });
       });
     });
diff --git a/tests/spec/fs.stat.spec.js b/tests/spec/fs.stat.spec.js
index e417ad2..e2ad90b 100644
--- a/tests/spec/fs.stat.spec.js
+++ b/tests/spec/fs.stat.spec.js
@@ -113,7 +113,7 @@ describe('fs.stat', function() {
         expect(result['ctimeMs']).to.be.a('number');
         expect(result['type']).to.equal('FILE');
 
-        done();
+        fs.close(fd, done);
       });
     });
   });
diff --git a/tests/spec/fs.stats.spec.js b/tests/spec/fs.stats.spec.js
index 1752923..79e7fb7 100644
--- a/tests/spec/fs.stats.spec.js
+++ b/tests/spec/fs.stats.spec.js
@@ -21,9 +21,11 @@ describe('fs.stats', function() {
 
       fs.open('/myfile', 'w+', function(error, fd) {
         if(error) throw error;
+
         fs.fstat(fd, function(error, stats) {
+          expect(error).not.to.exist;
           expect(stats.isFile()).to.be.true;
-          done();
+          fs.close(fd, done);
         });
       });
     });
@@ -50,6 +52,7 @@ describe('fs.stats', function() {
           fs.symlink('/myfile', '/myfilelink', function(error) {
             if(error) throw error;
             fs.lstat('/myfilelink', function(error, stats) {
+              expect(error).not.to.exist;
               expect(stats.isFile()).to.be.false;
               done();
             });
@@ -78,8 +81,9 @@ describe('fs.stats', function() {
       fs.open('/myfile', 'w+', function(error, fd) {
         if(error) throw error;
         fs.fstat(fd, function(error, stats) {
+          expect(error).not.to.exist;
           expect(stats.isDirectory()).to.be.false;
-          done();
+          fs.close(fd, done);
         });
       });
     });
@@ -178,8 +182,9 @@ describe('fs.stats', function() {
       fs.open('/myfile', 'w+', function(error, fd) {
         if(error) throw error;
         fs.fstat(fd, function(error, stats) {
+          expect(error).not.to.exist;
           expect(stats.isSymbolicLink()).to.be.false;
-          done();
+          fs.close(fd, done);
         });
       });
     });
@@ -290,7 +295,7 @@ describe('fs.stats', function() {
           if(err) throw err;
 
           expect(stats.name).to.equal(Path.basename(filepath));
-          done();
+          fs.close(fd, done);
         });
       });
     });
diff --git a/tests/spec/fs.write.spec.js b/tests/spec/fs.write.spec.js
index 7d30f90..c93936b 100644
--- a/tests/spec/fs.write.spec.js
+++ b/tests/spec/fs.write.spec.js
@@ -35,7 +35,7 @@ describe('fs.write', function() {
           expect(error).not.to.exist;
           expect(result.isFile()).to.be.true;
           expect(result.size).to.equal(buffer.length);
-          done();
+          fs.close(fd, done);
         });
       });
     });
@@ -62,7 +62,7 @@ describe('fs.write', function() {
             expect(error).not.to.exist;
             expect(_result).to.equal(2 * buffer.length);
             expect(result.size).to.equal(_result);
-            done();
+            fs.close(fd, done);
           });
         });
       });
diff --git a/tests/spec/fs.xattr.spec.js b/tests/spec/fs.xattr.spec.js
index 7fb8161..a624f8e 100644
--- a/tests/spec/fs.xattr.spec.js
+++ b/tests/spec/fs.xattr.spec.js
@@ -236,7 +236,7 @@ describe('fs.xattr', function() {
         fs.fgetxattr(ofd, 'test', function (error, value) {
           expect(error).not.to.exist;
           expect(value).to.equal('value');
-          done();
+          fs.close(ofd, done);
         });
       });
     });
@@ -368,7 +368,7 @@ describe('fs.xattr', function() {
             fs.fgetxattr(ofd, 'test', function (error) {
               expect(error).to.exist;
               expect(error.code).to.equal('ENOATTR');
-              done();
+              fs.close(ofd, done);
             });
           });
         });
diff --git a/tests/spec/node-js/simple/test-fs-watch.js b/tests/spec/node-js/simple/test-fs-watch.js
index c7d7fef..ba43ae3 100644
--- a/tests/spec/node-js/simple/test-fs-watch.js
+++ b/tests/spec/node-js/simple/test-fs-watch.js
@@ -52,20 +52,30 @@ describe('node.js tests: https://github.com/joyent/node/blob/master/test/simple/
 
   it('should allow watches on dirs', function(done) {
     var fs = util.fs();
+
     fs.mkdir('/tmp', function(error) {
       if(error) throw error;
+      var steps = 0;
+
+      function cleanup() {
+        steps++;
+        
+        if(steps === 2) {
+          done();
+        }
+      }
 
       var watcher = fs.watch('/tmp', function(event, filename) {
         // TODO: node thinks this should be 'rename', need to add rename along with change.
         expect(event).to.equal('change');
         expect(filename).to.equal('/tmp');
         watcher.close();
-        done();
+        cleanup();
       });
 
       fs.open('/tmp/newfile.txt', 'w', function(error, fd) {
         if(error) throw error;
-        fs.close(fd);
+        fs.close(fd, cleanup);
       });
     });
   });
diff --git a/tests/spec/times.spec.js b/tests/spec/times.spec.js
index c6ca38e..4ee431a 100644
--- a/tests/spec/times.spec.js
+++ b/tests/spec/times.spec.js
@@ -517,7 +517,7 @@ describe('node times (atime, mtime, ctimeMs)', function() {
               expect(stats2.ctimeMs).to.be.at.least(stats1.ctimeMs);
               expect(stats2.mtimeMs).to.equal(stats1.mtimeMs);
               expect(stats2.atimeMs).to.be.at.least(stats1.atimeMs);
-              done();
+              fs.close(fd, done);
             });
           });
         });
@@ -568,7 +568,7 @@ describe('node times (atime, mtime, ctimeMs)', function() {
                 expect(stats2.ctimeMs).to.equal(stats1.ctimeMs);
                 expect(stats2.mtimeMs).to.equal(stats1.mtimeMs);
                 expect(stats2.atimeMs).to.equal(stats1.atimeMs);
-                done();
+                fs.close(fd, done);
               });
             });
           });
@@ -618,7 +618,7 @@ describe('node times (atime, mtime, ctimeMs)', function() {
                 expect(stats2.ctimeMs).to.be.at.least(stats1.ctimeMs);
                 expect(stats2.mtimeMs).to.equal(stats1.mtimeMs);
                 expect(stats2.atimeMs).to.be.at.least(stats1.atimeMs);
-                done();
+                fs.close(fd, done);
               });
             });
           });