Current (unreleased) version does not remove temp file on exit

[robinhouston]

If I run the following tiny script with the released version of node-tmp, the file does not exist after the node process has exited:

require("tmp").file(function(error, filename, fd) {
    console.log("Temp file", filename);
});

But with the current master branch version (3c28d1cd5059405a1cba8e69c324d8e7dbab2224), it does.

[silkentrance]

@robinhouston good find! thanks. in fact, on master we can see

      if (opts.discardDescriptor) {
        return fs.close(fd, function _discardCallback(err) {
      ...
     }
      /* istanbul ignore else */
      if (opts.detachDescriptor) {
        return cb(null, name, fd, _prepareTmpFileRemoveCallback(name, -1, opts));
     }

something must have gone wrong with the latest merges/rebases...

[silkentrance]

@robinhouston do you by any chance call tmp.setGracefulCleanup(). if not, then you will have to call the remove callback you receive in your callback yourself, otherwise the file will not be cleaned up on process exit.

E.g.

require("tmp").file(function(error, filename, fd, removeCallback) {
    console.log("Temp file", filename);
        removeCallback();
});

or otherwise do

tmp = require('tmp');
tmp.setGracefulCleanup();
...

[robinhouston]

@silkentrance Is that right? I thought the cleanup happened anyway on process exit, and that setGracefulCleanup only affects whether cleanup happens if there’s an uncaught exception.

In any case the behaviour certainly differs between master and the released version, which doesn’t seem to be intentional.

[silkentrance]

@robinhouston the cleanup only occurs if you call the remove callback yourself. AFAIK, the previous version did not handle it correctly.

[silkentrance]

@robinhouston and the readme states that

One may want to cleanup the temporary files even when an uncaught exception occurs [...] to enforce this [...]

So, basically, the graceful cleanup is good for both use cases, first, you forget to call the removal callback yourself, and, second, in case that there is an uncaught exception.

[silkentrance]

@robinhouston I am closing this as it is the expected behaviour.

[robinhouston]

I don’t mind what the behaviour is, but I think if you’re going to change this it should be a major version bump & needs revised documentation. This change will break production systems.

[silkentrance]

@robinhouston we are currently working on this, making it a 1.0.0 release, but somehow @raszi is rather reluctant on making this happen before every single issue has been fleshed out :grin: