fix #197: return sync callback when using the sync interface, otherwise return the async callback

Author: silkentrance

lib/tmp.js

@@ -42,7 +42,10 @@ const
   SIGINT = 'SIGINT',
 
   // this will hold the objects need to be removed on exit
-  _removeObjects = [];
+  _removeObjects = [],
+
+  // API change in fs.rmdirSync leads to error when passing in a second parameter, e.g. the callback
+  FN_RMDIR_SYNC = fs.rmdirSync.bind(fs);
 
 var
   _gracefulCleanup = false;
@@ -251,7 +254,9 @@ function file(options, callback) {
       /* istanbul ignore else */
       if (err) return cb(err);
 
+      // FIXME overall handling of opts.discardDescriptor is off
       if (opts.discardDescriptor) {
+        // FIXME? must not unlink as the user expects the filename to be reserved
         return fs.close(fd, function _discardCallback(err) {
           /* istanbul ignore else */
           if (err) {
@@ -270,12 +275,11 @@ function file(options, callback) {
           }
           cb(null, name, undefined, _prepareTmpFileRemoveCallback(name, -1, opts));
         });
+      } else {
+        // FIXME detachDescriptor passes the descriptor whereas discardDescriptor closes it
+        const discardOrDetachDescriptor = opts.discardDescriptor || opts.detachDescriptor;
+        cb(null, name, fd, _prepareTmpFileRemoveCallback(name, discardOrDetachDescriptor ? -1 : fd, opts, false));
       }
-      /* istanbul ignore else */
-      if (opts.detachDescriptor) {
-        return cb(null, name, fd, _prepareTmpFileRemoveCallback(name, -1, opts));
-      }
-      cb(null, name, fd, _prepareTmpFileRemoveCallback(name, fd, opts));
     });
   });
 }
@@ -304,7 +308,7 @@ function fileSync(options) {
   return {
     name: name,
     fd: fd,
-    removeCallback: _prepareTmpFileRemoveCallback(name, discardOrDetachDescriptor ? -1 : fd, opts)
+    removeCallback: _prepareTmpFileRemoveCallback(name, discardOrDetachDescriptor ? -1 : fd, opts, true)
   };
 }
 
@@ -330,7 +334,7 @@ function dir(options, callback) {
       /* istanbul ignore else */
       if (err) return cb(err);
 
-      cb(null, name, _prepareTmpDirRemoveCallback(name, opts));
+      cb(null, name, _prepareTmpDirRemoveCallback(name, opts, false));
     });
   });
 }
@@ -352,7 +356,7 @@ function dirSync(options) {
 
   return {
     name: name,
-    removeCallback: _prepareTmpDirRemoveCallback(name, opts)
+    removeCallback: _prepareTmpDirRemoveCallback(name, opts, true)
   };
 }
 
@@ -370,7 +374,7 @@ function _removeFileAsync(fdPath, next) {
       return next(err);
     }
     next();
