Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add options to control descriptor management #94

Merged
merged 2 commits into from
Nov 1, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,47 @@ console.log("File: ", tmpobj.name);
console.log("Filedescriptor: ", tmpobj.fd);
```

### Controlling the Descriptor

As a side effect of creating a unique file `tmp` gets a file descriptor that is
returned to the user as the `fd` parameter. The descriptor may be used by the
application and is closed when the `removeCallback` is invoked.

In some use cases the application does not need the descriptor, needs to close it
without removing the file, or needs to remove the file without closing the
descriptor. Two options control how the descriptor is managed:

* `discardDescriptor` - if `true` causes `tmp` to close the descriptor after the file
is created. In this case the `fd` parameter is undefined.
* `detachDescriptor` - if `true` causes `tmp` to return the descriptor in the `fd`
parameter, but it is the application's responsibility to close it when it is no
longer needed.

```javascript
var tmp = require('tmp');

tmp.file({discardDescriptor: true},
function _tempFileCreated(err, path, fd, cleanupCallback) {
if (err) throw err;
// fd will be undefined, allowing application to use fs.createReadStream(path)
// without holding an unused descriptor open.
});
```

```javascript
var tmp = require('tmp');

tmp.file({detachDescriptor: true},
function _tempFileCreated(err, path, fd, cleanupCallback) {
if (err) throw err;

cleanupCallback();
// Application can store data through fd here; the space used will automatically
// be reclaimed by the operating system when the descriptor is closed or program
// terminates.
});
```

### Asynchronous directory creation

Creates a directory with mode `0755`, prefix will be `myTmpDir_`.
Expand Down
24 changes: 23 additions & 1 deletion lib/tmp.js
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,26 @@ function _createTmpFile(options, callback) {
fs.open(name, CREATE_FLAGS, opts.mode || FILE_MODE, function _fileCreated(err, fd) {
if (err) return cb(err);

if (opts.discardDescriptor) {
return fs.close(fd, function _discardCallback(err) {
if (err) {
// Low probability, and the file exists, so this could be
// ignored. If it isn't we certainly need to unlink the
// file, and if that fails too its error is more
// important.
try {
fs.unlinkSync(name);
} catch (e) {
err = e;
}
return cb(err);
}
cb(null, name, undefined, _prepareTmpFileRemoveCallback(name, -1, opts));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the error from fs.close() gets lost and must be passed to cb().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd considered that and figured the chance of a close error at this point (where nobody else can see the file) is tiny, and besides the file was successfully created so the call should succeed.

An alternative perspective which is somewhat compelling is to return the error, but then we also must unlink the file. If that also errors, we have a problem because we can only return one error. In that case the unlink error should be returned instead of the close error, as it's more important for the programmer to know that an orphan file might be present than that a descriptor got leaked.

I've added a separate commit with this behavior so we can see what it looks like, but it can't be tested since I can't force a failure in the close operation without completely reworking tmp's unit test infrastructure.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you know about sinonjs? you could temporarily replace fs.close with a mock that would then call the callback with a fake error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do in fact know sinon, and I use it extensively in projects I maintain where I target 100% unit test coverage. I'm not going to add it to node-tmp, especially since its unit tests are already fragile (they leave a lot of garbage in /tmp after they're run, and are non-deterministic as to whether they can be safely run repeatedly).

});
}
if (opts.detachDescriptor) {
return cb(null, name, fd, _prepareTmpFileRemoveCallback(name, -1, opts));
}
cb(null, name, fd, _prepareTmpFileRemoveCallback(name, fd, opts));
});
});
Expand Down Expand Up @@ -345,7 +365,9 @@ function _createTmpDirSync(options) {
function _prepareTmpFileRemoveCallback(name, fd, opts) {
var removeCallback = _prepareRemoveCallback(function _removeCallback(fdPath) {
try {
fs.closeSync(fdPath[0]);
if (0 <= fdPath[0]) {
fs.closeSync(fdPath[0]);
}
}
catch (e) {
// under some node/windows related circumstances, a temporary file
Expand Down
5 changes: 5 additions & 0 deletions test/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ function _testGracefulSync(type, graceful, cb) {
_spawnTestWithoutError('graceful-sync.js', [ type, graceful ], cb);
}

function _assertNoDescriptor(err, name, fd) {
assert.strictEqual(fd, undefined);
}

function _assertName(err, name) {
assert.isString(name);
assert.isNotZero(name.length, 'an empty string is not a valid name');
Expand Down Expand Up @@ -141,6 +145,7 @@ module.exports.testGraceful = _testGraceful;
module.exports.testGracefulSync = _testGracefulSync;
module.exports.assertName = _assertName;
module.exports.assertNameSync = _assertNameSync;
module.exports.assertNoDescriptor = _assertNoDescriptor;
module.exports.testName = _testName;
module.exports.testNameSync = _testNameSync;
module.exports.testUnsafeCleanup = _testUnsafeCleanup;
Expand Down
54 changes: 54 additions & 0 deletions test/file-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,35 @@ function _testFile(mode, fdTest) {
};
}

function _testFileNoDescriptor(mode) {
return function _testFileNoDescriptor(err, name, fd) {
assert.ok(existsSync(name), 'should exist');

var stat = fs.statSync(name);
assert.equal(stat.size, 0, 'should have zero size');
assert.ok(stat.isFile(), 'should be a file');

Test.testStat(stat, mode);

assert.strictEqual(fd, undefined);
};
}

function _testFileAfterDetachRemove(mode) {
return function _testFileAfterDetachRemove(err, name, fd) {
assert.ok(!existsSync(name), 'File should be removed');

var fstat = fs.fstatSync(fd);
assert.equal(fstat.size, 0, 'should have zero size');
assert.ok(fstat.isFile(), 'should be a file');
Test.testStat(fstat, mode);

var data = new Buffer('something');
assert.equal(fs.writeSync(fd, data, 0, data.length, 0), data.length, 'should be writable');
assert.ok(!fs.closeSync(fd), 'should not return with error');
};
}

vows.describe('File creation').addBatch({
'when using without parameters': {
topic: function () {
Expand Down Expand Up @@ -93,6 +122,31 @@ vows.describe('File creation').addBatch({
}
},

'when using discardDescriptor': {
topic: function () {
tmp.file({ discardDescriptor: true }, this.callback);
},

'should not return with an error': assert.isNull,
'should return with a name': Test.assertName,
'should not return with a descriptor': Test.assertNoDescriptor,
'should be a file': _testFileNoDescriptor(0100600),
},

'when using detachDescriptor': {
topic: function () {
var cb = this.callback;
tmp.file({ detachDescriptor: true }, function (err, name, fd, removeCallback) {
removeCallback();
return cb(err, name, fd);
});
},

'should not return with an error': assert.isNull,
'should return with a name': Test.assertName,
'should have working descriptor after removeCallback': _testFileAfterDetachRemove(0100600),
},

'when using multiple options': {
topic: function () {
tmp.file({ prefix: 'foo', postfix: 'bar', mode: 0640 }, this.callback);
Expand Down