python / cpython

The Python programming language
https://www.python.org
Other
62.27k stars 29.92k forks source link

Trackstack includes `__exit__` in `contextlib` which is not in previous versions #92118

Closed Cheukting closed 2 years ago

Cheukting commented 2 years ago

Bug report

See https://github.com/HypothesisWorks/hypothesis/issues/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

Also looping @Zac-HD in

hauntsaninja commented 2 years ago

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 commented 2 years ago

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

iritkatriel commented 2 years ago

I'm looking, this was caused by:

commit 396b58345f81d4c8c5a52546d2288e666a1b9b8b Author: Irit Katriel 1055913+iritkatriel@users.noreply.github.com Date: Fri Dec 17 14:46:22 2021 +0000

bpo-45711: Remove type and traceback from exc_info (GH-30122)
Cheukting commented 2 years ago

Thanks @iritkatriel

iritkatriel commented 2 years ago

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 commented 2 years ago

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

iritkatriel commented 2 years ago

PR is here: https://github.com/python/cpython/pull/92202

serhiy-storchaka commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

I made a PR to add a test for ExitStack. This test passes on 3.10 as well, so there was no change there: https://github.com/python/cpython/pull/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 commented 2 years ago

Thank you for reporting this @Cheukting .

graingert commented 2 years ago

@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 commented 1 year ago

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 https://github.com/apache/thrift/pull/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 commented 1 year ago

@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 commented 1 year ago

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.