Trackstack includes __exit__ in contextlib which is not in previous versions

[Cheukting]

Bug report

See HypothesisWorks/hypothesis#3298 trackstack includes __exit__ in contextlib which is not the case in previous python versions. Wonder is this intentional and will it be cleaned up when released?

Your environment

• CPython versions tested on: 3.11.0a6
• Operating system and architecture: MacOS 12.1

Also looping @Zac-HD in

[hauntsaninja]

In case it helps, here's a minimal example of what this looks like:

λ cat test.py
import contextlib

@contextlib.contextmanager
def f():
    try:
        yield
    finally:
        pass

with f(): 1/0

λ python3.10 test.py
Traceback (most recent call last):
  File "/Users/shantanu/dev/test.py", line 10, in <module>
    with f(): 1/0
ZeroDivisionError: division by zero

λ python3.11 test.py
Traceback (most recent call last):
  File "/Users/shantanu/.pyenv/versions/3.11-dev/lib/python3.11/contextlib.py", line 155, in __exit__
    self.gen.throw(typ, value, traceback)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/shantanu/dev/test.py", line 6, in f
    yield
    ^^^^^
  File "/Users/shantanu/dev/test.py", line 10, in <module>
    with f(): 1/0
              ~^~
ZeroDivisionError: division by zero

[hauntsaninja]

cc @brandtbucher (since you asked me to tag you)

[iritkatriel]

I'm looking, this was caused by:

commit 396b583
Author: Irit Katriel [email protected]
Date: Fri Dec 17 14:46:22 2021 +0000

bpo-45711: Remove type and traceback from exc_info (GH-30122)

[Cheukting]

Thanks @iritkatriel

[iritkatriel]

I get the old traceback with this change:

diff --git a/Lib/contextlib.py b/Lib/contextlib.py
index 4cff9c6..5ef8124 100644
--- a/Lib/contextlib.py
+++ b/Lib/contextlib.py
@@ -183,6 +183,7 @@ def __exit__(self, typ, value, traceback):
                 # and the __exit__() protocol.
                 if exc is not value:
                     raise
+                exc.__traceback__ = traceback
                 return False
             raise RuntimeError("generator didn't stop after throw()")

The question now is which beaviour we prefer. I'll make a PR so that we can discuss.

cc @gvanrossum @markshannon

[gvanrossum]

I think I prefer the 3.10 behavior, so just send the PR.

[iritkatriel]

PR is here: #92202

[serhiy-storchaka]

Wow, you are an expert of fixing tracebacks @iritkatriel!

Should this be fixed in contextlib.contextmanager or in the core code which calls __exit__ and reraises the exception? Does it affect other context managers (for example TestCase.assertRaises and friends)?

[iritkatriel]

Thanks ☺️

Should this be fixed in contextlib.contextmanager or in the core code which calls __exit__ and reraises the exception? Does it affect other context managers (for example TestCase.assertRaises and friends)?

I don’t think it can be fixed in the core code. __exit__ was called with the right traceback, but then new frames were added to the traceback inside __exit__ when the exception was raised from the generator.

We used to propagate the traceback separately from the exception in the (exc, val, tb) triplet. Now we don’t, we always take the traceback from the exception instance. So modifications to the exception are visible. I don’t think all context managers do this, but we might run into a few more cases.

[serhiy-storchaka]

I mean that setting exc.__traceback__ = traceback after calling __exit__(extype, exc, traceback) would guarantee restoring the former behavior, but perhaps it is better to fix on case by case basis.

What was the behavior of ExitStack in 3.10 in case the traceback was changed in __exit__ of a nested context manager? Is it the same in 3.11?

[iritkatriel]

I mean that setting exc.__traceback__ = traceback after calling __exit__(extype, exc, traceback) would guarantee restoring the former behavior, but perhaps it is better to fix on case by case basis.

Ah yes, that would fix this case. But it would also prevent you from changing the traceback in the exit block if you want to.

What was the behavior of ExitStack in 3.10 in case the traceback was changed in __exit__ of a nested context manager? Is it the same in 3.11?

Good question. I don't see any tests for traceback with ExitStack (there are some tests for the context links). We should add some.

I'm heading back to London soon. @serhiy-storchaka - feel free to merge the PR if you think its ready (we can work on ExitStack separately). Or if you have further comments I'll follow up tomorrow/Thursday.

[iritkatriel]

I made a PR to add a test for ExitStack. This test passes on 3.10 as well, so there was no change there: #92339

I think it's unfortunate that the traceback contains so many frames from contextlib (from the dance it does to set __context__). But that's separate from the current issue.

[iritkatriel]

Thank you for reporting this @Cheukting .

[graingert]

@iritkatriel Doesn't this need to be the same in both asynccontextmanager and AsyncExitStack?

indeed it do:

import contextlib


@contextlib.asynccontextmanager
async def f():
    try:
        yield
    finally:
        pass

async def amain():
    async with f(): 1/0


with contextlib.closing(amain().__await__()) as gen:
    next(gen, None)

Traceback (most recent call last):
  File "/home/graingert/projects/cpython/demo.py", line 16, in <module>
    next(gen, None)
  File "/usr/lib/python3.11/contextlib.py", line 222, in __aexit__
    await self.gen.athrow(typ, value, traceback)
  File "/home/graingert/projects/cpython/demo.py", line 7, in f
    yield
  File "/home/graingert/projects/cpython/demo.py", line 12, in amain
    async with f(): 1/0
                    ~^~
ZeroDivisionError: division by zero

[fishy]

This breaks thrift generated python code on Python 3.11.

Thrift made sure that all generated python exceptions are immutable because Python3 actually expects all exceptions to be hashable, see https://issues.apache.org/jira/browse/THRIFT-4002 and apache/thrift#1835 for more context.

Since exceptions are immutable, trying to modify the traceback will cause runtime errors (see https://github.com/reddit/baseplate.py/actions/runs/5205369418/jobs/9390744047 for an example).

So is making exceptions immutable wrong?

[gvanrossum]

@fishy There is no need for the __traceback__ attribute to be read-only -- it is not included in the hash calculation. In fact, it looks like Exception just inherits its equality and hash implementations from object, which only takes the object's address into account.

[fishy]

we did override both __eq__ and __hash__ in thrift generated python exceptions, though. but thanks for the pointer, I think we at least have some workarounds for now.