-  }
+  };
 
   if (0 <= fdPath[0])
     fs.close(fdPath[0], function (err) {
@@ -405,19 +409,23 @@ function _removeFileSync(fdPath) {
 /**
  * Prepares the callback for removal of the temporary file.
  *
+ * Returns either a sync callback or a async callback depending on whether
+ * fileSync or file was called, which is expressed by the sync parameter.
+ *
  * @param {string} name the path of the file
  * @param {number} fd file descriptor
  * @param {Object} opts
- * @returns {fileCallback}
+ * @param {boolean} sync
+ * @returns {fileCallback | fileCallbackSync}
  * @private
  */
-function _prepareTmpFileRemoveCallback(name, fd, opts) {
-  const removeCallbackSync = _prepareRemoveCallback(_removeFileSync, [fd, name]);
-  const removeCallback = _prepareRemoveCallback(_removeFileAsync, [fd, name], removeCallbackSync);
+function _prepareTmpFileRemoveCallback(name, fd, opts, sync) {
+  const removeCallbackSync = _prepareRemoveCallback(_removeFileSync, [fd, name], sync);
+  const removeCallback = _prepareRemoveCallback(_removeFileAsync, [fd, name], sync, removeCallbackSync);
 
   if (!opts.keep) _removeObjects.unshift(removeCallbackSync);
 
-  return removeCallback;
+  return sync ? removeCallbackSync : removeCallback;
 }
 
 /**
@@ -445,67 +453,62 @@ function _rimrafRemoveDirSyncWrapper(dirPath, next) {
   }
 }
 
-const FN_RMDIR_SYNC = fs.rmdirSync.bind(fs);
-
 /**
  * Prepares the callback for removal of the temporary directory.
  *
+ * Returns either a sync callback or a async callback depending on whether
+ * tmpFileSync or tmpFile was called, which is expressed by the sync parameter.
+ *
  * @param {string} name
  * @param {Object} opts
+ * @param {boolean} sync
  * @returns {Function} the callback
  * @private
  */
-function _prepareTmpDirRemoveCallback(name, opts) {
+function _prepareTmpDirRemoveCallback(name, opts, sync) {
   const removeFunction = opts.unsafeCleanup ? _rimrafRemoveDirWrapper : fs.rmdir.bind(fs);
   const removeFunctionSync = opts.unsafeCleanup ? _rimrafRemoveDirSyncWrapper : FN_RMDIR_SYNC;
-  const removeCallbackSync = _prepareRemoveCallback(removeFunctionSync, name);
-  const removeCallback = _prepareRemoveCallback(removeFunction, name, removeCallbackSync);
+  const removeCallbackSync = _prepareRemoveCallback(removeFunctionSync, name, sync);
+  const removeCallback = _prepareRemoveCallback(removeFunction, name, sync, removeCallbackSync);
   if (!opts.keep) _removeObjects.unshift(removeCallbackSync);
 
-  return removeCallback;
+  return sync ? removeCallbackSync : removeCallback;
 }
 
 /**
  * Creates a guarded function wrapping the removeFunction call.
  *
+ * The cleanup callback is save to be called multiple times.
+ * Subsequent invocations will be ignored.
+ *
  * @param {Function} removeFunction
- * @param {Object} arg
- * @returns {Function}
+ * @param {string} fileOrDirName
+ * @param {boolean} sync
+ * @param {cleanupCallbackSync?} cleanupCallbackSync
+ * @returns {cleanupCallback | cleanupCallbackSync}
  * @private
  */
-function _prepareRemoveCallback(removeFunction, arg, cleanupCallbackSync) {
+function _prepareRemoveCallback(removeFunction, fileOrDirName, sync, cleanupCallbackSync) {
   var called = false;
 
+  // if sync is true, the next parameter will be ignored
   return function _cleanupCallback(next) {
-    next = next || function () {};
+
+    /* istanbul ignore else */
     if (!called) {
+      // remove cleanupCallback from cache
       const toRemove = cleanupCallbackSync || _cleanupCallback;
       const index = _removeObjects.indexOf(toRemove);
       /* istanbul ignore else */
       if (index >= 0) _removeObjects.splice(index, 1);
 
       called = true;
-      // sync?
-      if (removeFunction.length === 1) {
-        try {
-          removeFunction(arg);
-          return next(null);
-        }
-        catch (err) {
-          // if no next is provided and since we are
-          // in silent cleanup mode on process exit,
-          // we will ignore the error
-          return next(err);
-        }
+      if (sync || removeFunction === FN_RMDIR_SYNC) {
+        return removeFunction(fileOrDirName);
       } else {
-        // must no call rmdirSync/rmSync this way
-        if (removeFunction == FN_RMDIR_SYNC) {
-          return removeFunction(arg);
-        } else {
-          return removeFunction(arg, next);
-        }
+        return removeFunction(fileOrDirName, next || function() {});
       }
-    } else return next(new Error('cleanup callback has already been called'));
+    }
   };
 }
 
@@ -726,18 +729,39 @@ _safely_install_sigint_listener();
  * @param {cleanupCallback} fn the cleanup callback function
  */
 
+/**
+ * @callback fileCallbackSync
+ * @param {?Error} err the error object if anything goes wrong
+ * @param {string} name the temporary file name
+ * @param {number} fd the file descriptor
+ * @param {cleanupCallbackSync} fn the cleanup callback function
+ */
+
 /**
  * @callback dirCallback
  * @param {?Error} err the error object if anything goes wrong
  * @param {string} name the temporary file name
  * @param {cleanupCallback} fn the cleanup callback function
  */
 
+/**
+ * @callback dirCallbackSync
+ * @param {?Error} err the error object if anything goes wrong
+ * @param {string} name the temporary file name
+ * @param {cleanupCallbackSync} fn the cleanup callback function
+ */
+
 /**
  * Removes the temporary created file or directory.
  *
  * @callback cleanupCallback
- * @param {simpleCallback} [next] function to call after entry was removed
+ * @param {simpleCallback} [next] function to call whenever the tmp object needs to be removed
+ */
+
+/**
+ * Removes the temporary created file or directory.
+ *
+ * @callback cleanupCallbackSync
  */
 
 /**

test/dir-sync-test.js

@@ -3,7 +3,6 @@
 
 var
   assert = require('assert'),
-  fs = require('fs'),
   path = require('path'),
   inbandStandardTests = require('./inband-standard'),
   childProcess = require('./child-process').genericChildProcess,
@@ -20,7 +19,7 @@ describe('tmp', function () {
     describe('when running inband standard tests', function () {
       inbandStandardTests(false, function before() {
         this.topic = tmp.dirSync(this.opts);
-      });
+      }, true);
 
       describe('with invalid tries', function () {
         it('should result in an error on negative tries', function () {
@@ -68,8 +67,8 @@ describe('tmp', function () {
           if (!stderr) return done(new Error('stderr expected'));
           try {
             assertions.assertExists(stdout);
-          } catch (err) {
             rimraf.sync(stdout);
+          } catch (err) {
             return done(err);
           }
           done();
@@ -82,8 +81,8 @@ describe('tmp', function () {
           if (stderr) return done(new Error(stderr));
           try {
             assertions.assertExists(stdout);
-          } catch (err) {
             rimraf.sync(stdout);
+          } catch (err) {
             return done(err);
           }
           done();
@@ -130,8 +129,8 @@ describe('tmp', function () {
             } else {
               assertions.assertExists(path.join(stdout, 'symlinkme-target'));
             }
-          } catch (err) {
             rimraf.sync(stdout);
+          } catch (err) {
             return done(err);
           }
           done();

test/file-sync-test.js

@@ -3,7 +3,6 @@
 
 var
   assert = require('assert'),
-  fs = require('fs'),
   inbandStandardTests = require('./inband-standard'),
   assertions = require('./assertions'),
   childProcess = require('./child-process').genericChildProcess,
@@ -20,7 +19,7 @@ describe('tmp', function () {
     describe('when running inband standard tests', function () {
       inbandStandardTests(true, function before() {
         this.topic = tmp.fileSync(this.opts);
-      });
+      }, true);
 
       describe('with invalid tries', function () {
         it('should result in an error on negative tries', function () {
@@ -71,8 +70,8 @@ describe('tmp', function () {
           if (!stderr) return done(new Error('stderr expected'));
           try {
             assertions.assertExists(stdout, true);
-          } catch (err) {
             rimraf.sync(stdout);
+          } catch (err) {
             return done(err);
           }
           done();
@@ -85,8 +84,8 @@ describe('tmp', function () {
           if (stderr) return done(new Error(stderr));
           try {
             assertions.assertExists(stdout, true);
-          } catch (err) {
             rimraf.sync(stdout);
+          } catch (err) {
             return done(err);
           }
           done();
@@ -122,6 +121,19 @@ describe('tmp', function () {
           done();
         });
       });
+      it('on issue #115', function (done) {
+        childProcess(this, 'issue115-sync.json', function (err, stderr, stdout) {
+          if (err) return done(err);
+          if (stderr) return done(new Error(stderr));
+          try {
+            assertions.assertDoesNotExist(stdout);
+          } catch (err) {
+            rimraf.sync(stdout);
+            return done(err);
+          }
+          done();
+        });
+      });
     });
   });
 });