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

fix invokation of windows processes, so we can terminate() them #1740

Closed

Conversation

dothebart
Copy link
Contributor

one by one without shooting ourselves.

this takes care of:
#1737

@giampaolo
Copy link
Owner

I thought you were talking about the test suite. Here you're touching psutil itself. The point of terminate() method is being able to terminate any process, not only the ones created with the CREATE_NEW_PROCESS_GROUP flag. If that is your goal you should do it in your own code, registering your own signal handler. Doing so in a general purpose lib is not a good idea.

@giampaolo giampaolo closed this Apr 29, 2020
@dothebart
Copy link
Contributor Author

it will continue to be able to terminate any process. However, if you SPAWN them without, it will continue to kill all processes you spawned.

In the current implementation state psutils will simply allways hard-kill processes on windows allways which to that regard is way worse!

@giampaolo
Copy link
Owner

giampaolo commented Apr 29, 2020

psutil.Process class can be used with any PID. In this PR you are registering a signal handler and using exit(). Both of those things affect the current process (os.getpid()), and possibly its children, not the PID/process the Process instance is referring to.

@dothebart
Copy link
Contributor Author

dothebart commented Apr 29, 2020

since its only registered for SIGINT this case shouldn't happen? What would be a better way to do this?

@giampaolo
Copy link
Owner

You mean avoid killing the entire group of process/PID you did NOT spawn? I don't know. But it seems to me your use case is about the processes created by yourself (os.getpid()), so you already have a solution. You have all the bits already, so just use it in your own code. =)

To fully recap, changing Process.terminate() the way you propose is a no-go because:

  • it does not apply to any PID, just os.getpid()
  • it register a signal handler
  • it would change the semantic of Process.terminate(), which should be the same as subprocess.Popen.terminate(), which is calling TerminateProcess Windows API

With that said, I agree that having something more high level (like a parameter to pass to terminate() or something) to avoid killing the entire process group (of any PID), would be nice to have, but as of now I can't say I understand the problem well enough to have a solid solution.

@dothebart
Copy link
Contributor Author

the problem is, with whoever is spawning processes without using CREATE_NEW_PROCESS_GROUP will have a sigint sent to all processes in that group - be they parented from you or not.
Thus you cannot soft-kill (signal to terminate with a regular shutdown) single processes of a group alone.

Ok, I will have to fork psutils then and stop taking the work to do PRs.

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

Successfully merging this pull request may close these issues.

None yet

2 participants