From 959a39fbd11ab4ad20c62c83863898c4e30582b0 Mon Sep 17 00:00:00 2001 From: Carsten Klein Date: Fri, 24 Jan 2020 23:26:05 +0100 Subject: [PATCH 1/4] fix #197: cleanupCallback must be sync or async based on initial API call --- CHANGELOG.md | 0 lib/tmp.js | 111 ++++++++++++++++++++++++++++++--------------------- 2 files changed, 66 insertions(+), 45 deletions(-) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..e69de29 diff --git a/lib/tmp.js b/lib/tmp.js index 4270d03..ad4316b 100644 --- a/lib/tmp.js +++ b/lib/tmp.js @@ -251,7 +251,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 return fs.close(fd, function _discardCallback(err) { /* istanbul ignore else */ if (err) { @@ -270,12 +272,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 +305,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 +331,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 +353,7 @@ function dirSync(options) { return { name: name, - removeCallback: _prepareTmpDirRemoveCallback(name, opts) + removeCallback: _prepareTmpDirRemoveCallback(name, opts, true) }; } @@ -370,7 +371,7 @@ function _removeFileAsync(fdPath, next) { return next(err); } next(); - } + }; if (0 <= fdPath[0]) fs.close(fdPath[0], function (err) { @@ -405,19 +406,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 +450,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 removeFunctionSync = opts.unsafeCleanup ? _rimrafRemoveDirSyncWrapper : fs.rmdirSync.bind(fs); + 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) { + 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,6 +726,14 @@ _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 @@ -733,11 +741,24 @@ _safely_install_sigint_listener(); * @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 */ /** From 3e67fe484c27869d503a6e581952d77512354359 Mon Sep 17 00:00:00 2001 From: Carsten Klein Date: Mon, 27 Jan 2020 16:46:32 +0100 Subject: [PATCH 2/4] fix #210: regression after bug fix for #197 --- lib/tmp.js | 11 ++++--- test/dir-sync-test.js | 9 +++--- test/file-sync-test.js | 20 ++++++++++--- test/inband-standard.js | 64 ++++++++++++++++++++++------------------- 4 files changed, 62 insertions(+), 42 deletions(-) diff --git a/lib/tmp.js b/lib/tmp.js index ad4316b..ca03f47 100644 --- a/lib/tmp.js +++ b/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; @@ -253,7 +256,7 @@ function file(options, callback) { // FIXME overall handling of opts.discardDescriptor is off if (opts.discardDescriptor) { - // FIXME must not unlink + // 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) { @@ -464,7 +467,7 @@ function _rimrafRemoveDirSyncWrapper(dirPath, next) { */ function _prepareTmpDirRemoveCallback(name, opts, sync) { const removeFunction = opts.unsafeCleanup ? _rimrafRemoveDirWrapper : fs.rmdir.bind(fs); - const removeFunctionSync = opts.unsafeCleanup ? _rimrafRemoveDirSyncWrapper : fs.rmdirSync.bind(fs); + const removeFunctionSync = opts.unsafeCleanup ? _rimrafRemoveDirSyncWrapper : FN_RMDIR_SYNC; const removeCallbackSync = _prepareRemoveCallback(removeFunctionSync, name, sync); const removeCallback = _prepareRemoveCallback(removeFunction, name, sync, removeCallbackSync); if (!opts.keep) _removeObjects.unshift(removeCallbackSync); @@ -500,7 +503,7 @@ function _prepareRemoveCallback(removeFunction, fileOrDirName, sync, cleanupCall if (index >= 0) _removeObjects.splice(index, 1); called = true; - if (sync) { + if (sync || removeFunction === FN_RMDIR_SYNC) { return removeFunction(fileOrDirName); } else { return removeFunction(fileOrDirName, next || function() {}); diff --git a/test/dir-sync-test.js b/test/dir-sync-test.js index 7afe4cc..b75da94 100644 --- a/test/dir-sync-test.js +++ b/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(); diff --git a/test/file-sync-test.js b/test/file-sync-test.js index 2c063ed..1dfb2c1 100644 --- a/test/file-sync-test.js +++ b/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(); + }); + }); }); }); }); diff --git a/test/inband-standard.js b/test/inband-standard.js index f56b457..f787b09 100644 --- a/test/inband-standard.js +++ b/test/inband-standard.js @@ -3,32 +3,30 @@ var assert = require('assert'), - fs = require('fs'), path = require('path'), - existsSync = fs.existsSync || path.existsSync, assertions = require('./assertions'), rimraf = require('rimraf'), tmp = require('../lib/tmp'); -module.exports = function inbandStandard(isFile, beforeHook) { - var testMode = isFile ? 0600 : 0700; - describe('without any parameters', inbandStandardTests({ mode: testMode, prefix: 'tmp-' }, null, isFile, beforeHook)); - describe('with prefix', inbandStandardTests({ mode: testMode }, { prefix: 'something' }, isFile, beforeHook)); - describe('with postfix', inbandStandardTests({ mode: testMode }, { postfix: '.txt' }, isFile, beforeHook)); - describe('with template and no leading path', inbandStandardTests({ mode: testMode, prefix: 'clike-', postfix: '-postfix' }, { template: 'clike-XXXXXX-postfix' }, isFile, beforeHook)); - describe('with template and leading path', inbandStandardTests({ mode: testMode, prefix: 'clike-', postfix: '-postfix' }, { template: path.join(tmp.tmpdir, 'clike-XXXXXX-postfix')}, isFile, beforeHook)); - describe('with name', inbandStandardTests({ mode: testMode }, { name: 'using-name' }, isFile, beforeHook)); - describe('with mode', inbandStandardTests(null, { mode: 0755 }, isFile, beforeHook)); - describe('with multiple options', inbandStandardTests(null, { prefix: 'foo', postfix: 'bar', mode: 0750 }, isFile, beforeHook)); +module.exports = function inbandStandard(isFile, beforeHook, sync = false) { + var testMode = isFile ? 0o600 : 0o700; + describe('without any parameters', inbandStandardTests({ mode: testMode, prefix: 'tmp-' }, null, isFile, beforeHook, sync)); + describe('with prefix', inbandStandardTests({ mode: testMode, prefix: 'tmp-' }, { prefix: 'tmp-something' }, isFile, beforeHook, sync)); + describe('with postfix', inbandStandardTests({ mode: testMode, prefix: 'tmp-' }, { postfix: '.txt' }, isFile, beforeHook, sync)); + describe('with template and no leading path', inbandStandardTests({ mode: testMode, prefix: 'tmp-clike-', postfix: '-postfix' }, { template: 'tmp-clike-XXXXXX-postfix' }, isFile, beforeHook, sync)); + describe('with template and leading path', inbandStandardTests({ mode: testMode, prefix: 'tmp-clike-', postfix: '-postfix' }, { template: path.join(tmp.tmpdir, 'tmp-clike-XXXXXX-postfix')}, isFile, beforeHook, sync)); + describe('with name', inbandStandardTests({ mode: testMode }, { name: 'tmp-using-name' }, isFile, beforeHook, sync)); + describe('with mode', inbandStandardTests(null, { mode: 0o755 }, isFile, beforeHook, sync)); + describe('with multiple options', inbandStandardTests(null, { prefix: 'tmp-multiple', postfix: 'bar', mode: 0o750 }, isFile, beforeHook, sync)); if (isFile) { - describe('with discardDescriptor', inbandStandardTests(null, { mode: testMode, discardDescriptor: true }, isFile, beforeHook)); - describe('with detachDescriptor', inbandStandardTests(null, { mode: testMode, detachDescriptor: true }, isFile, beforeHook)); + describe('with discardDescriptor', inbandStandardTests(null, { mode: testMode, discardDescriptor: true }, isFile, beforeHook, sync)); + describe('with detachDescriptor', inbandStandardTests(null, { mode: testMode, detachDescriptor: true }, isFile, beforeHook, sync)); } }; -function inbandStandardTests(testOpts, opts, isFile, beforeHook) { +function inbandStandardTests(testOpts, opts, isFile, beforeHook, sync = false) { return function () { opts = opts || {}; testOpts = testOpts || {}; @@ -51,7 +49,7 @@ function inbandStandardTests(testOpts, opts, isFile, beforeHook) { assertions.assertMode(this.topic.name, testOpts.mode || opts.mode); }.bind(topic)); - if (opts.prefix || testOpts.prefix) { + if(testOpts.prefix || opts.prefix) { it('should have the expected prefix', function () { assertions.assertPrefix(this.topic.name, testOpts.prefix || opts.prefix); }.bind(topic)); @@ -73,18 +71,26 @@ function inbandStandardTests(testOpts, opts, isFile, beforeHook) { }.bind(topic)); } - it('should have a working removeCallback', function (done) { - const self = this; - this.topic.removeCallback(function (err) { - if (err) return done(err); - try { - assertions.assertDoesNotExist(self.topic.name); - } catch (err) { - rimraf.sync(self.topic.name); - return done(err); - } - done(); - }); - }.bind(topic)); + if (sync) { + it('should have a working removeCallback', function () { + assert.ok(typeof this.topic.removeCallback === 'function'); + // important: remove file or dir unconditionally + rimraf.sync(this.topic.name); + }.bind(topic)); + } else { + it('should have a working removeCallback', function (done) { + const self = this; + this.topic.removeCallback(function (err) { + if (err) return done(err); + try { + assertions.assertDoesNotExist(self.topic.name); + } catch (err) { + rimraf.sync(self.topic.name); + return done(err); + } + done(); + }); + }.bind(topic)); + } }; } From be5fc21407327e9295b914052374749c2c351c6d Mon Sep 17 00:00:00 2001 From: Carsten Klein Date: Mon, 27 Jan 2020 17:32:27 +0100 Subject: [PATCH 3/4] update build matrices to include lastest releases from v8 onward --- .travis.yml | 2 ++ appveyor.yml | 2 +- package.json | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index f3dde6f..0f4c1ae 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,6 +1,8 @@ language: node_js node_js: - "node" + - "12" + - "11" - "10" - "9" - "8" diff --git a/appveyor.yml b/appveyor.yml index 743d881..c0d80e6 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -2,11 +2,11 @@ environment: matrix: - - nodejs_version: "7" - nodejs_version: "8" - nodejs_version: "9" - nodejs_version: "10" - nodejs_version: "11" + - nodejs_version: "12" install: - ps: Install-Product node $env:nodejs_version diff --git a/package.json b/package.json index 3973e2e..1b8e448 100644 --- a/package.json +++ b/package.json @@ -19,7 +19,7 @@ "url": "http://github.com/raszi/node-tmp/issues" }, "engines": { - "node": ">=6" + "node": ">=8" }, "dependencies": { "rimraf": "^3.0.0" From 119f3114e82b218f55ff91341d81140a260dc495 Mon Sep 17 00:00:00 2001 From: Carsten Klein Date: Mon, 27 Jan 2020 17:52:48 +0100 Subject: [PATCH 4/4] fix #197: put guard around call to sync cleanup callback --- test/inband-standard.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/test/inband-standard.js b/test/inband-standard.js index f787b09..25894bd 100644 --- a/test/inband-standard.js +++ b/test/inband-standard.js @@ -73,9 +73,16 @@ function inbandStandardTests(testOpts, opts, isFile, beforeHook, sync = false) { if (sync) { it('should have a working removeCallback', function () { - assert.ok(typeof this.topic.removeCallback === 'function'); - // important: remove file or dir unconditionally - rimraf.sync(this.topic.name); + try { + this.topic.removeCallback(); + } catch (err) { + // important: remove file or dir unconditionally + try { + rimraf.sync(this.topic.name); + } catch (_ignored) { + } + throw err; + } }.bind(topic)); } else { it('should have a working removeCallback', function (done) {