bpo-25782: avoid hang in PyErr_SetObject when current exception has a…#27626
bpo-25782: avoid hang in PyErr_SetObject when current exception has a…#27626ambv merged 7 commits intopython:mainfrom
Conversation
… cycle in its __context__ chain
|
Maybe it would be nice to have a test case with a longer cycle? As in def test_no_hang_on_context_chain_cycle2(self):
class A(Exception):
pass
class B(Exception):
pass
class C(Exception):
pass
class D(Exception):
pass
class E(Exception):
pass
# Context cycle:
# +-----------+
# V |
# E --> D --> C --> B --> A
with self.assertRaises(E):
try:
raise A()
except A as _a:
a = _a
try:
raise B()
except B as _b:
b = _b
try:
raise C()
except C as _c:
c = _c
a.__context__ = c
try:
raise D()
except D as _d:
d = _d
e = E()
raise e
# Finish without breaking the cycle
self.assertIs(e.__context__, d)
self.assertIs(d.__context__, c)
self.assertIs(c.__context__, b)
self.assertIs(b.__context__, a)
self.assertIs(a.__context__, c)Also, would it be good to check that the traceback doesn't get stuck in a loop when printing? I tried it, and it terminates as you'd hope, but it's unclear to me why that's the case. |
|
The traceback printing code does its own cycle detection. I'll extend the test, thanks. |
|
If you do add a longer test, I would keep the short one there as it helps to have a minimal example. |
cjerdonek
left a comment
There was a problem hiding this comment.
Is it possible / would it make sense to also add a test case that corresponds to the example I asked about most recently in the ticket (where the top exception was part of a cycle)?
| exc = e | ||
|
|
||
| self.assertIsInstance(exc, TypeError) | ||
| self.assertIsInstance(exc.__context__, ValueError) |
There was a problem hiding this comment.
Does it make sense also to check exc.__context__.__context__ (to check that the cycle was preserved)?
|
I thought of another case that might be worth testing. What about e.g. |
You're right. This test should have been there already because this is old behaviour, but it doesn't seem to be there. I've added it. |
cjerdonek
left a comment
There was a problem hiding this comment.
Thanks for all your great work on this, @iritkatriel. I approve! 👍
|
Thank you @cjerdonek. @pablogsal , @ambv - let's figure out how far back to backport this. According to https://bugs.python.org/issue25782#msg399281 it seems like 3.9? |
|
Thanks @iritkatriel for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10. |
… cycle in its context chain (pythonGH-27626) Co-authored-by: Dennis Sweeney 36520290+sweeneyde@users.noreply.github.com (cherry picked from commit d5c2174) Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
|
GH-27706 is a backport of this pull request to the 3.10 branch. |
… cycle in its context chain (pythonGH-27626) Co-authored-by: Dennis Sweeney 36520290+sweeneyde@users.noreply.github.com (cherry picked from commit d5c2174) Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
|
GH-27707 is a backport of this pull request to the 3.9 branch. |
… cycle in its context chain
Co-authored-by: Dennis Sweeney 36520290+sweeneyde@users.noreply.github.com
https://bugs.python.org/issue25782