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.

Sign up for GitHub

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.Condition prevents cancellation #77022

Closed
bharel mannequin opened this issue Feb 13, 2018 · 13 comments
Closed

Asyncio.Condition prevents cancellation #77022

bharel mannequin opened this issue Feb 13, 2018 · 13 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@bharel
Copy link
Mannequin

bharel mannequin commented Feb 13, 2018

BPO 32841
Nosy @njsmith, @asvetlov, @1st1, @bharel
PRs
  • bpo-32841: Asyncio.Condition prevents cancellation #5665
  • [3.7] bpo-32841: Fix cancellation in awaiting asyncio.Condition (GH-5665) #5682
  • [3.6] bpo-32841: Fix cancellation in awaiting asyncio.Condition (GH-5665) #5683
    		•
    Files
  • le_bug.py
  • le_patch.diff
    		•

    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-05-25.20:17:45.261>
created_at = <Date 2018-02-13.21:30:02.123>
labels = ['3.8', 'type-bug', '3.7', 'expert-asyncio']
title = 'Asyncio.Condition prevents cancellation'
updated_at = <Date 2018-05-25.20:17:45.260>
user = 'https://github.com/bharel'

    bugs.python.org fields:

    activity = <Date 2018-05-25.20:17:45.260>
actor = 'bar.harel'
assignee = 'none'
closed = True
closed_date = <Date 2018-05-25.20:17:45.261>
closer = 'bar.harel'
components = ['asyncio']
creation = <Date 2018-02-13.21:30:02.123>
creator = 'bar.harel'
dependencies = []
files = ['47439', '47440']
hgrepos = []
issue_num = 32841
keywords = ['patch']
message_count = 13.0
messages = ['312140', '312141', '312143', '312157', '312163', '312164', '312165', '312166', '312167', '312168', '312169', '317702', '317715']
nosy_count = 4.0
nosy_names = ['njs', 'asvetlov', 'yselivanov', 'bar.harel']
pr_nums = ['5665', '5682', '5683']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue32841'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']
    @bharel
    Copy link
    Mannequin Author

    bharel mannequin commented Feb 13, 2018

    Hey guys,

    A week after a serious asyncio.Lock bug, I found another bug that makes asyncio.Condition ignore and prevent cancellation in some cases due to an "except: pass" which tbh is a little embarrassing.

    What happens is that during the time it takes to get back a conditional lock after notifying it, asyncio completely ignores all cancellations sent to the waiting task.

    le_bug.py: Contains the bug
    le_patch.diff: Contains a very simple fix (will send a pull on Github too)
    le_meme.jpg: Contains my face after debugging this for 4 hours

    Yuri, I hope you didn't miss me during this week ;-)

    -- Bar

    @bharel bharel mannequin added 3.7 (EOL) end of life 3.8 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error labels Feb 13, 2018
    @1st1
    Copy link
    Member

    1st1 commented Feb 13, 2018

    le_meme.jpg is a pretty common facial expression when one debugs locks/conditions, so let's not attach it in the future :) We try to keep the bug tracker free of unnecessary information.

    As for the bug—please submit a PR!

    @njsmith
    Copy link
    Contributor

    njsmith commented Feb 13, 2018

    Interesting case! This is super subtle. I think the patch is correct though.

    (In Trio this is one of the cases where we use shielding: https://github.com/python-trio/trio/blob/f48d8922dfe2118bfaaf9a85f2524e58146ba369/trio/_sync.py#L749-L752 . But asyncio.shield has different semantics -- it still delivers the CancelledError to the caller, which is no good in this case.)

    @njsmith
    Copy link
    Contributor

    njsmith commented Feb 14, 2018

    It does make me wonder if asyncio.shield *should* wait for the thing it's shielding though, so that it *would* work in this case? (Similar to bpo-32751.)

    @bharel
    Copy link
    Mannequin Author

    bharel mannequin commented Feb 14, 2018

    I don't think so. Having shield not cancel immediately but rather wait and
    cancel will cause long timed shielded operations to stall the task
    cancellation, usually for no good. This isn't the general case.
    However, adding another function which does so might just be a good idea. I
    think another parameter to shield to choose cancellation time will clutter
    the function call.

    On Wed, Feb 14, 2018, 7:28 AM Nathaniel Smith <report@bugs.python.org>
    wrote:

    Nathaniel Smith <njs@pobox.com> added the comment:

    It does make me wonder if asyncio.shield *should* wait for the thing it's
    shielding though, so that it *would* work in this case? (Similar to
    bpo-32751.)

    ----------

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

    @asvetlov
    Copy link
    Contributor

    New changeset 5746510 by Andrew Svetlov (Bar Harel) in branch 'master':
    bpo-32841: Fix cancellation in awaiting asyncio.Condition (bpo-5665)
    5746510

    @miss-islington
    Copy link
    Contributor

    New changeset 8caee0f by Miss Islington (bot) in branch '3.7':
    bpo-32841: Fix cancellation in awaiting asyncio.Condition (GH-5665)
    8caee0f

    @asvetlov
    Copy link
    Contributor

    New changeset a23eeca by Andrew Svetlov (Miss Islington (bot)) in branch '3.6':
    bpo-32841: Fix cancellation in awaiting asyncio.Condition (GH-5665) (GH-5683)
    a23eeca

    @asvetlov
    Copy link
    Contributor

    Thanks Bar Harel

    @njsmith
    Copy link
    Contributor

    njsmith commented Feb 14, 2018

    Having shield not cancel immediately but rather wait and cancel will cause long timed shielded operations to stall the task cancellation, usually for no good. This isn't the general case.

    What I'm suggesting is that maybe it actually is good in the general case :-). If an operation can't be cancelled, that's too bad, but it's generally still better to wait then to silently leak it into the background.

    @njsmith njsmith reopened this Feb 14, 2018
    @asvetlov
    Copy link
    Contributor

    Well, makes sense

    @1st1
    Copy link
    Member

    1st1 commented May 25, 2018

    Should this issue be closed now?

    @bharel
    Copy link
    Mannequin Author

    bharel mannequin commented May 25, 2018

    Yup. Thanks Yuri :-)

    @bharel bharel mannequin closed this as completed May 25, 2018
    @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.7 (EOL) end of life 3.8 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

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