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

Dir is not cleaned when I hit ctrl+c #121

Closed
JanBednarik opened this issue May 9, 2017 · 28 comments · May be fixed by mike-north/devcert#45 or sekikn/avro#9
Closed

Dir is not cleaned when I hit ctrl+c #121

JanBednarik opened this issue May 9, 2017 · 28 comments · May be fixed by mike-north/devcert#45 or sekikn/avro#9
Assignees

Comments

@JanBednarik
Copy link

I'm using tmp package in unit tests with tmp.setGracefulCleanup();. I'm creating temp directory for tests with unsafeCleanup = true.

It works fine if I don't hit ctrl+c to interrupt tests. Then the temp dir is not deleted. How can I get it deleted automatically in this case?

@silkentrance
Copy link
Collaborator

silkentrance commented Jun 8, 2017

@JanBednarik Please provide additional information: OS, tmp version, node version etc.

Could you also give us a link to your repository, so that we may look at the code directly?

@silkentrance silkentrance self-assigned this Jun 8, 2017
@silkentrance
Copy link
Collaborator

@JanBednarik ping

@JanBednarik
Copy link
Author

Versions:
OS X Sierra 10.12.5
Node LTS 6.11.0
tmp 0.0.31
jest 20.0.3

Should cleanup work with ctrl+c? Maybe it's somehow related to Jest test runner. I'll try to prepare some simple failing case.

@JanBednarik
Copy link
Author

JanBednarik commented Jun 30, 2017

@silkentrance Here is an example:

File: package.json

{
  "name": "tmptest",
  "scripts": {
    "test": "jest"
  },
  "dependencies": {
    "jest": "20.0.4",
    "tmp": "0.0.31"
  }
}

File: main.test.js

'use strict';

const tmp = require('tmp');

tmp.setGracefulCleanup();

test('main', done =>
    tmp.dir({ template: 'tmp-XXXXXX' }, (err, path) => {
        console.log(`dir: ${path}\npress ctrl+c`);
        setTimeout(done, 3000);
    })
);

Run:

$ npm install
$ npm test

And interrupt the test with ctrl+c.

@silkentrance
Copy link
Collaborator

@JanBednarik thank your very much. I think that I have found the problem. There exists a SIGINT event and tmp should be listening on that, too. The only problem is that under Windows one must take a different approach. I will look into this.

@davidshepherd7
Copy link

This package might be helpful. It's supposed to allow you to run code no matter how the process exits. It has Windows tests so presumably it works on Windows too.

@silkentrance
Copy link
Collaborator

@davidshepherd7 the package looks good. I will try that.

@silkentrance
Copy link
Collaborator

silkentrance commented Nov 30, 2017

@davidshepherd7 I tried signal-exit, but to no avail so far. In addition, it also installs listeners for SIGHUP which I believe is not what we want as most user code uses this to implement some way of applying changes to the configuration etc. And we do not want to clean up existing tmp files or directories prematurely.

https://stackoverflow.com/questions/10021373/what-is-the-windows-equivalent-of-process-onsigint-in-node-js

silkentrance added a commit that referenced this issue Nov 30, 2017
silkentrance added a commit that referenced this issue Nov 30, 2017
silkentrance added a commit that referenced this issue Dec 1, 2017
@silkentrance
Copy link
Collaborator

silkentrance commented Dec 1, 2017

@JanBednarik perhaps you want to try the https://github.com/raszi/node-tmp/tree/gh-121 branch

If you do not know already, you can configure the tmp dependency so that it will point to that branch. See https://docs.npmjs.com/files/package.json#git-urls-as-dependencies and https://docs.npmjs.com/files/package.json#github-urls for more information.

I would appreciate it if you'd provide us with a report of your experience with that branch.
I myself tested this on OSX so it might work for you, too.

@silkentrance
Copy link
Collaborator

silkentrance commented Dec 1, 2017

@JanBednarik since you are using jest, the branch also incorporates a fix for the problem with jest's inband sandboxing and tmp installing its process.exit listeners multiple times. See #129 and #125.

