[issue39349] Add "cancel_futures" parameter to concurrent.futures.Executor.shutdown()

[Kyle Stanley]

Kyle Stanley  added the comment:

Thanks for bringing attention to cancel_futures being missing in the base 
Executor class and for the PR, Shantanu!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39349] Add "cancel_futures" parameter to concurrent.futures.Executor.shutdown()

[Kyle Stanley]

Kyle Stanley  added the comment:


New changeset a763ee3c583e6a2dfe1b1ac0600a48e8a978ed50 by Shantanu in branch 
'3.9':
[3.9] bpo-39349: Add cancel_futures to Executor.shutdown base class (GH-22023) 
(GH-22048)
https://github.com/python/cpython/commit/a763ee3c583e6a2dfe1b1ac0600a48e8a978ed50


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39349] Add "cancel_futures" parameter to concurrent.futures.Executor.shutdown()

[Shantanu]

Change by Shantanu :


--
pull_requests: +21145
pull_request: https://github.com/python/cpython/pull/22048

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39349] Add "cancel_futures" parameter to concurrent.futures.Executor.shutdown()

[Kyle Stanley]

Kyle Stanley  added the comment:


New changeset 17dc1b789ecc33b4a254eb3b799085f4b3624ca5 by Shantanu in branch 
'master':
bpo-39349: Add cancel_futures to Executor.shutdown base class (GH-22023)
https://github.com/python/cpython/commit/17dc1b789ecc33b4a254eb3b799085f4b3624ca5


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39349] Add "cancel_futures" parameter to concurrent.futures.Executor.shutdown()

[Shantanu]

Change by Shantanu :


--
pull_requests: +21123
pull_request: https://github.com/python/cpython/pull/22023

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39349] Add "cancel_futures" parameter to concurrent.futures.Executor.shutdown()

[Kyle Stanley]

Kyle Stanley  added the comment:

Good catch, Shantanu. It was not intentional on my part, and it would make 
sense to include *cancel_futures* in the base Executor class as documented. If 
you'd like to submit a PR to add it (attaching it to this issue), I should be 
able to review it within a reasonable period.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39349] Add "cancel_futures" parameter to concurrent.futures.Executor.shutdown()

[Shantanu]

Shantanu  added the comment:

I was working on updating typeshed stubs to reflect this change. It looks like 
the parameter wasn't actually added to the base class 
(https://github.com/python/cpython/blob/c3a651ad2544d7d1be389b63e9a4a58a92a31623/Lib/concurrent/futures/_base.py#L608),
 even though it's documented as having the new parameter 
(https://docs.python.org/3.9/library/concurrent.futures.html#concurrent.futures.Executor.shutdown).

Is this intentional? If not, I'd be happy to submit a PR adding the parameter 
to the base class.

--
nosy: +hauntsaninja

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39349] Add "cancel_futures" parameter to concurrent.futures.Executor.shutdown()

[Kyle Stanley]

Kyle Stanley  added the comment:

> Thank you very muck, Kyle!

No problem. Thanks for reviewing and merging the PR, Antoine!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39349] Add "cancel_futures" parameter to concurrent.futures.Executor.shutdown()

[Antoine Pitrou]

Antoine Pitrou  added the comment:


New changeset 339fd46cb764277cbbdc3e78dcc5b45b156bb6ae by Kyle Stanley in 
branch 'master':
bpo-39349: Add *cancel_futures* to Executor.shutdown() (GH-18057)
https://github.com/python/cpython/commit/339fd46cb764277cbbdc3e78dcc5b45b156bb6ae


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39349] Add "cancel_futures" parameter to concurrent.futures.Executor.shutdown()

[Antoine Pitrou]

Antoine Pitrou  added the comment:

Thank you very muck, Kyle!

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39349] Add "cancel_futures" parameter to concurrent.futures.Executor.shutdown()

[Kyle Stanley]

Change by Kyle Stanley :


--
keywords: +patch
pull_requests: +17451
stage: needs patch -> patch review
pull_request: https://github.com/python/cpython/pull/18057

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39349] Add "cancel_futures" parameter to concurrent.futures.Executor.shutdown()

[Kyle Stanley]

Kyle Stanley  added the comment:

I now have a working implementation, for both ThreadPoolExecutor and 
ProcessPoolExecutor. I've also ensured that the tests I added are not 
vulnerable to race conditions with the following:

```
[aeros:~/repos/aeros-cpython]$ ./python -m test test_concurrent_futures --match 
test_cancel_futures -j200 -v -F

[snip]
Ran 4 tests in 2.865s

OK
0:03:24 load avg: 143.25 [1018] test_concurrent_futures passed -- running: 
test_concurrent_futures (2 min 36 sec), test_concurrent_futures (35.8 sec)
test_cancel_futures 
(test.test_concurrent_futures.ProcessPoolForkProcessPoolShutdownTest) ... 0.57s 
ok
test_cancel_futures 
(test.test_concurrent_futures.ProcessPoolForkserverProcessPoolShutdownTest) ... 
0.80s ok
test_cancel_futures 
(test.test_concurrent_futures.ProcessPoolSpawnProcessPoolShutdownTest) ... 
0.53s ok
test_cancel_futures (test.test_concurrent_futures.ThreadPoolShutdownTest) ... 
0.20s ok
```

I'll attach a PR to the issue once I finish writing the documentation and 
"What's New" entry.

Note: I was originally going to do this in two separate PRs, one for each 
executor, but it seemed to make more sense to just have it as a single cohesive 
PR since Executor.shutdown() shares the same documentation for both executors.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39349] Add "cancel_futures" parameter to concurrent.futures.Executor.shutdown()

[Kyle Stanley]

Kyle Stanley  added the comment:

> It was a bit more involved than I originally anticipated, as I had to make a 
> minor change in the _worker() function to allow the new parameter to be 
> compatible with wait (which is important, as it prevents dangling threads).

Never mind, I just realized that I could simply add work_queue.put(None) at the 
end of the queue drain to unblock the indefinitely hanging thread. So, the 
global constant and change in logic for _worker() isn't needed.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39349] Add "cancel_futures" parameter to concurrent.futures.Executor.shutdown()

[Kyle Stanley]

Kyle Stanley  added the comment:

As of now, I have the implementation for ThreadPoolExecutor working correctly, 
and a unit test added to ensure its behavior. It was a bit more involved than I 
originally anticipated, as I had to make a minor change in the _worker() 
function to allow the new parameter to be compatible with wait (which is 
important, as it prevents dangling threads).

With my initial implementation, using "wait=True" and "cancel_futures=True" was 
resulting in any threads that called work_queue.get(block=True) to hang 
indefinitely. In order to remedy this, I changed it to use 
work_queue.get_nowait(). If a queue.Empty exception occurs, it checks for a 
global constant _cancelling_futures (set to True just in shutdown before it 
starts draining the work queue). If it's true, the while True loop is broken, 
otherwise it continues to the next iteration. This effectively has the same 
behavior as before. 

I experimented with a few different possible solutions, and the above was the 
only one that worked while still maintaining the current behavior as much as 
possible. From what I can tell, this was the only viable means of implementing 
the new parameter without making it entirely incompatible with "wait=True".

At this point, I believe the only remaining step for the ThreadPoolExecutor 
implementation is to update the documentation and decide on the name. After 
working with it, I'm leaning more towards *cancel_futures*, as I think this 
more accurately describes what it does compared to just *cancel* (which is a 
bit vague IMO).

--
title: Add "cancel" parameter to concurrent.futures.Executor.shutdown() -> Add 
"cancel_futures" parameter to concurrent.futures.Executor.shutdown()

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com