sindresorhus / fkill-cli

Fabulously kill processes. Cross-platform.
MIT License
6.88k stars 159 forks source link

Force kill if normal kill fails #61

Closed medusalix closed 4 years ago

medusalix commented 5 years ago

Forces a kill if the normal kill behavior fails. handleFkillError already includes functionality for handling the case of a failed kill, but fkill doesn't throw an error when it fails to kill a process, so I chose a different way to implement this feature.

Resolves #42


IssueHunt Summary IssueHunt has been backed by the following sponsors. [Become a sponsor](https://issuehunt.io/membership/members)
stroncium commented 5 years ago

This won't work. It doesn't allow process any time to terminate. Effectively it's just like setting force all the time, just slower.

medusalix commented 5 years ago

@stroncium It works just fine, you can try it out yourself if you want. The delay between fkill and checking if the process is still running is enough for most cases. There's no alternative solution because fkill uses killall on Linux, which doesn't report back whether the kill was successful.

stroncium commented 5 years ago

@medusalix That's definitely not true. The delay can be as small as 50ms, when it's easy to find processes which take many seconds to terminate. The least we can do here is provide a sensible default(which would probably be in a range of 1-2 seconds), and make wait time configurable.

Another thing which I missed originally is the opposite case, where process died really fast and it's pid was already reused when we check for process existence / force kill them. We should check if as much info about process as possible matches original process, otherwise we might be killing a process which wasn't ever intended to get killed.

As for the thing about using killall, kill/killall doesn't kill processes, it sends them SIGTERM, and of course it doesn't know what the process does to them, there is no such thing as successful kill.

medusalix commented 5 years ago

@stroncium It might not work when killing a real application process, that's definitely a good point. I just wanted to point out that the test case, which is a small Node.JS server, is force-killed just fine 😃.

I really like your solution of a configurable wait time, but I would introduce that setting through an optional flag, so we maintain compatibility with existing applications.

Another thing which I missed originally is the opposite case, where process died really fast and it's pid was already reused when we check for process existence / force kill them.

We should definitely add a test case for that too.

The killall command has the flag --wait, where it checks if the process is still running. But that seems to be unsuitable for our use case because it gets stuck in an infinite loop if the process ignores SIGTERM. So a manual solution is preferred.

sindresorhus commented 5 years ago

What would be a good default time?

I think this should be implemented as an option in https://github.com/sindresorhus/fkill first.

sindresorhus commented 4 years ago

@medusalix Still interested in finishing this?

medusalix commented 4 years ago

@sindresorhus Not really, far too busy at the moment. Feel free to close this or let somebody else finish it.

stroncium commented 4 years ago

I think I had something I wanted to do with fkill in near future, I'll try to look into this one too.