Navigation Menu

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

asyncio: We should prohibit setting a ProcessPoolExecutor in with set_default_executor #78256

Closed
1st1 opened this issue Jul 9, 2018 · 13 comments
Labels
3.8 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@1st1
Copy link
Member

1st1 commented Jul 9, 2018

BPO 34075
Nosy @gvanrossum, @vstinner, @asvetlov, @1st1, @basnijholt
PRs
  • bpo-34075: Deprecate non-ThreadPoolExecutor in loop.set_default_executor() #8533
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2018-07-30.10:43:39.503>
    created_at = <Date 2018-07-09.15:29:34.966>
    labels = ['type-bug', '3.8', 'expert-asyncio']
    title = 'asyncio: We should prohibit setting a ProcessPoolExecutor in with set_default_executor'
    updated_at = <Date 2019-03-26.11:40:40.218>
    user = 'https://github.com/1st1'

    bugs.python.org fields:

    activity = <Date 2019-03-26.11:40:40.218>
    actor = 'basnijholt'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-07-30.10:43:39.503>
    closer = 'vstinner'
    components = ['asyncio']
    creation = <Date 2018-07-09.15:29:34.966>
    creator = 'yselivanov'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 34075
    keywords = ['patch']
    message_count = 13.0
    messages = ['321324', '321330', '321331', '321338', '321339', '321459', '321469', '321476', '321479', '321620', '322665', '322666', '338872']
    nosy_count = 5.0
    nosy_names = ['gvanrossum', 'vstinner', 'asvetlov', 'yselivanov', 'basnijholt']
    pr_nums = ['8533']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue34075'
    versions = ['Python 3.8']

    @1st1
    Copy link
    Member Author

    1st1 commented Jul 9, 2018

    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

    @1st1 1st1 added 3.8 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error labels Jul 9, 2018
    @gvanrossum
    Copy link
    Member

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Jul 9, 2018

    Great! Thanks for the quick reply, Guido.

    @1st1
    Copy link
    Member Author

    1st1 commented Jul 9, 2018

    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.

    @gvanrossum
    Copy link
    Member

    Of course, that's what I meant.

    @vstinner vstinner changed the title We should prohibit setting a ProcessPoolExecutor in with set_default_executor asyncio: We should prohibit setting a ProcessPoolExecutor in with set_default_executor Jul 11, 2018
    @vstinner
    Copy link
    Member

    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.

    @gvanrossum
    Copy link
    Member

    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)

    @vstinner
    Copy link
    Member

    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 :-)

    @1st1
    Copy link
    Member Author

    1st1 commented Jul 11, 2018

    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.

    @asvetlov
    Copy link
    Contributor

    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.

    @vstinner
    Copy link
    Member

    New changeset 22d2508 by Victor Stinner (Elvis Pranskevichus) in branch 'master':
    bpo-34075: Deprecate non-ThreadPoolExecutor in loop.set_default_executor() (GH-8533)
    22d2508

    @vstinner
    Copy link
    Member

    Elvis Pranskevichus: well done, your first PR was already good to be merged ;-)

    @basnijholt
    Copy link
    Mannequin

    basnijholt mannequin commented Mar 26, 2019

    I think this issue is related to the problem in https://bugs.python.org/issue36281

    If it indeed is the case, then the fix proposed here and implemented in 22d2508 won't really help.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants