Skip to content

Commit

Permalink
fix #197: return sync callback when using the sync interface, otherwi…
Browse files Browse the repository at this point in the history 
…se return the async callback
  • Loading branch information
@silkentrance
silkentrance committed Jan 27, 2020
1 parent 6e79e62 commit 17b9080
Show file tree
Hide file tree
Showing 4 changed files with 124 additions and 83 deletions.
114 changes: 69 additions & 45 deletions lib/tmp.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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));
});
});
}
Expand Down Expand Up @@ -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)
};
}

Expand All @@ -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));
});
});
}
Expand All @@ -352,7 +356,7 @@ function dirSync(options) {

return {
name: name,
removeCallback: _prepareTmpDirRemoveCallback(name, opts)
removeCallback: _prepareTmpDirRemoveCallback(name, opts, true)
};
}

Expand All @@ -370,7 +374,7 @@ function _removeFileAsync(fdPath, next) {
return next(err);
}
next();
}
};

if (0 <= fdPath[0])
fs.close(fdPath[0], function (err) {
Expand Down Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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'));
}
};
}

Expand Down Expand Up @@ -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
*/

/**
Expand Down
9 changes: 4 additions & 5 deletions test/dir-sync-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

var
assert = require('assert'),
fs = require('fs'),
path = require('path'),
inbandStandardTests = require('./inband-standard'),
childProcess = require('./child-process').genericChildProcess,
Expand All @@ -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 () {
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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();
Expand Down
20 changes: 16 additions & 4 deletions test/file-sync-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

var
assert = require('assert'),
fs = require('fs'),
inbandStandardTests = require('./inband-standard'),
assertions = require('./assertions'),
childProcess = require('./child-process').genericChildProcess,
Expand All @@ -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 () {
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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();
});
});
});
});
});

0 comments on commit 17b9080

Please sign in to comment.