Without tmp.setGracefulCleanup(), empty directories are not removed automatically when process exits

[simonbrent]

Operating System

Linux

NodeJS Version

10.11.0

Tmp Version

0.1.0

Expected Behavior

Empty directories created with tmp.dir are removed when the process exits

Experienced Behavior

Empty directories created with tmp.dir remain when the process exits

Code to demonstrate

const tmp = require('tmp');

const tmpDir = tmp.dir((err, path) => {
  console.log(err, path);
  process.exit();
});

const tmp = require('tmp');

const tmpDir = tmp.dirSync();
console.log(tmpDir.name);
process.exit();

In the above cases, once the process exits, ls [temp dir path] shows that the directory still exists, and is empty.

If I add tmp.setGracefulCleanup(); to the above examples, the directory is gone once the process exits.

[silkentrance]

@simonbrent this is by design. if graceful cleanup was not called, then the garbage collector will refrain from removing any temporary objects from the filesystem.

the main reason for this is so that you can view review your temporary files after your program exists. normally, you will want to enable graceful cleanup.

of course, one could argue that this should be the default behaviour, but changing it now might break existing software, which of course will always enable graceful cleanup...

so let me think whether we can keep the setter and extend it so that one can pass a boolean which will then turn on/off the default.

[silkentrance]

I will prepare a PR for this right away.

[silkentrance]

See PR #195

[silkentrance]

The rationale behind this is that under normal operation, users of tmp will always call setGracefulCleanup so that on process exit, the temporary files and empty directories will be removed from the temporary storage.

That being said, we can always also make this the default behaviour, and, in order to not break existing integrations, i.e. if someone calls setGracefulCleanup() without any parameter, the behaviour should be the same as before.

Calling setGracefulCleanup(false) will now disable that behaviour, similar to what it was before without setGracefulCleanup() being called at all.

If at all, this will only break a few fringe usages of tmp, and I think that we can live with that.

[simonbrent]

@silkentrance Thanks for the response.

In light of this explanation, I think my issue is really with the documentation (and my reading of it), rather than the behaviour of the library itself.

The README states for directory creation "Simple temporary directory creation, it will be removed on process exit", and the example code does not contain setGracefulCleanup (the same is true for the file creation section). I followed this example and removed the manual cleanup line, and was then surprised when the directory was not removed on process exit.

The discussion of setGracefulCleanup is right at the bottom of the README, and only talks about needing it to remove files/directories in the case of uncaught exceptions. Since you have pointed out that setGracefulCleanup must be called to get the directory removed in my example code, even though there are no uncaught exceptions, this documentation is misleading.

I think simply fixing these issues in the README would go a long way towards helping others avoid the confusion I suffered, without any need to change the behaviour of the code :-)

[silkentrance]

@simonbrent Yes, the documentation lacks a little bit in this regard. However, I do think that making this the default behaviour is actually a good think; who would want a bunch of temporary files and empty folders lying around after the process terminated?

We will have to wait and see what @raszi thinks about this. If he is fine with just the addition to the README.md then we can make this happen as well.

[raszi]

Actually I was thinking of completely removing this feature. This causes more harm and headache than any good.

We could provide a function what can be called and what would remove the leftover files, but let's not try to capture any kind of event.

[silkentrance]

@raszi there is no event involved in here except for the exit handler, which now, by default, will remove all stale tmp files and directories, even if they have been populated.

and if the user wants to keep the populated (or empty) directories, then he must call setGracefulCleanup(false).

[raszi]

I am talking about this part:

https://github.com/raszi/node-tmp/blob/master/lib/tmp.js#L602-L678

[joma74]

To chime in, consider the following case

import onExit from  "async-exit-hook"
...
this.profileDir = tmp.dirSync({
      dir: TESTCAFE_EXT_TMP_DIRS_ROOT,
      prefix: namePrefix + "-",
      keep: false,
      unsafeCleanup: true,
    })
onExit(this.profileDir.removeCallback)

As of tmp@0.1.0, observed that when hooking in that async exit handler to tmp's 'removeCallback', the dirs are not removed. Deeper cause is that tmp's callback triggers a rimraf function that in turn calls node's async fs.lstat with a callback that never runs. Hmm, maybe a local problem. See also https://github.com/tapppi/async-exit-hook#considerations-and-warning for caveats.

So i opted for

tmp.setGracefulCleanup()
...
this.profileDir = tmp.dirSync({
      dir: TESTCAFE_EXT_TMP_DIRS_ROOT,
      prefix: namePrefix + "-",
      keep: false,
      unsafeCleanup: true,
    })

and all dirs were deleted. Difference is that on 'tmp.setGracefulCleanup()', tmp calls _rimrafRemoveDirSyncWrapper directly, which, by using node's sync fs.lstatSync, works.

Point is that i feel that users should have an option to decide if they want a sync version of tmp's 'removeCallback'. Even more if you ever decide to remove setGracefulCleanup with it's exit listeners.

[silkentrance]

@joma74 that is exactly why, under the hood, tmp uses rimraf synchronously, especially in its exit and sigint handlers.

as for user callbacks and the cleanup callbacks passed to them, they have always been async, even in the past, so there was no change to that behaviour.

[silkentrance]

This should have been fixed with commit https://github.com/raszi/node-tmp/commit/943853907e9952595b4832286402985a49978890#diff-4f99eaef47493ba13b06879592d2a1c4R501. node introduced a second option to rmdirSync. In the past we just passed the optional next callback handler, relying on rmdirSync to just ignore any additional parameters. The same behaviour is now on master which incorporates the bug fix for #197.