Fix transaction issues #36
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When running inside an existing transaction the disable_side_effects decorator has a bug. It disables and then enables the registry so that side-effects don't run, but if the call to `run_side_effects` is deferred using `transaction.on_commit` it may be called after the context manager has exited, in which case the registry will have been re-enabled. In this case the side-effects will run. The fix is to pull some of the logic around suppression out so that it runs immediately on calling `run_side_effects` - if the registry is disabled at that point it will fire the signal and return immediately.
hugorodgerbrown
commented
Jan 16, 2024
Comment on lines
-66
to
+65
| # when in TEST_MODE do not defer the side-effects, as they | ||
| # are never fired anyway. | ||
| registry_func = ( | ||
| registry.run_side_effects | ||
| if TEST_MODE | ||
| else registry.run_side_effects_on_commit | ||
| ) | ||
| registry_func(label, *args, return_value=return_value, **kwargs) | ||
| registry.run_side_effects(label, *args, return_value=return_value, **kwargs) |
There was a problem hiding this comment.
Part one of the change is to always call run_side_effects rather than adding in fancy logic to the decorator. Simpler.
hugorodgerbrown
commented
Jan 16, 2024
side_effects/registry.py
Outdated
Comment on lines
162
to
180
| # if the registry is suppressed we are inside disable_side_effects, | ||
| # so we send the signal and return early. | ||
| if _registry.is_suppressed: | ||
| suppressed_side_effect.send(Registry, label=label) | ||
| else: | ||
| transaction.on_commit( | ||
| partial( | ||
| _registry.run_side_effects, | ||
| label, | ||
| *args, | ||
| return_value=return_value, | ||
| **kwargs, | ||
| ) |
There was a problem hiding this comment.
This is the meat of the change. Instead of having two separate methods we now always run the method on_commit, but we abort early if _registry.is_suppressed - which is True inside the disable_side_effects context. This means that we immediately return regardless of the transaction. The transaction is only relevant when actually running the side-effects for real, which is the desired behaviour.
hugorodgerbrown
force-pushed
the
fix-transaction-issues
branch
from
1e0d552 to
0bd8ae4
Compare
hugorodgerbrown
commented
Jan 16, 2024
Comment on lines
-143
to
-148
| # TODO: this is all becoming over-complex - need to simplify this | ||
| self.try_bind_all(label, *args, return_value=return_value, **kwargs) | ||
| if self.is_suppressed: | ||
| self.suppressed_side_effect.send(Registry, label=label) | ||
| else: | ||
| self._run_side_effects(label, *args, return_value=return_value, **kwargs) |
There was a problem hiding this comment.
TODONE - it was too complex so I have removed it.
hugorodgerbrown
force-pushed
the
fix-transaction-issues
branch
from
864a6aa to
8f21d92
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a refactoring of the internals so that when using the
disable_side_effectscontext manager or decorator therun_side_effectsmethod call is completely bypassed, and thesuppressed_side_effectsignal is immediately fired, rather than deferred tilltransaction.on_commit.This should fix a bug whereby side-effects were deferred using
transaction.on_commit, which meant that when they were called the registry was outside of the context manager and theis_suppressedattr was no longer true. i.e. side-effects were fired even when usingdisable_side_effectscorrectly.