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

Jump to bottom

asyncio.wait function creates futures set two times #86306

Closed
dutradda mannequin opened this issue Oct 24, 2020 · 13 comments
Closed

asyncio.wait function creates futures set two times #86306

dutradda mannequin opened this issue Oct 24, 2020 · 13 comments
Labels
3.9 only security fixes 3.10 only security fixes topic-asyncio type-feature A feature request or enhancement

Comments

@dutradda
Copy link
Mannequin

dutradda mannequin commented Oct 24, 2020

BPO 42140
Nosy @ods, @asvetlov, @cjerdonek, @1st1, @jstasiak, @miss-islington, @JustinTArthur, @dutradda
PRs
  • bpo-42140: Improve asyncio.wait function #22938
  • [3.9] bpo-42140: Improve asyncio.wait function (GH-22938) #23233
    		•

    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 2020-11-11.13:28:36.573>
created_at = <Date 2020-10-24.06:51:22.455>
labels = ['type-feature', '3.9', '3.10', 'expert-asyncio']
title = 'asyncio.wait function creates futures set two times'
updated_at = <Date 2020-11-11.13:28:36.572>
user = 'https://github.com/dutradda'

    bugs.python.org fields:

    activity = <Date 2020-11-11.13:28:36.572>
actor = 'asvetlov'
assignee = 'none'
closed = True
closed_date = <Date 2020-11-11.13:28:36.573>
closer = 'asvetlov'
components = ['asyncio']
creation = <Date 2020-10-24.06:51:22.455>
creator = 'dutradda'
dependencies = []
files = []
hgrepos = []
issue_num = 42140
keywords = ['patch']
message_count = 13.0
messages = ['379522', '379613', '379638', '379685', '379689', '379714', '379725', '379734', '379736', '380129', '380721', '380726', '380727']
nosy_count = 8.0
nosy_names = ['ods', 'asvetlov', 'chris.jerdonek', 'yselivanov', 'jstasiak', 'miss-islington', 'JustinTArthur', 'dutradda']
pr_nums = ['22938', '23233']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue42140'
versions = ['Python 3.9', 'Python 3.10']
    @dutradda
    Copy link
    Mannequin Author

    dutradda mannequin commented Oct 24, 2020

    The python3.9 code creates the futures set two times.
    We can create this set before, avoiding the second creation.

    This python3.9 behaviour breaks the aiokafka library, because it gives an iterator to that function, so the second iteration become empty.

    @dutradda dutradda mannequin added 3.9 only security fixes topic-asyncio type-feature A feature request or enhancement labels Oct 24, 2020
    @cjerdonek
    Copy link
    Member

    Are you suggesting this is a bug, or is it just a suggested code cleanup? I ask because the docs suggest that a set should be passed:
    https://docs.python.org/3/library/asyncio-task.html#asyncio.wait

    And the docstring says it should be a sequence:

    cpython/Lib/asyncio/tasks.py

    Lines 373 to 376 in d1a0a96

    async def wait(fs, *, loop=None, timeout=None, return_when=ALL_COMPLETED):
    """Wait for the Futures and coroutines given by fs to complete.
    The sequence futures must not be empty.

    So while code cleanup is okay, I'm not sure support for general iterator arguments can / should be guaranteed.

    @ods
    Copy link
    Mannequin

    ods mannequin commented Oct 26, 2020

    The current error message is way too cryptic anyway. And restricting it to the set type only, as in docs, will certainly break a lot of code (passing a list is quite common).

    @dutradda
    Copy link
    Mannequin Author

    dutradda mannequin commented Oct 26, 2020

    Are you suggesting this is a bug, or is it just a suggested code cleanup?

    It is a suggested code cleanup.

    My point is that the code creates two sets based on the Sequence fs.
    I think it is better if the code creates the set just one time, instead of
    two times.

    Em dom., 25 de out. de 2020 às 18:57, Chris Jerdonek <report@bugs.python.org>
    escreveu:

    Chris Jerdonek <chris.jerdonek@gmail.com> added the comment:

    Are you suggesting this is a bug, or is it just a suggested code cleanup?
    I ask because the docs suggest that a set should be passed:
    https://docs.python.org/3/library/asyncio-task.html#asyncio.wait

    And the docstring says it should be a sequence:

    cpython/Lib/asyncio/tasks.py

    Lines 373 to 376 in d1a0a96

    async def wait(fs, *, loop=None, timeout=None, return_when=ALL_COMPLETED):
    """Wait for the Futures and coroutines given by fs to complete.
    The sequence futures must not be empty.

    So while code cleanup is okay, I'm not sure support for general iterator
    arguments can / should be guaranteed.

    ----------
    nosy: +chris.jerdonek

    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue42140\>

    @JustinTArthur
    Copy link
    Mannequin

    JustinTArthur mannequin commented Oct 26, 2020

    Your change makes perfect sense to me. It would be a backport-only change as the 2nd set creation is actually getting removed for the development version (3.10) to finalize the deprecation of wait's coroutine scheduling.

    @cjerdonek
    Copy link
    Member

    If it's just a code cleanup and not a bugfix though, it shouldn't be backported. But yes, as a cleanup it's fine. And even if it's expected to go away later, it's okay to do now.

    @JustinTArthur
    Copy link
    Mannequin

    JustinTArthur mannequin commented Oct 27, 2020

    I believe the documentation may be referring to the English set and not a Python set, but I could be wrong.

    Yury changed the wording from sequence to set in 3.7, but we didn't document a breaking change as far as I know. The purpose of those set constructions was to handle the many things other than a set that can be passed in that may have non-unique values. Early asyncio documentation included examples of passing wait() a list.

    I wouldn't be surprised if there are other libraries or apps out there for which removing iterator support was an accidentally-breaking change and it may be strange if this is only is broken in 3.9.x, but working both before and after.

    @dutradda
    Copy link
    Mannequin Author

    dutradda mannequin commented Oct 27, 2020

    I wouldn't be surprised if there are other libraries or apps out there
    for which removing iterator support was an accidentally-breaking change and
    it may be strange if this is only is broken in 3.9.x, but working both
    before and after.

    Justin, when the deprecation message has been removed, the iterator will
    works again, because the code will iterate just one time.
    So the behaviour you describe will happen anyway.

    My changes proposal is a “improvement”, because the second iteration can be
    made on the already created set.

    I think that change make senses because it removes unnecessary data
    creation.

    Em ter, 27 de out de 2020 às 00:24, Justin Arthur <report@bugs.python.org>
    escreveu:

    Justin Arthur <justinarthur@gmail.com> added the comment:

    I believe the documentation may be referring to the English set and not a
    Python set, but I could be wrong.

    Yury changed the wording from sequence to set in 3.7, but we didn't
    document a breaking change as far as I know. The purpose of those set
    constructions was to handle the many things other than a set that can be
    passed in that may have non-unique values. Early asyncio documentation
    included examples of passing wait() a list.

    I wouldn't be surprised if there are other libraries or apps out there for
    which removing iterator support was an accidentally-breaking change and it
    may be strange if this is only is broken in 3.9.x, but working both before
    and after.

    ----------

    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue42140\>

    @JustinTArthur
    Copy link
    Mannequin

    JustinTArthur mannequin commented Oct 27, 2020

    So the behaviour you describe will happen anyway.
    Unless we make your change against the 3.9 branch, in which case the fix will make it into the 3.9.x series of patch releases.

    @jstasiak
    Copy link
    Mannequin

    jstasiak mannequin commented Nov 1, 2020

    I opened https://bugs.python.org/issue42230 to make the documentation reflect the reality in this matter.

    @miss-islington
    Copy link
    Contributor

    New changeset 7e5ef0a by Diogo Dutra in branch 'master':
    bpo-42140: Improve asyncio.wait function (GH-22938)
    7e5ef0a

    @miss-islington
    Copy link
    Contributor

    New changeset 33922cb by Miss Islington (bot) in branch '3.9':
    bpo-42140: Improve asyncio.wait function (GH-22938)
    33922cb

    @1st1
    Copy link
    Member

    1st1 commented Nov 10, 2020

    Thanks for the change and for the discussion to everybody involved. The PR makes sense and I've already merged it.

    @asvetlov asvetlov added the 3.10 only security fixes label Nov 11, 2020
    @asvetlov asvetlov added the 3.10 only security fixes label Nov 11, 2020
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.9 only security fixes 3.10 only security fixes topic-asyncio type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @1st1 @cjerdonek @asvetlov @miss-islington