-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add abort() related statuses to Transaction. #64
base: master
Are you sure you want to change the base?
Conversation
@jenstroeger Can you please rebase on master? |
To me that makes sense. It makes def a(tx):
try:
b(tx)
except:
tx.abort()
raise
def b(tx):
try:
c(tx)
except:
tx.abort()
raise
def c(tx):
try:
d(tx)
except:
tx.abort()
raise |
At least in explicit mode it is and should remain an error to abort an inactive transaction. In implicit mode usually abort starts an implicit begin and aborts it. |
@jenstroeger Is this PR still active or can it be closed? |
@icemac, I think it ought to me merged, no? |
@jenstroeger No, this pull request has not been merged, yet, e. g. I can't find the |
@icemac, I’ll be offline until early July if it can wait that long? |
@jenstroeger No problem, take the time you need. |
A transaction manager is either explicit or implicit, and I would fully agree that asking an explicit transaction manager to abort a transaction that doesn't exist should be an error ( An individual transaction, on the other hand...well, the current behaviour doesn't seem that useful and depends on the internal details of the implementation of >>> import transaction
>>> tm = transaction.TransactionManager(explicit=True) # Explicit
>>> tx = tm.begin()
>>> tx.abort()
None
>>> tx.abort()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "//lib/python3.8/site-packages/transaction/_transaction.py", line 522, in abort
self._free()
File "//lib/python3.8/site-packages/transaction/_transaction.py", line 473, in _free
self._manager.free(self)
File "//python3.8/site-packages/transaction/_manager.py", line 93, in free
raise ValueError("Foreign transaction")
ValueError: Foreign transaction
>>> tm = transaction.TransactionManager() # Implicit
>>> tx = tm.get()
>>> tx.abort()
None
>>> tx.abort()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "//lib/python3.8/site-packages/transaction/_transaction.py", line 522, in abort
self._free()
File "//lib/python3.8/site-packages/transaction/_transaction.py", line 473, in _free
self._manager.free(self)
File "//lib/python3.8/site-packages/transaction/_manager.py", line 93, in free
raise ValueError("Foreign transaction")
ValueError: Foreign transaction This is because the transaction doesn't "forget" its transaction manager when it is aborted, but the transaction manager did forget about it. I argue that there's no harm in allowing an individual transaction to be aborted more than once --- abort should be idempotent. Currently, if there's any chance that a transaction that needs to be aborted might already have been aborted, complicated error handling is required. def do_stuff_in_transaction(tx):
try:
do_other_stuff_in_transaction(tx)
except Abortable:
try:
tx.abort()
except ValueError:
# Was this caused by aborting a transaction twice, or by some other error?
# If it's "some other error" we should let it propagate because we may not
# have correctly cleaned things up.
try:
if transaction.get() is tx: # XXX: Assuming we're using the thread-local transaction manager
raise # "some other error"
except NoTransaction:
# We're using an explicit manager, so as long as we guessed right about
# using the thread-local manager, this means we're aborting twice.
pass
else:
# Not explicit, but not the same. Probably aborting twice
pass If each level might want to abort the transaction, this logic gets duplicated. In a shared code base it can be factored out, but cross libraries it may still be duplicated. Unless we can guarantee that this never happens in worker functions, the top-level of the framework that handles retries and scheduling also has to be prepared for this. So essentially every generic transaction runner has to include something like this. Why not simply Why not pass the active transaction manager down through the call tree instead of the transaction itself? Logically, the transaction manager belongs to the part of the framework that includes the retry and scheduling logic. Worker functions should never need to be able to Letting |
This has conflicts now and would need to be rebased. |
84f6d34
to
810a03c
Compare
I had completely forgotten about this PR, my apologies! Just rebased it on master, and I’m going to tend to fixes next. Are you still interested in this change? |
810a03c
to
3bdcea1
Compare
So, transaction/src/transaction/_transaction.py Lines 264 to 265 in 647880b
and I think we’d want something similar in if self.status is Status.ABORTFAILED:
self._prior_operation_failed() Also, a few jobs of the current Action run fail because of Sphinx. The Actions use Sphinx v3.5.4 which defines its Jinja2 dependency as 'Jinja2>=2.3' and therefore imports v3.1.1. However, as of Jinja2 v3.0.0 the |
@@ -541,6 +546,7 @@ def abort(self): | |||
|
|||
try: | |||
self._synchronizers.map(lambda s: s.beforeCompletion(self)) | |||
self.status = Status.ABORTING |
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.
Probably move that down to line 555, right before the for rm in self._resources
loop which iterates and aborts.
These problems are solved in #111. (Currently waiting for review.) |
Thanks @icemac! I’ll wait for the merge then… |
#111 is merged now. |
32d0c67
to
ea21c00
Compare
Ping? |
Address parts of issue #63. This change adds three new statuses,
"Aborting"
,"Aborted"
, and"Abort failed"
, and wires them into the appropriate places.I have not yet made the
Status
class public, as I wanted to discuss that first. It currently is not exposed by the module initialization and I feel reluctant to add it there. Instead, it makes sense to move theStatus
class into theTransaction
class, such that users can then write:Alternatively, following the
Transaction.isDoomed()
approach, adding equivalentisActive()
,isCommitted()
,isFailed()
andisAborted()
would be sensible. I would prefer the latter as it’s the least impact on theStatus
class:Addendum
The
abort()
function of the initial commit 7160162 tested the status of a transaction (likecommit()
andjoin()
do) and would raise aTransactionFailedError
if a failed transaction would be aborted. However, that broke three of the test cases, and so I removed that check with commit 84f6d34. I am not sure if it is intended behavior that a transaction with"Commit failed"
or"Abort failed"
status can still be aborted… again?