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

contextlib.contextmanager may incorrectly unchain RuntimeError #73878

Closed
ncoghlan opened this issue Mar 2, 2017 · 10 comments
Closed

contextlib.contextmanager may incorrectly unchain RuntimeError #73878

ncoghlan opened this issue Mar 2, 2017 · 10 comments
Assignees
Labels
3.7 (EOL) end of life type-bug An unexpected behavior, bug, or error

Comments

@ncoghlan
Copy link
Contributor

ncoghlan commented Mar 2, 2017

BPO 29692
Nosy @rhettinger, @ncoghlan, @serhiy-storchaka, @JelleZijlstra, @Mariatta, @svelankar
PRs
  • bpo-29692: contextlib.contextmanager may incorrectly unchain RuntimeE… #891
  • bpo-29692: contextlib.contextmanager may incorrectly unchain RuntimeError #949
  • bpo-29692: Add missing ACKS entry #1079
  • [3.6] bpo-29692: contextlib.contextmanager may incorrectly unchain Ru… #1105
  • [3.5] bpo-29692: contextlib.contextmanager may incorrectly unchain Ru… #1107
  • 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 = 'https://github.com/ncoghlan'
    closed_at = <Date 2017-04-13.10:17:40.288>
    created_at = <Date 2017-03-02.10:11:35.188>
    labels = ['type-bug', '3.7']
    title = 'contextlib.contextmanager may incorrectly unchain RuntimeError'
    updated_at = <Date 2017-04-13.10:17:40.286>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2017-04-13.10:17:40.286>
    actor = 'Mariatta'
    assignee = 'ncoghlan'
    closed = True
    closed_date = <Date 2017-04-13.10:17:40.288>
    closer = 'Mariatta'
    components = []
    creation = <Date 2017-03-02.10:11:35.188>
    creator = 'ncoghlan'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 29692
    keywords = ['3.5regression']
    message_count = 10.0
    messages = ['288793', '290813', '291183', '291185', '291466', '291468', '291472', '291594', '291596', '291597']
    nosy_count = 6.0
    nosy_names = ['rhettinger', 'ncoghlan', 'serhiy.storchaka', 'JelleZijlstra', 'Mariatta', 'svelankar']
    pr_nums = ['891', '949', '1079', '1105', '1107']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue29692'
    versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

    @ncoghlan
    Copy link
    Contributor Author

    ncoghlan commented Mar 2, 2017

    As part of PEP-479, an extra check was added to contextlib._GeneratorContextManager to avoid getting confused when a StopIteration exception was raised in the body of the with statement, and hence thrown into the generator body implementing the context manager.

    This extra check should only be used when the passed in exception is StopIteration, but that guard is currently missing, so it may unchain arbitrary RuntimeError exceptions if they set their __cause__ to the originally passed in value.

    Compare the current contextmanager behaviour:

    >>> from contextlib import contextmanager
    >>> @contextmanager
    ... def chain_thrown_exc():
    ...     try:
    ...         yield
    ...     except Exception as exc:
    ...         raise RuntimeError("Chained!") from exc
    ... 
    >>> with chain_thrown_exc():
    ...     1/0
    ... 
    Traceback (most recent call last):
      File "<stdin>", line 2, in <module>
    ZeroDivisionError: division by zero
    

    To the expected inline behaviour:

    >>> try:
    ...     1/0
    ... except Exception as exc:
    ...     raise RuntimeError("Chained!") from exc
    ... 
    Traceback (most recent call last):
      File "<stdin>", line 2, in <module>
    ZeroDivisionError: division by zero
    
    The above exception was the direct cause of the following exception:
    
    Traceback (most recent call last):
      File "<stdin>", line 4, in <module>
    RuntimeError: Chained!
    
    

    @ncoghlan ncoghlan added 3.7 (EOL) end of life type-bug An unexpected behavior, bug, or error labels Mar 2, 2017
    @serhiy-storchaka
    Copy link
    Member

    The __exit__() method doesn't conform PEP-8.

    PEP-8: """Be consistent in return statements. Either all return statements in a function should return an expression, or none of them should. If any return statement returns an expression, any return statements where no value is returned should explicitly state this as return None , and an explicit return statement should be present at the end of the function (if reachable)."""

    The __exit__() method has explicit "return False", bare "return", and implicit "return" at the end of the method. Together with different styles in different "except" clauses this makes it slightly hard to read.

    @rhettinger
    Copy link
    Contributor

    PEP-8's rule makes sense elsewhere, but for context managers I think an implicit return None is the norm and that making it explicit wouldn't improve readability.

    @serhiy-storchaka
    Copy link
    Member

    Then the exception for the __exit__ method should be documented in PEP-8.

    @ncoghlan
    Copy link
    Contributor Author

    New changeset 00c75e9 by Nick Coghlan (svelankar) in branch 'master':
    bpo-29692: contextlib.contextmanager may incorrectly unchain RuntimeError (GH-949)
    00c75e9

    @ncoghlan
    Copy link
    Contributor Author

    This has been merged for 3.7, but cherry-picks to the other branches are still needed.

    I also inadvertently missed adding svelankar's name (Siddharth Velankar) to Misc/ACKS, so that oversight will need to be tidied up as well.

    @ncoghlan ncoghlan removed the 3.7 (EOL) end of life label Apr 11, 2017
    @ncoghlan ncoghlan self-assigned this Apr 11, 2017
    @ncoghlan
    Copy link
    Contributor Author

    New changeset e8a6bb4 by Nick Coghlan in branch 'master':
    bpo-29692: Add missing ACKS entry (bpo-1079)
    e8a6bb4

    @Mariatta
    Copy link
    Member

    New changeset 9b409ff by Mariatta in branch '3.6':
    [3.6] bpo-29692: contextlib.contextmanager may incorrectly unchain RuntimeError (GH-949) (bpo-1105)
    9b409ff

    @Mariatta
    Copy link
    Member

    New changeset 4d015a4 by Mariatta in branch '3.5':
    [3.5] bpo-29692: contextlib.contextmanager may incorrectly unchain RuntimeError (GH-949) (bpo-1107)
    4d015a4

    @Mariatta
    Copy link
    Member

    PR has been backported into 3.5 and 3.6. Thanks all :)

    @Mariatta Mariatta added the 3.7 (EOL) end of life label Apr 13, 2017
    @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 type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants