[mypyc] Fix exception swallowing in async try/finally blocks with await #19353
Conversation
Any await that causes a context switch inside a finally block swallows any exception (re-)raised in the preceding try or except blocks. As the exception 'never happened', this also changes control flow. This commit adds several tests (which fail) for this bug, and triggers a compiler error if this pattern is detected in the code. `# type: ignore[mypyc-try-finally-await, unused-ignore]` can be used on the try line to bypass the error. This also newly causes the testAsyncReturn test to fail, as it should. See mypyc/mypyc#1114
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
When a try/finally block in an async function contains an await statement in the finally block, exceptions raised in the try block are silently swallowed if a context switch occurs. This happens because mypyc stores exception information in registers that don't survive across await points. The Problem: - mypyc's transform_try_finally_stmt uses error_catch_op to save exceptions to a register, then reraise_exception_op to restore from that register - When await causes a context switch, register values are lost - The exception information is gone, causing silent exception swallowing The Solution: - Add new transform_try_finally_stmt_async for async-aware exception handling - Use sys.exc_info() to preserve exceptions across context switches instead of registers - Check error indicator first to handle new exceptions raised in finally - Route to async version when finally block contains await expressions Implementation Details: - transform_try_finally_stmt_async uses get_exc_info_op/restore_exc_info_op which work with sys.exc_info() that survives context switches - Proper exception priority: new exceptions in finally replace originals - Added has_await_in_block helper to detect await expressions Test Coverage: Added comprehensive async exception handling tests: - testAsyncTryExceptFinallyAwait: 8 test cases covering various scenarios - Simple try/finally with exception and await - Exception caught but not re-raised - Exception caught and re-raised - Different exception raised in except - Try/except inside finally block - Try/finally inside finally block - Control case without await - Normal flow without exceptions - testAsyncContextManagerExceptionHandling: Verifies async with still works - Basic exception propagation - Exception in __aexit__ replacing original See mypyc/mypyc#1114
Chainfire
force-pushed
the
try-finally-await
branch
from
8c9600d to
589694f
Compare
for more information, see https://pre-commit.ci
|
I couldn't let it go so I reverted the compiler error and created a fix instead, based on how async context managers work. With ample help from artificial friends. [mypyc] Fix exception swallowing in async try/finally blocks with await When a try/finally block in an async function contains an await statement The Problem:
The Solution:
Implementation Details:
Test Coverage:
|
Chainfire
changed the title
|
Thanks for the PR! There may be an easier way to fix this. Here's my attempt (it doesn't quite work yet though): One of the new tests added in this PR fails with the above changes, however, but maybe there's a way to fix it (I wonder if the fix in #19361 is relevant here): Do you think that my fix above looks like a promising alternative? If yes, maybe you can figure out why we are getting the AttributeError (feel free to use my suggestion above in your PR)? |
|
Well, you're the expert here. I've attempted several different approaches that I now don't even remember the details of, the one I PR'd is the only one I could get to work. If I made an oversight that would've made it fixable much easier in a different way, then that is for you to determine. I did apply your patch over my other patch to see if it solves the AttributeError, but it doesn't - and many more tests are failing now. The only place I've seen the AttributeError so far is if a spilled value is read while it has been created but not been assigned to or holds NULL (not Py_None). Perhaps that is a hint ? Either way, I spent nearly two full days on this patch because I have code that just needs to work. I do not have time right now to spend another two days trying to get a different approach to work. |
|
I will spend some time later this week and see I can get my approach to work so that all your tests pass, as it could be more compact. But it's quite possible that it's a dead end, and since your PR here works and fixes the issue, we can always get back to it merge it. This part of the codebase is quite tricky, so this is quite impressive work. I also like the detailed test coverage. |
EDIT: This text is obsolete and replaced by my next post, scroll down.
[mypyc] Test cases and compilation error for mypyc-try-finally-await
Any await that causes a context switch inside a finally block swallows any exception (re-)raised in the preceding try or except blocks. As the exception 'never happened', this also changes control flow.
This commit adds several tests (which intentionally fail under mypyc but run fine under cpython) for this bug, and triggers a compiler error if this pattern is detected in the code.
# type: ignore[mypyc-try-finally-await, unused-ignore]can be used on the try line to bypass the error.This also newly causes the testAsyncReturn test to fail, as it should.
See mypyc/mypyc#1114
I have made several attempts to fix this, but as I am unfamiliar with the code base and this does not seem to be a quick simple fix, I have given up on that for now.
I found it more important to create a compiler error that detects this case so my code stops silently and unexpectedly swallowing exceptions and changing control flow.
I've run the full pytest suite under Python 3.12.10, and the only additional failures are one of the test cases I added and testAsyncReturn (as noted above). Mypyc compiles itself and mypy and some small projects of my own.