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

Force kill if normal kill fails #61

Closed
wants to merge 1 commit into from
Closed

Force kill if normal kill fails #61

wants to merge 1 commit into from

Conversation

medusalix
Copy link

@medusalix medusalix commented Mar 4, 2019

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

@stroncium
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Contributor

stroncium commented Mar 6, 2019

@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
Copy link
Author

medusalix commented Mar 6, 2019

@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
Copy link
Owner

sindresorhus commented Jun 20, 2019

What would be a good default time?

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

@sindresorhus
Copy link
Owner

@medusalix Still interested in finishing this?

@medusalix
Copy link
Author

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

@stroncium
Copy link
Contributor

stroncium commented Feb 16, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Process is not killed
3 participants