This was referenced Dec 1, 2017
silkentrance added a commit that referenced this issue Dec 2, 2017
silkentrance added a commit that referenced this issue Dec 2, 2017
silkentrance added a commit that referenced this issue Dec 2, 2017
silkentrance added a commit that referenced this issue Dec 26, 2017
silkentrance added a commit that referenced this issue Dec 27, 2017
@cythrauL
Copy link

cythrauL commented Jan 5, 2018

same problem,
OSX Sierra: 10.13.1
tmp version: 0.0.33
node version: v8.9.1

Also using a custom dir, postfix and prefix.

@dustinjohnson13
Copy link

The provided branch gh-121 works perfectly for me with CTRL-C for both files and directories. Thanks!

@silkentrance
Copy link
Collaborator

@JanBednarik @cythrauL @davidshepherd7 the current solution, which calls upon process.exit(0) needs to be revised. Upon SIGINT the process should at least exist with a non zero (0) exit code.

silkentrance added a commit that referenced this issue Apr 30, 2018
silkentrance added a commit that referenced this issue Apr 30, 2018
@silkentrance
Copy link
Collaborator

I have added one more commit to fix the tests for this issue. They seem to work, at least under OS X.

silkentrance added a commit that referenced this issue Sep 12, 2018
silkentrance added a commit that referenced this issue Sep 12, 2018
silkentrance added a commit that referenced this issue Sep 12, 2018
silkentrance added a commit that referenced this issue Oct 6, 2018
silkentrance added a commit that referenced this issue Oct 6, 2018
@silkentrance
Copy link
Collaborator

Tests work fine on travis, but fail on appveyor. It seems as if the tests do not terminate the external processes as one would expected. Fricking windows platform.

@stephanebachelier
Copy link

Any update ?

@sheerun
Copy link

sheerun commented Mar 13, 2019

We could do it ourselves if tmp exposed method like .forceCleanup() that removes all temporary directories created so far. Many tools like jest expose global teardown hooks.

@silkentrance
Copy link
Collaborator

The fix in branch gh-121 is working, however, the tests will not work under windows as we are having problems with simulating the CTRL-C there. And there are some conflicts that need to be resolved first prior to merging this to master. If I find the time, I can at least prepare everything so that it can be merged to master, disabling the tests on windows for now.

silkentrance added a commit that referenced this issue Mar 13, 2019
silkentrance added a commit that referenced this issue Mar 16, 2019
silkentrance added a commit that referenced this issue Mar 16, 2019
silkentrance added a commit that referenced this issue Mar 16, 2019
silkentrance added a commit that referenced this issue Mar 16, 2019
silkentrance added a commit that referenced this issue Mar 16, 2019
@silkentrance
Copy link
Collaborator

silkentrance commented Mar 16, 2019

I am giving up on support for SIGINT on Windows. This platform just sucks as node will never
invoke the SIGINT listener using standard kill(signal) mechanisms, which actually works just fine on POSIX based platforms such as OSX and Linux.

And I am not inclined to add any support for invoking a "PowerShell" script that will terminate the external process in our tests.

The current state is that it will work fine on both OSX and Linux.

silkentrance added a commit that referenced this issue Mar 16, 2019
@silkentrance
Copy link
Collaborator

On travis the tests for this still fail, locally (OSX) they will succeed, working on that issue.

silkentrance added a commit that referenced this issue Mar 16, 2019
silkentrance added a commit that referenced this issue Mar 16, 2019
dropping support for node v6.x as it is not working correctly with the installed SIGINT handlers
silkentrance added a commit that referenced this issue Mar 16, 2019
dropping support for node v6.x as it is not working correctly with the installed SIGINT handlers
add appveyor build for node 11
silkentrance added a commit that referenced this issue Mar 18, 2019
dropping support for node v6.x as it is not working correctly with the installed SIGINT handlers
add appveyor build for node 11
silkentrance added a commit that referenced this issue Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants