Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-39349: Add *cancel_futures* to Executor.shutdown() #18057
Conversation
If *cancel_futures* is ``True``, this method will cancel all pending | ||
futures that the executor has not started running. Any futures that | ||
are completed or running won't be cancelled, regardless of the value | ||
of *cancel_futures*. |
This comment has been minimized.
This comment has been minimized.
brianquinlan
Jan 21, 2020
Contributor
I think that you might need to explain what happens when cancel_futures
And wait
are both true? Does wait
cause shutdown
to not return until all of the currently running futures are finished?
with self._shutdown_lock: | ||
self._shutdown = True | ||
self._work_queue.put(None) | ||
if cancel_futures: | ||
while True: |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
aeros
Jan 22, 2020
Author
Member
Sure, I can add a comment here. The reason I didn't before is because very similar logic is used directly above in _initializer_failed()
, it's effectively the same process of draining the queue with a few changes. But, since this is a public method, I think it makes sense to include a brief description here as well.
@@ -342,6 +342,31 @@ def test_hang_issue12364(self): | |||
for f in fs: | |||
f.result() | |||
|
|||
def test_cancel_futures(self): |
This comment has been minimized.
This comment has been minimized.
brianquinlan
Jan 21, 2020
Contributor
Could you test some more scenarios? Like when wait
is also true? When called and the interpreter exits.
This comment has been minimized.
This comment has been minimized.
aeros
Jan 22, 2020
Author
Member
I addressed the part about wait
in #18057 (comment), but I'm not certain about how to write tests that account for the interpreter exiting. Could you elaborate on what that might look like? I'd certainly be glad to add some additional test coverage.
@@ -660,9 +665,12 @@ def map(self, fn, *iterables, timeout=None, chunksize=1): | |||
timeout=timeout) | |||
return _chain_from_iterable_of_lists(results) | |||
|
|||
def shutdown(self, wait=True): | |||
def shutdown(self, wait=True, cancel_futures=False): |
This comment has been minimized.
This comment has been minimized.
brianquinlan
Jan 22, 2020
Contributor
Maybe new arguments should be keyword-only - there is at least one other PR out now aiming to add an argument to shutdown
.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The current version of the documentation and tests already does cover what happens when That being said, I think it would be worth elaborating on the behavior when ThreadPoolExecutor - All work items still in the queue (meaning they haven't been assigned to a thread yet) are removed from the queue, and their associated futures are cancelled. This occurs regardless of the value of When The difference between using ProcessPoolExecutor - Most of the above applies, but underlying details differ a bit. In PPE, the pending work items are not directly accessible in This results in a bit of delay from when the flag is set to when the pending futures are cancelled, but I think this is the best way to implement cancel_futures for PPE without causing substantial performance losses. IMO, it's preferable to lose out on cancelling a few pending futures and have better overall performance. Admittedly though, I don't have a strong understanding of the specifics that occur though when I suspect it has to do with the way non-joined processes are finalized during interpreter shutdown, but that's not an area I'm particularly knowledgeable with. Maybe @pitrou could clarify? I just found this out while writing an example to demonstrate the above. In my own personal usage of Examples: Interaction between wait and cancel_futures: https://gist.github.com/aeros/2e73c8d6dccc94fd863967715826c78d PPE deadlock demo: https://gist.github.com/aeros/d1ff62b730426584413bca0c8f2ed99d |
This comment has been minimized.
This comment has been minimized.
Thanks for the review @brianquinlan! I'll go ahead and make some of the recommend changes while waiting on your response to the above comment. |
aeros commentedJan 19, 2020
•
edited by bedevere-bot
https://bugs.python.org/issue39349