search

jest-community / jest-editor-support

A module for handling editor to jest integration
MIT License
28 stars 21 forks source link

address orphan process issue #39

Closed connectdotz closed 4 years ago

connectdotz commented 4 years ago

see jest-community/vscode-jest#550, specifically the 2nd scenario mentioned here

when the spawned process spawns another child process in the background, such as in react-scripts use case, even when we close the jest process, the react-script spawned process will not be terminated.

It is clearly mentioned in nodejs child_process kill, I think we just missed it when switching to the shell option 😱 ...

On Linux, child processes of child processes will not be terminated when attempting to kill their parent. This is likely to happen when running a new process in a shell or with the use of the shell option of ChildProcess

anyway, there are a few possible ways to fix this issue, looks like on windows, which we have been using shell option way back, it spawned an external taskkill to kill all child processes. At least on Unix like systems, it seems we could utilize the system's "process group" mechanism to create and terminate the child processes without spawning another external command nor introducing a 3rd party library...

The "process group" mechanism in nodejs means to spawn a process with the "detached" option, which created a new process group that we can use later to kill every process within by process.kill() with the negative PID (served as PGID)

I tested this on mac and it seems to work as expected. I am a bit concern if this will work for all systems, so still keep the current behavior as the fallback.

Also not sure if the detached flag has any unexpected effect on windows and if we should only turn this on for non-windows?

@stephtr or anyone has a windows system, would appreciate some quick test to verify all is well there.

stephtr commented 4 years ago

On Windows this unfortunately spawns a couple of cmd windows ("On Windows [...] the child will have its own console window.").

Since, as far as I've tested, vscode-jest#550 doesn't affect Windows, I would enable detached just on non-Windows systems.

connectdotz commented 4 years ago

thanks, @stephtr. I made the change you suggested, please give it a try when you get a chance...

stephtr commented 4 years ago

Besides the failing test, it seems fine to me 👍

stephtr commented 4 years ago

I have only tested it on Windows, but to me the PR looks fine. Maybe someone should still test it on Linux, just to be sure.

jeantil commented 4 years ago

I have only tested it on Windows, but to me the PR looks fine. Maybe someone should still test it on Linux, just to be sure.

I run on linux and I am impacted by the issue, I'm interested in testing the fix but I haven't the faintest idea how to do that ...

connectdotz commented 4 years ago

@jeantil it's true that this is hard to test in isolation... I usually test it with vscode-jest, which we should cut a release ASAP then maybe you can help us to test the release candidate there?

@seanpoulter sorry I didn't see your review until now, I will address them and cut a release later after work.

Also, can you guys review the other leg of this issue in jest-community/vscode-jest#573 ? Hopefully we can cut a vscode-jest release candidate this weekend...

jeantil commented 4 years ago

I had never looked in details at how extensions are packaged but it looks like it's only two files produced by webpack. Building master with this PR included seems a bit involved but if you have a built extension.js I don't mind dropping it in place of the current one and restarting vscode. I encounter the orphan process issue very often I will let you know really quick if this fixes it and in which cases :)

connectdotz commented 4 years ago

@jeantil

here is a .vsix file from my local build (had to zip it as github won't accept .vsix file in comments)

vscode-jest-3.2.0.vsix.zip

you can follow vscode's instruction to install: https://code.visualstudio.com/docs/editor/extension-gallery#_install-from-a-vsix

Thanks for your help! If you don't feel comfortable taking a binary this way, I can fully understand... Hopefully, we should be able to produce the official candidate in a few days.

connectdotz commented 4 years ago

just realized @seanpoulter volunteered to refactor the tests later, sounds good I will leave them to you then. merging...

jeantil commented 4 years ago

I can confirm that using the provided vsix on linux the orphan processes issue disappeared