feat: add options to control descriptor management #94
Conversation
This commit adds two options to control how tmp manages the file descriptor that results from creating the temporary file. Fixes raszi#88 Fixes raszi#91 Signed-off-by: Peter A. Bigot <[email protected]>
| @@ -220,6 +220,14 @@ 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 _invokeCallback(err) { | |||
| cb(null, name, undefined, _prepareTmpFileRemoveCallback(name, -1, opts)); | |||
There was a problem hiding this comment.
the error from fs.close() gets lost and must be passed to cb().
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
do you know about sinonjs? you could temporarily replace fs.close with a mock that would then call the callback with a fake error.
There was a problem hiding this comment.
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).
|
nice. with this in place, the api could be extended for creating tmpWriteStream()s as someone suggested in #46. Thinking of it, what sense does it make to create a tmp readstream from a new and by that empty file 😀. |
Addresses comment in pull request raszi#94 Signed-off-by: Peter A. Bigot <[email protected]>
|
Sorry for the late answer, I love this change as well. Unfortunately the source code needs some refactoring but I don't have that much time for this project. |
raszi
changed the title
This is a proposed implementation supporting issue #88 and providing a solution to the problem identified in issue #91. This works on Linux. I'm unable to test on Windows, and am unsure whether it behaves the same as POSIX systems when a descriptor is kept open after the file is removed.
Let me know if you find this worthwhile, or would like changes made before merging.
Thanks in advance for considering this.