Skip to content

gh-95882: fix regression in the traceback of exceptions propagated from inside a contextlib context manager#95883

Merged
iritkatriel merged 15 commits intopython:mainfrom
graingert:resolve-differences-between-asynccontextmanager-and-contextmanager
Jan 3, 2023
Merged

gh-95882: fix regression in the traceback of exceptions propagated from inside a contextlib context manager#95883
iritkatriel merged 15 commits intopython:mainfrom
graingert:resolve-differences-between-asynccontextmanager-and-contextmanager

Conversation

@graingert
Copy link
Contributor

@graingert graingert commented Aug 11, 2022 •
edited
Loading

@graingert graingert marked this pull request as ready for review August 11, 2022 10:02
@graingert graingert requested a review from 1st1 as a code owner August 11, 2022 10:02
@graingert graingert requested a review from iritkatriel August 11, 2022 10:02
@iritkatriel
Copy link
Member

iritkatriel commented Aug 11, 2022

Some tests are failing.

@graingert
Copy link
Contributor Author

graingert commented Aug 11, 2022 •
edited
Loading

Oh dear, missing import. I probably need to port more tests over too I also have this as a drive by fix https://github.com/python/cpython/pull/95888/files to save a bit of __context__ mutation and import time

graingert
graingert commented Aug 11, 2022
View reviewed changes
Lib/contextlib.py Outdated
isinstance(value, (StopIteration, StopAsyncIteration))
and exc.__cause__ is value
):
exc.__traceback__ = traceback
Copy link
Contributor Author

@graingert graingert Aug 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs a test for this case

Copy link
Contributor Author

@graingert graingert Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iritkatriel it doesn't look like this line does anything. If I delete it I get the same behavior, same with L176:

cpython/Lib/contextlib.py

Lines 166 to 177 in adf2bf3

# Avoid suppressing if a StopIteration exception
# was passed to throw() and later wrapped into a RuntimeError
# (see PEP 479 for sync generators; async generators also
# have this behavior). But do this only if the exception wrapped
# by the RuntimeError is actually Stop(Async)Iteration (see
# issue29692).
if (
isinstance(value, StopIteration)
and exc.__cause__ is value
):
exc.__traceback__ = traceback
return False

graingert
graingert commented Aug 11, 2022
View reviewed changes
Lib/test/test_contextlib.py Outdated
Comment on lines +138 to +139
self.assertEqual(frames[0].name, 'f')
self.assertEqual(frames[0].line, 'yield')
Copy link
Contributor Author

@graingert graingert Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iritkatriel do you expect to see this yield here?

Copy link
Member

@iritkatriel iritkatriel Aug 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does 3.10 do?

Copy link
Contributor Author

@graingert graingert Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the yield is new, incorrect, behavior in 3.11:

import contextlib
import logging

logger = logging.getLogger("__name__")

@contextlib.contextmanager
def f():
    yield


try:
    with f():
        raise StopIteration
except StopIteration:
    logger.exception("stop iteration")



print("================ overwriting StopIteration via globals!==============")


class StopIteration(Exception):
    pass


try:
    with f():
        raise StopIteration
except StopIteration:
    logger.exception("stop iteration")
 graingert@conscientious  testing311  ~/projects/trio   master ± python3.10 demo.py
stop iteration
Traceback (most recent call last):
  File "/home/graingert/projects/trio/demo.py", line 13, in <module>
    raise StopIteration
StopIteration
================ overwriting StopIteration via globals!==============
stop iteration
Traceback (most recent call last):
  File "/home/graingert/projects/trio/demo.py", line 28, in <module>
    raise StopIteration
StopIteration

Notice the extra yield in the first run before overwriting StopIteration via globals happens:

 graingert@conscientious  testing311  ~/projects/trio   master ± python3.11 demo.py 
stop iteration
Traceback (most recent call last):
  File "/home/graingert/projects/trio/demo.py", line 8, in f
    yield
  File "/home/graingert/projects/trio/demo.py", line 13, in <module>
    raise StopIteration
