X Tutup
Skip to content

bpo-25782: avoid hang in PyErr_SetObject when current exception has a…#27626

Merged
ambv merged 7 commits intopython:mainfrom
iritkatriel:bpo-25782
Aug 10, 2021
Merged

bpo-25782: avoid hang in PyErr_SetObject when current exception has a…#27626
ambv merged 7 commits intopython:mainfrom
iritkatriel:bpo-25782

Conversation

@iritkatriel
Copy link
Copy Markdown
Member

@iritkatriel iritkatriel commented Aug 6, 2021

… cycle in its context chain

Co-authored-by: Dennis Sweeney 36520290+sweeneyde@users.noreply.github.com

https://bugs.python.org/issue25782

@sweeneyde
Copy link
Copy Markdown
Member

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.

@iritkatriel
Copy link
Copy Markdown
Member Author

The traceback printing code does its own cycle detection. I'll extend the test, thanks.

@cjerdonek
Copy link
Copy Markdown
Member

If you do add a longer test, I would keep the short one there as it helps to have a minimal example.

Copy link
Copy Markdown
Member

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

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

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)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does it make sense also to check exc.__context__.__context__ (to check that the cycle was preserved)?

@cjerdonek
Copy link
Copy Markdown
Member

cjerdonek commented Aug 8, 2021

I thought of another case that might be worth testing. What about e.g. A -> B -> C and then raising C while A is active (basically the "Avoid creating new reference cycles" part of your code comment)? I believe that's an example of the kind of case that comment is referencing (or if not, maybe you could add one).

@iritkatriel
Copy link
Copy Markdown
Member Author

I thought of another case that might be worth testing. What about e.g. A -> B -> C and then raising C while A is active (basically the "Avoid creating new reference cycles" part of your code comment)? I believe that's an example of the kind of case that comment is referencing (or if not, maybe you could add one).

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.

Copy link
Copy Markdown
Member

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

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

Thanks for all your great work on this, @iritkatriel. I approve! 👍

@iritkatriel
Copy link
Copy Markdown
Member Author

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?

@ambv ambv merged commit d5c2174 into python:main Aug 10, 2021
@miss-islington
Copy link
Copy Markdown
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 10, 2021
… 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>
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Aug 10, 2021
@bedevere-bot
Copy link
Copy Markdown

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 10, 2021
… 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>
@bedevere-bot
Copy link
Copy Markdown

GH-27707 is a backport of this pull request to the 3.9 branch.

ambv pushed a commit that referenced this pull request Aug 10, 2021
… cycle in its context chain (GH-27626) (GH-27707)

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>
miss-islington added a commit that referenced this pull request Aug 10, 2021
… cycle in its context chain (GH-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>
@iritkatriel iritkatriel deleted the bpo-25782 branch November 5, 2021 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

X Tutup