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

gh-128041: Add a terminate_workers method to ProcessPoolExecutor #128043

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

csm10495
Copy link
Contributor

@csm10495 csm10495 commented Dec 17, 2024

Provides a way to forcefully stop all the workers in the pool

Typically this would be used as a last effort to stop all workers if unable to shutdown / join in the expected way.

Provides a way to forcefully stop all the workers in the pool

Typically this would be used as a last effort to stop all workers if unable to shutdown / join in the expected way
Copy link

cpython-cla-bot bot commented Dec 17, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

Lib/concurrent/futures/process.py Outdated Show resolved Hide resolved
Lib/concurrent/futures/process.py Outdated Show resolved Hide resolved
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some additional comments. It would be great to have type checks test, e.g., when you pass an invalid signal value (namely, check that os.kill would raise a TypeError / ValueError).

Lib/test/test_concurrent_futures/test_process_pool.py Outdated Show resolved Hide resolved
Lib/test/test_concurrent_futures/test_process_pool.py Outdated Show resolved Hide resolved
Lib/test/test_concurrent_futures/test_process_pool.py Outdated Show resolved Hide resolved
Lib/test/test_concurrent_futures/test_process_pool.py Outdated Show resolved Hide resolved
Lib/test/test_concurrent_futures/test_process_pool.py Outdated Show resolved Hide resolved
Doc/library/concurrent.futures.rst Outdated Show resolved Hide resolved
Doc/library/concurrent.futures.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.14.rst Show resolved Hide resolved
@csm10495
Copy link
Contributor Author

Thanks @picnixz, I think I resolved all of your comments, please check again when you can.

@csm10495
Copy link
Contributor Author

@picnixz picnixz requested a review from gpshead December 17, 2024 23:43
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some other comments. I'll let @gpshead take over the review for a more in-depth knowledge (I'm not enough well-versed in concurrent.futures since I mainly use multiprocessing instead).

Lib/concurrent/futures/process.py Outdated Show resolved Hide resolved
Lib/concurrent/futures/process.py Outdated Show resolved Hide resolved
Lib/concurrent/futures/process.py Outdated Show resolved Hide resolved
@@ -855,3 +856,31 @@ def shutdown(self, wait=True, *, cancel_futures=False):
self._executor_manager_thread_wakeup = None

shutdown.__doc__ = _base.Executor.shutdown.__doc__

def terminate_workers(self, signal=signal.SIGTERM):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, can we perhaps name it kill? (namely, you kill the pool's workers with the given signal) or is kill already taken (possibly for something else in the future)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. I thought this was explicit. I'm open for either or.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so after some research:

  • Process.terminate -- mimics SIGTERM
  • Process.kill -- sends SIGKILL to the process.

Many other interfaces use terminate for sending SIGTERM and kill for sending SIGKILL. Now, we also have multiprocessing.Pool.terminate, so maybe we could mimic it by just naming it terminate(signal=...)? It would be slightly inconsistent with the others terminate methods since this one would be able to send a SIGKILL as well.

So it's up to you (or up to Gregory)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could definitely be down for terminate to match multiprocessing.Pool. (Though i'd still like to be able to change the signal sent so it would be a slight api difference). I'll wait to see what Gregory thinks to either change it or leave as is.

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.

2 participants