Add abort() related statuses to Transaction.

[jenstroeger]

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 the Status class into the Transaction class, such that users can then write:

if tx.status is Transaction.Status.ACTIVE:
    pass  # Do something.

Alternatively, following the Transaction.isDoomed() approach, adding equivalent isActive(), isCommitted(), isFailed() and isAborted() would be sensible. I would prefer the latter as it’s the least impact on the Status class:

if tx.isActive():
    pass  # Do something.

Addendum

The abort() function of the initial commit 7160162 tested the status of a transaction (like commit() and join() do) and would raise a TransactionFailedError 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?

[jamadden]

@jenstroeger Can you please rebase on master?

[jamadden]

I am not sure if it is intended behavior that a transaction with "Commit failed" or "Abort failed" status can still be aborted… again?

To me that makes sense. It makes abort() (close to) idempotent. You can call it all the way up your stack of exception handlers:

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 

[mmerickel]

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.

[icemac]

@jenstroeger Is this PR still active or can it be closed?

[jenstroeger]

@icemac, I think it ought to me merged, no?

[icemac]

@jenstroeger No, this pull request has not been merged, yet, e. g. I can't find the ABORTFAILED variable on the master branch. Currently at least the request of @jamadden to rebase the pull request to the master branch is still open.

[jenstroeger]

@icemac, I’ll be offline until early July if it can wait that long?

[icemac]

@jenstroeger No problem, take the time you need.

[jamadden]

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.

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 (NoTransaction, specifically). That's documented.

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 Transaction and TransactionManager. For both explicit and implicit TransactionManagers, attempting to abort the same transaction twice results in a ValueError.

>>> 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 tx.doom()? It doesn't do the same thing as abort(), in that it doesn't actually release any resources, it only schedules them to be released in the future. Sometimes timely releasing matters (for example, if an error handler needs to do something that takes an arbitrary amount of time --- say, sending an email error report --- I'd rather not have database locks hang around while that happens).

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 begin() a new transaction. One could argue that they shouldn't be able to abort() it either; I would accept that, if doom() was passed through to resource managers so they could do pre-emptive cleanup; I would also suggest that the other lifecycle methods on a Transaction instance be deprecated and it documented that using a TransactionManager to access them is the strongly preferred way.

Letting Transaction.abort be idempotent seems like a small, safe change. #84 does that.

[jamadden]

This has conflicts now and would need to be rebased.

[jenstroeger]

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?

[jenstroeger]

Picking up the change again: I just rebased on the latest changes.

@jamadden, would you like to address #85 as part of this PR or a separate one?

[jenstroeger]

So, commit() contains

transaction/src/transaction/_transaction.py

Lines 264 to 265 in 647880b

	if self.status is Status.COMMITFAILED:
	self._prior_operation_failed() # doesn't return

and I think we’d want something similar in abort(), for example

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 environmentfilter function is deprecated so I’m inclined to say that Sphinx has a bug here.

[jenstroeger]

Probably move that down to line 555, right before the for rm in self._resources loop which iterates and aborts.

[icemac]

Also, a few jobs of the current Action run fail because of Sphinx

These problems are solved in #111. (Currently waiting for review.)

[jenstroeger]

Also, a few jobs of the current Action run fail because of Sphinx

These problems are solved in #111. (Currently waiting for review.)

Thanks @icemac! I’ll wait for the merge then…

[icemac]

#111 is merged now.

[jenstroeger]

#111 is merged now.

Thanks @icemac, rebased my PR.

[jenstroeger]

Ping?