StopIteration
================ overwriting StopIteration via globals!==============
stop iteration
Traceback (most recent call last):
  File "/home/graingert/projects/trio/demo.py", line 28, in <module>
    raise StopIteration
StopIteration

Copy link
Contributor Author

@graingert graingert Aug 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed it!

graingert
graingert commented Aug 11, 2022
View reviewed changes
@iritkatriel
Copy link
Member

iritkatriel commented Aug 11, 2022

Could you make a PR against 3.10 to backport only the new tests, so we can make sure this is not changing anything?

@graingert
Copy link
Contributor Author

graingert commented Aug 11, 2022

Could you make a PR against 3.10 to backport only the new tests, so we can make sure this is not changing anything?

Yeah, I'll see how far I can backport it in contextlib2 too

@bedevere-bot bedevere-bot mentioned this pull request Dec 20, 2022
Closed
@graingert
Copy link
Contributor Author

graingert commented Dec 20, 2022

@iritkatriel can you re-review this please?

@iritkatriel
Copy link
Member

iritkatriel commented Dec 20, 2022

Is this done?

@graingert
Copy link
Contributor Author

graingert commented Dec 20, 2022

Once it's in a release I'll apply it to the backport

@iritkatriel
Copy link
Member

iritkatriel commented Dec 20, 2022

Once it's in a release I'll apply it to the backport

It has to be a separate PR anyway because it will only contain the tests. Why not do it as part of the review process for this PR?

@iritkatriel
Copy link
Member

iritkatriel commented Dec 22, 2022

This looks fine but I think we should see the tests pass on all 3.10 buildbots before merging this. It’s the same amount of work, we might as well do it in the right order.

@iritkatriel iritkatriel mentioned this pull request Jan 3, 2023
Merged
@bedevere-bot
Copy link

bedevere-bot commented Jan 3, 2023

GH-100715 is a backport of this pull request to the 3.10 branch.

@iritkatriel iritkatriel added type-bug An unexpected behavior, bug, or error stdlib Standard Library Python modules in the Lib/ directory needs backport to 3.11 only security fixes labels Jan 3, 2023
@iritkatriel iritkatriel changed the title gh-95882: fix traceback of exceptions propagated from inside a context… gh-95882: fix regression in the traceback of exceptions propagated from inside a contextlib context manager Jan 3, 2023
@iritkatriel iritkatriel merged commit b3722ca into python:main Jan 3, 2023
@miss-islington
Copy link
Contributor

miss-islington commented Jan 3, 2023

Thanks @graingert for the PR, and @iritkatriel for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

bedevere-bot commented Jan 3, 2023

GH-100718 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jan 3, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 3, 2023
@graingert @miss-islington
pythongh-95882: fix regression in the traceback of exceptions propaga…
ee672ce 
…ted from inside a contextlib context manager (pythonGH-95883)

(cherry picked from commit b3722ca)

Co-authored-by: Thomas Grainger <tagrain@gmail.com>
@bedevere-bot
Copy link

bedevere-bot commented Jan 3, 2023

GH-100715 is a backport of this pull request to the 3.10 branch.

miss-islington added a commit that referenced this pull request Jan 3, 2023
@miss-islington @graingert
gh-95882: fix regression in the traceback of exceptions propagated fr…
861cdef 
…om inside a contextlib context manager (GH-95883)

(cherry picked from commit b3722ca)

Co-authored-by: Thomas Grainger <tagrain@gmail.com>
iritkatriel added a commit that referenced this pull request Jan 3, 2023
@iritkatriel
[3.10] gh-95882: Add tests for traceback from contextlib context mana…
ff9ac58 
…gers (GH-95883) (#100715)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel approved these changes

@1st1 1st1 Awaiting requested review from 1st1

Assignees

No one assigned

Labels

stdlib Standard Library Python modules in the Lib/ directory type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@graingert @iritkatriel @bedevere-bot @miss-islington