jest-process-manager icon indicating copy to clipboard operation
jest-process-manager copied to clipboard

timeout does not clean up processes started by 'command'

Open thernstig opened this issue 5 years ago • 9 comments

Set a command like:

module.exports = {
  command: 'npm run start',
  launchTimeout: 1000,
}

If the timeout occurs, you get this message:

Server has taken more than 1000ms to start.

The problem here is that jest-process-manager then never sends the proper signals to the processes started by the command, meaning they get hanging. As a comparison, pressing ctrl+c after the server has started properly cleans up the resources.

To Reproduce

Add the above config and a very short launchTimeout and notice how the started command never stops (check e.g. via the ps command).

Expected behavior

jest-process-manager should send the proper signals to launched command, even after a launchTimeout occured.

thernstig avatar Jun 01 '20 09:06 thernstig

Same issue here - trying to start a webpack server but when the timeout is exceeded, jest terminates but webpack keeps on running forever

olee avatar Nov 12 '20 15:11 olee

I know that it was quite old issue. But can someone produce representation? Cause I can't figure out on the problem

mmarkelov avatar Dec 12 '20 19:12 mmarkelov

@mmarkelov I am unsure if I read your sentence correctly, but do you mean if we can explain it better?

Try creating a long-running process in command and a short timeout for launchTimeout e.g. 10 ms. Then do a ps aux | grep command for the command you ran, and you will notice it got stuck hanging and never cleaned up properly.

When launchTimeout occurs, you need to send a SIGTERM followed by SIGKILL (after 10 sec which many tools do) to the PID started by command.

thernstig avatar Dec 13 '20 17:12 thernstig

@thernstig what is your OS? I can't reproduce it on my MAC machine

mmarkelov avatar Dec 14 '20 13:12 mmarkelov

@mmarkelov maybe this has been fixed since then? I can try again later possibly.

thernstig avatar Dec 15 '20 17:12 thernstig

@thernstig it will be nice. Also I added some cleanup functionality in catch block, so I hope it will help

mmarkelov avatar Dec 16 '20 07:12 mmarkelov

@mmarkelov I can still re-create this, so I took a look at the code. I believe I found multiple things that can cause errors, unless I am misreading the code.

  1. This is what I believe should kill the processes if the waitOn() times out from a normal timeout:

https://github.com/playwright-community/jest-process-manager/blob/0fd77b16c82df3ed63150fdba33265e742b0c46c/src/index.ts#L246-L257

Note that this only sends a killProc() to a port. I am not sure why this assumption is taken in the code. Even though a program might wait for a port, the kill signal should be sent to the process that was started by command. E.g. in our case we use npm start with starts several subprocesses through a program called concurrently. One of them is a server we wait for. But the kill signal should be sent to "npm start" and the process it starts, so it in turn can send the signal to concurrently.

  1. In the case no port is configured it looks like this:

https://github.com/playwright-community/jest-process-manager/blob/0fd77b16c82df3ed63150fdba33265e742b0c46c/src/index.ts#L258-L260

and this piece of code seems to properly call treeKill() on the spawned process.

  1. And if we come back to what was reported in this issue, it is that when I press ctrl+C (which sends a SIGINT), I cannot see that being handled anywhere in the code and sent to the appropriate process. I cannot see it in either case of config.port being present or not (the if-else branches).

Am I missing something in all of this?

thernstig avatar Dec 29 '20 16:12 thernstig

@thernstig So if I understood you correctly , I should check out the cases when port is not provided?

mmarkelov avatar Dec 31 '20 09:12 mmarkelov

Point 1 and 3 should be checked out. Point 1) is about a port being provided. Point 3) is about pressing ctrl+c (in both the port and no-port scenarios), then you need to send SIGINT to the processes started.

thernstig avatar Jan 01 '21 14:01 thernstig