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.wait_for can cancel futures faster with timeout==0 #75737

Closed
hellysmile mannequin opened this issue Sep 22, 2017 · 8 comments
Closed

asyncio.wait_for can cancel futures faster with timeout==0 #75737

hellysmile mannequin opened this issue Sep 22, 2017 · 8 comments
Labels
3.7 (EOL) end of life topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@hellysmile
Copy link
Mannequin

hellysmile mannequin commented Sep 22, 2017

BPO 31556
Nosy @asvetlov, @1st1, @hellysmile
PRs
  • bpo-31556: asyncio.wait_for can cancel futures faster with timeout <= 0 #3703
  • 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 2017-10-05.16:05:32.055>
    created_at = <Date 2017-09-22.20:47:57.541>
    labels = ['type-bug', '3.7', 'expert-asyncio']
    title = 'asyncio.wait_for can cancel futures faster with timeout==0'
    updated_at = <Date 2017-10-05.16:59:54.129>
    user = 'https://github.com/hellysmile'

    bugs.python.org fields:

    activity = <Date 2017-10-05.16:59:54.129>
    actor = 'yselivanov'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-10-05.16:05:32.055>
    closer = 'yselivanov'
    components = ['asyncio']
    creation = <Date 2017-09-22.20:47:57.541>
    creator = 'hellysmile'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31556
    keywords = ['patch']
    message_count = 8.0
    messages = ['302770', '302771', '302772', '302773', '302774', '302775', '302779', '303777']
    nosy_count = 3.0
    nosy_names = ['asvetlov', 'yselivanov', 'hellysmile']
    pr_nums = ['3703']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue31556'
    versions = ['Python 3.6', 'Python 3.7']

    @hellysmile
    Copy link
    Mannequin Author

    hellysmile mannequin commented Sep 22, 2017

    There is no need to create extra future and use loop.call_later to cancel base future from asyncio.wait_for when timeout==0. If loop is heavy loaded it can be cancelled simnifically later then 0 seconds later.

    @hellysmile hellysmile mannequin added 3.7 (EOL) end of life topic-asyncio labels Sep 22, 2017
    @1st1
    Copy link
    Member

    1st1 commented Sep 22, 2017

    Do you have a use case where this optimization is important?

    @hellysmile
    Copy link
    Mannequin Author

    hellysmile mannequin commented Sep 22, 2017

    If coroutine function has some blocking calls before first await/yield from statement maybe makes sense do no let them be executed, if timeout equals 0

    @1st1
    Copy link
    Member

    1st1 commented Sep 22, 2017

    I think this is a backwards incompatible change and thus will be rejected. Currently there's a guarantee that "wait_for" can throw a TimeoutError *only* when when you await it.

       fut = wait_for(something, 0)

    # some important code

    try:
    await fut
    except TimeoutError:
    # do something

    With your PR merged, the above asyncio code would be broken, because asyncio users can guard with try..except only the await expression.

    @hellysmile
    Copy link
    Mannequin Author

    hellysmile mannequin commented Sep 22, 2017

    asyncio.wait_for is coroutine itself, to start executing code, no matter with this PR or not it needs to be awaited/yield from

    import asyncio
    
    @asyncio.coroutine
    def foo():
        print(1)
    
    loop = asyncio.get_event_loop()
    
    fut = asyncio.wait_for(foo(), 0)
    
    print('it is not raised yet')

    try:
    loop.run_until_complete(fut)
    except asyncio.TimeoutError:
    print('raised here')

    will print
    it is not raised yet
    raised here

    @1st1
    Copy link
    Member

    1st1 commented Sep 22, 2017

    You're right! Let's work on the PR then, I've left a review comment.

    @hellysmile
    Copy link
    Mannequin Author

    hellysmile mannequin commented Sep 22, 2017

    Actually provided example without patch will print 1, which is not expected

    @1st1 1st1 closed this as completed Oct 5, 2017
    @1st1 1st1 added the type-bug An unexpected behavior, bug, or error label Oct 5, 2017
    @1st1
    Copy link
    Member

    1st1 commented Oct 5, 2017

    Note, that this will not be backported to 3.6, as it behaves in a slightly incompatible way. I consider this patch as an enhancement that makes 'asyncio.wait_for' semantics easier to reason about and more practical.

    @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
    Labels
    3.7 (EOL) end of life topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant