raszi / node-tmp

Temporary file and directory creator for node.js
MIT License
736 stars 92 forks source link

Temporary files not beeing removed after tests are completed (Jest, Windows) #229

Closed armano2 closed 4 years ago

armano2 commented 4 years ago

Operating System

NodeJS Version

Tmp Version

0.1.0 and master

Expected Behavior

Temporary files should be removed after test is finishes

Experienced Behavior

Temporary files are not removed


I'm experiencing this issues within jest tests, this can be reproduced in

If needed i can prepare small repo project or create test scenario

armano2 commented 4 years ago

i did some digging and this seems to be issue with jest isolating tests process events are not being triggered and cleanup is not triggered

silkentrance commented 4 years ago

I will have a look at this.

silkentrance commented 4 years ago

@armano2 Ah I see. Now, there was a change to the rmdirSync API which expects a second optional parameter. ATM we will pass the next handler to this function, too, which causes node fs rmdirSync to fail.

There is already a bug fix for that. Perhaps you can try out node-tmp/master?

I am currently working on stabilising the solution and getting the event handling stuff right. Hopefully @raszi will then make a new release :smile:

Other than that: this should be a duplicate of #209

silkentrance commented 4 years ago

Reopening, seeing that you already tried out master.

silkentrance commented 4 years ago

I just tried this with the latest master and running the tests from your project.

Turns out that the temporary objects are cleaned up as expected.

However, the SIGINT handler will not be called on CTRL-C. But this is a different issue.

Can you please check this again with the latest master?

silkentrance commented 4 years ago

There is a bug in current master on dirSync/removeCallback. (next is not a function.) The problem lies in the rimrafSync wrapper for removing directories. Will open a new issue for this, see #230.

silkentrance commented 4 years ago

@armano2 is this the issue that the SIGINT handler will not be called?

armano2 commented 4 years ago

yes, i did some debugging and it looks like process.on and process.addEventListener is not triggered at all

silkentrance commented 4 years ago

@armano2 thank you.

I just did some debugging and by looking at the jest code, it seems that they actually switched over to worker threads. In the past, they would just import the test suite module and provide it with a more or less empty sandbox context. This lead to the fact that tmp got loaded multiple times, installing its listeners over and over again.

With the workers now in place, the test suite module will be loaded in the context of the so spawned worker thread and tmp will be loaded once per worker thread, no longer installing its listeners multiple times.

In order to test this, I added the import for tmp to jest-cli and put some debug output in the safe listener installer, logging the available listeners. It shows that the process object is different from the worker thread's process.

https://nodejs.org/api/worker_threads.html#worker_threads_class_worker

And ATM I do not see any way of making SIGINT/EXIT handlers work in that scenario as these signals will not be passed to the worker thread.

silkentrance commented 4 years ago

Here is a small diagram on how the process environment looks like and which handlers will be called

main process (standard SIGINT handler is used) -- worker thread (will be terminated as soon as the main process is interrupted, tmp exit handler will not be called)

silkentrance commented 4 years ago

The only work around for this would be to use inband testing alone using the -i option.

However, initial tests have shown that this will also not work.

silkentrance commented 4 years ago

I dug somewhat deeper and the enableWorkerThreads option is not set, so jest is actually using child processes.

silkentrance commented 4 years ago

Nah, it is basically the same as with the worker threads. SIGINT will be received by the main process and the child processes will just be terminated. However, the exit handler should still be called during normal execution, similarly to our own tests.

silkentrance commented 4 years ago

@armano2 I have found a solution to this, see the gist https://gist.github.com/silkentrance/f0e1ff1d689891e124e2a8b73bd31299

silkentrance commented 4 years ago

Somehow all this runincontext stuff will break the process event listeners.

armano2 commented 4 years ago

thank you for investigating it, maybe you should consider, creating guide for other people in readme how to use within jest :>

silkentrance commented 4 years ago

@armano2 I an currently working on a package for doing exactly that. I will reference the package from the documentation. Got a little bit carried away, though :)

silkentrance commented 4 years ago

@armano2 can we close this for now?