Move open files to its own module, properly close fds in tests

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) {
@@ -349,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) {

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
+};

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);
       });
     });
   });

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();
         });

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);
+                  });
                 });
               });
             });

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,31 +133,31 @@ 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) {
         expect(error).not.to.exist;
         expect(result).to.exist;
         expect(result.mode).to.exist;
         expect(result.mode & 0o777).to.equal(0o777);
-        done();
+        fs.close(fd, done);
       });
     });
   });
 
   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) {
         expect(error).not.to.exist;
         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');
-      });
-  });
-});

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);
         });
       });
     });

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);
       });
     });
   });

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);
         });
       });
     });