classification
Title: asyncio: We should prohibit setting a ProcessPoolExecutor in with set_default_executor
Type: behavior Stage: resolved
Components: asyncio Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: asvetlov, gvanrossum, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2018-07-09 15:29 by yselivanov, last changed 2018-07-30 10:43 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 8533 merged Elvis.Pranskevichus, 2018-07-28 16:13
Messages (12)
msg321324 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-07-09 15:29
I've had a few conversations with people who were confused that asyncio starts to behave weirdly when a ProcessPoolExecutor is set as the default one.  We don't really test that asyncio's built-in functionality (like DNS resolving) works well with a process-pool, which leads to bug reports like [1].  Third-party libraries also always assume that the loop is always configured to use the ThreadPoolExecutor (as it is by default), and also don't even test against ProcessPool.

My idea here would be to deprecate setting ProcessPoolExecutor as a default one in 3.8 and prohibit that in 3.9.

Guido, Andrew, what do you think?

[1] https://bugs.python.org/issue34073
msg321330 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-07-09 16:12
Yeah, that's a good idea. It was never meant for a ProcessPoolExecutor.

We should warn against this in the docs right away (and backport the warning to all previous versions that have set_executor).

I also support non-silent deprecation in 3.8.
msg321331 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-07-09 16:16
Great! Thanks for the quick reply, Guido.
msg321338 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-07-09 18:23
> We should warn against this in the docs right away (and backport the warning to all previous versions that have set_executor).

I think we'll only allow instances of c.f.ThreadPoolExecutor (and its subclasses) to be passed to set_default_executor.  That's a more robust check than just guarding against ProcessPoolExecutor.
msg321339 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-07-09 18:24
Of course, that's what I meant.
msg321459 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-11 14:34
> I think we'll only allow instances of c.f.ThreadPoolExecutor (and its subclasses) to be passed to set_default_executor.  That's a more robust check than just guarding against ProcessPoolExecutor.

I suggest to only reject ProcessPoolExecutor, instead of having a very specific test on ThreadPoolExecutor which might fail with funky but valid thread executor.
msg321469 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-07-11 14:53
I disagree. Other than subclasses of thread executor, what are you going to
pass in? A mock?

On Wed, Jul 11, 2018 at 7:34 AM STINNER Victor <report@bugs.python.org>
wrote:

>
> STINNER Victor <vstinner@redhat.com> added the comment:
>
> > I think we'll only allow instances of c.f.ThreadPoolExecutor (and its
> subclasses) to be passed to set_default_executor.  That's a more robust
> check than just guarding against ProcessPoolExecutor.
>
> I suggest to only reject ProcessPoolExecutor, instead of having a very
> specific test on ThreadPoolExecutor which might fail with funky but valid
> thread executor.
>
> ----------
> nosy: +vstinner
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue34075>
> _______________________________________
>
-- 
--Guido (mobile)
msg321476 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-11 15:08
> Other than subclasses of thread executor, what are you going to pass in?

I don't see why asyncio should prevent people to experiment their own custom executor. You can imagine a custom "green executor" which inherit from Executor but uses its own black magic like greenlet. People like to do crazy things to implement asynchronous programming :-)
msg321479 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-07-11 15:14
> I don't see why asyncio should prevent people to experiment their own custom executor. You can imagine a custom "green executor" which inherit from Executor but uses its own black magic like greenlet.

Because asyncio and its ecosystem is built around the fact that the default executor is a ThreadPoolExecutor.

asyncio users are free to:

1. Pass any kind of executor as a first argument to `loop.run_in_executor()` -- no restrictions there.

2. Pass a subclass of ThreadPoolExecutor to `loop.set_default_executor()` -- that's how people can experiment.

Mind that (2) also covers exotic use cases like using greenlets under the hood (although I don't think it's possible), as long as they subclass ThreadPoolExecutor.

My plan so far: 

* in 3.8 add a DeprecationWarning if an executor other than ThreadPoolExecutor is set as a default one.

* during 3.8 see what kind of feedback we get.

* if all goes great, in 3.9 we'll only allow subclasses of ThreadPoolExecutor.
msg321620 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2018-07-13 15:29
Agree, restricting to ThreadPoolExecutor sounds reasonable.

A custom executor may have the same problem as ProcessPoolExecutor.

Moreover it can work under same scenarios but crash with other third-party libs.
msg322665 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-30 10:42
New changeset 22d25085db2590932b3664ca32ab82c08f2eb2db by Victor Stinner (Elvis Pranskevichus) in branch 'master':
bpo-34075: Deprecate non-ThreadPoolExecutor in loop.set_default_executor() (GH-8533)
https://github.com/python/cpython/commit/22d25085db2590932b3664ca32ab82c08f2eb2db
msg322666 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-30 10:43
Elvis Pranskevichus: well done, your first PR was already good to be merged ;-)
History
Date User Action Args
2018-07-30 10:43:39vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg322666

stage: patch review -> resolved
2018-07-30 10:42:48vstinnersetmessages: + msg322665
2018-07-28 16:13:03Elvis.Pranskevichussetkeywords: + patch
stage: patch review
pull_requests: + pull_request8050
2018-07-13 15:29:21asvetlovsetmessages: + msg321620
2018-07-11 15:14:35yselivanovsetmessages: + msg321479
2018-07-11 15:08:56vstinnersetmessages: + msg321476
2018-07-11 14:53:02gvanrossumsetmessages: + msg321469
2018-07-11 14:34:44vstinnersetnosy: + vstinner
messages: + msg321459
2018-07-11 14:33:26vstinnersettitle: We should prohibit setting a ProcessPoolExecutor in with set_default_executor -> asyncio: We should prohibit setting a ProcessPoolExecutor in with set_default_executor
2018-07-09 18:24:55gvanrossumsetmessages: + msg321339
2018-07-09 18:23:53yselivanovsetmessages: + msg321338
2018-07-09 16:16:08yselivanovsetmessages: + msg321331
2018-07-09 16:12:47gvanrossumsetmessages: + msg321330
2018-07-09 15:29:34yselivanovcreate