Skip to content

Refs #31040, Refs #31224 -- Prevented cycles in exceptions chain.#12978

Merged
felixxm merged 1 commit intodjango:masterfrom
felixxm:process_exception_by_middleware
May 28, 2020
Merged

Refs #31040, Refs #31224 -- Prevented cycles in exceptions chain.#12978
felixxm merged 1 commit intodjango:masterfrom
felixxm:process_exception_by_middleware

Conversation

@felixxm
Copy link
Copy Markdown
Member

@felixxm felixxm commented May 26, 2020

Exception handling was raising an exception that was creating a cycle in the exception chain (by re-raising an exception that was already being handled).

Thanks Chris Jerdonek for detailed analysis.

See also https://bugs.python.org/issue40696.

@felixxm felixxm requested a review from carltongibson May 26, 2020 06:21
@felixxm felixxm changed the title Refs #31040, Refs #31224 -- Prevented creating cycles in exceptions chain. Refs #31040, Refs #31224 -- Prevented cycles in exceptions chain. May 26, 2020
Exception handling was raising an exception that was creating a cycle
in the exception chain (by re-raising an exception that was already
being handled).

Thanks Chris Jerdonek for detailed analysis.
@felixxm felixxm force-pushed the process_exception_by_middleware branch from d9260a7 to 6c38cd9 Compare May 26, 2020 06:22
@cjerdonek
Copy link
Copy Markdown
Contributor

Great! Are you able to confirm that the test that was hanging with Python3.9.0a6 passes with this change?

@felixxm
Copy link
Copy Markdown
Member Author

felixxm commented May 26, 2020

Yes it works with this patch.

@cjerdonek
Copy link
Copy Markdown
Contributor

Looking at the patch: In process_exception_by_middleware(), what do middleware_method's do with the exception? Is it common or possible that they would re-raise the same exception, and would that also wind up causing the same situation? In the change to process_exception_by_middleware(), it appears that only the case where it gets past all middlewares is being addressed.

@felixxm
Copy link
Copy Markdown
Member Author

felixxm commented May 26, 2020

process_exception() should return either None or an HttpResponse object, so it's possible but it's not supported.

Copy link
Copy Markdown
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

OK, thanks @felixxm.

I might just be inclined to adjust the exended commit message to emphasize that it's sync_to_async that leads has the extra layer.

Async exception handling was raising an exception that was creating a
cycle in the exception chain (by re-raising an exception in
sync_to_async that was already being handled ).

(Up to you!) 👍

@felixxm felixxm merged commit d94a9aa into django:master May 28, 2020
@felixxm felixxm deleted the process_exception_by_middleware branch May 28, 2020 11:05
@felixxm
Copy link
Copy Markdown
Member Author

felixxm commented May 28, 2020

@carltongibson Thanks for checking 👍

sarahboyce pushed a commit to bmispelon/django that referenced this pull request Jun 18, 2024
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.

3 participants