Skip to content

Conversation

graingert
Copy link
Contributor

@graingert graingert commented Aug 11, 2022

@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

Some tests are failing.

@graingert
Copy link
Contributor Author

graingert commented Aug 11, 2022

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

@@ -239,6 +240,7 @@ async def __aexit__(self, typ, value, traceback):
isinstance(value, (StopIteration, StopAsyncIteration))
and exc.__cause__ is value
):
exc.__traceback__ = traceback
Copy link
Contributor Author

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

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

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

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

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

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

Choose a reason for hiding this comment

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

fixed it!

@iritkatriel
Copy link
Member

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

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

@graingert
Copy link
Contributor Author

@iritkatriel can you re-review this please?

@iritkatriel
Copy link
Member

Is this done?

@graingert
Copy link
Contributor Author

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

@iritkatriel
Copy link
Member

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

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.

@bedevere-bot
Copy link

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

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

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
…ted from inside a contextlib context manager (pythonGH-95883)

(cherry picked from commit b3722ca)

Co-authored-by: Thomas Grainger <[email protected]>
@bedevere-bot
Copy link

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
…om inside a contextlib context manager (GH-95883)

(cherry picked from commit b3722ca)

Co-authored-by: Thomas Grainger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Standard Library Python modules in the Lib/ directory type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants