-
Notifications
You must be signed in to change notification settings - Fork 6
Raise error when encountering unhandled callbacks #93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
HamaBarhamou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
263f581 to
2382836
Compare
Thanks for this -- I've renamed the flag to append
I'm going to keep it as-is for now. While explaining the meaning of the error is likely to be useful the first time it's encountered, I think the benefit of seeing the callbacks will be most useful long-term.
You're right, but the comments here need further improvement anyway, so I'm going to leave that for a later refinement. |
samueljsb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nitpick about the changelog, but otherwise this looks like it's ready now!
| # Django's `atomic` leaves unhandled after-commit actions on exit. | ||
| with django_transaction.atomic(): | ||
| db.run_after_commit(counter.increment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice demonstration of the cause of this problem.
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. Co-authored-by: Samuel Searles-Bryant <[email protected]>
e96eaf7 to
810aeb0
Compare
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.
Fixes #31
Alternative to #87