Raise error when encountering unhandled callbacks #93
Conversation
Before now, exiting `transaction` would run all after-commit callbacks, on the assumption that they were enqueued inside the transaction. In tests this sometimes hid order-of-execution bugs, where pre-existing unhandled after-commit callbacks would get called, but at the wrong time. When opening a transaction, we will now check for pre-existing unhandled after-commit callbacks, and raise an error when they're found.
| | ||
| callbacks: tuple[Callable[[], object], ...] |
There was a problem hiding this comment.
Note: I'm not sure if it's a good idea go add these callbacks to the exception. 🤔
| | ||
| ### Added | ||
| | ||
| - An error will be raised when opening a transaction if there are pre-existing unhandled after-commit callbacks. |
There was a problem hiding this comment.
We should mention here that this is a tests-only feature. Or we could check it outside tests too.
There was a problem hiding this comment.
This failure-mode is only possible in tests. You're right though that I should make that clearer.
| - An error will be raised when opening a transaction if there are pre-existing unhandled after-commit callbacks. | ||
| The pre-existing callbacks would previously run when `transaction` exits. | ||
| This helps catch order-of-execution bugs in tests. | ||
| The old behavior can be restored using `settings.SUBATOMIC_CATCH_UNHANDLED_AFTER_COMMIT_CALLBACKS` |
There was a problem hiding this comment.
If we keep this feature tests-only, we should have TEST in the setting name.
| | ||
| callbacks: tuple[Callable[[], object], ...] |
HamaBarhamou left a comment
HamaBarhamou left a comment There was a problem hiding this comment.
Thanks—this looks great! Two quick points:
CHANGELOG / naming — Since this is tests-only, I’d either explicitly note “(tests only)” in the CHANGELOG entry or rename the flag to
SUBATOMIC_TEST_CATCH_UNHANDLED_AFTER_COMMIT_CALLBACKS(and similarlySUBATOMIC_TEST_RUN_AFTER_COMMIT_CALLBACKS) to avoid ambiguity.Exception DX — Exposing the callables via
callbacksis handy but can get noisy (closures, long reprs). Maybe keep the attribute for debugging, but tweak the exception message/__str__to show just a count + actionable hint, e.g.
Found 2 unhandled on_commit callbacks. Ensure they’re run before opening a transaction, or set SUBATOMIC_TEST_CATCH_UNHANDLED_AFTER_COMMIT_CALLBACKS=False to opt out in tests.(Nit) A brief comment in
_execute_on_commit_callbacks_in_testsexplaining why we check both the entry state (only_in_testcase_transaction) and the exit state (_innermost_atomic_block_wraps_testcase) would help clarify the intent (avoid false positives if context changes between enter/exit).
Before now, exiting
transactionwould run all after-commit callbacks, on the assumption that they were enqueued inside the transaction. In tests this sometimes hid order-of-execution bugs, where pre-existing unhandled after-commit callbacks would get called, but at the wrong time.When opening a transaction, we will now check for pre-existing unhandled after-commit callbacks, and raise an error when they're found.
TODO:
Fixes #31
Alternative to #87