transaction.doom() for form processing

[Steve Alexander]

I think there's a problem with the use of transaction.abort() in the EditView and similar code.

 class EditView(BrowserView):
 ...
     def update(self):
 ...
         if Update in self.request:
             changed = False
             try:
                 changed = applyWidgetsChanges(self, self.schema,
                     target=content, names=self.fieldNames)
                 # We should not generate events when an adapter is used.
                 # That's the adapter's job.
                 if changed and self.context is self.adapted:
                     description = Attributes(self.schema, *self.fieldNames)
                     notify(ObjectModifiedEvent(content, description))
             except WidgetsError, errors:
                 self.errors = errors
                 status = _("An error occurred.")
                 transaction.abort()
             else:
 ...

The transaction is aborted when there is a widget error. The problem with doing this is that a new transaction will implicitly begin. (That's a misdesign in the transactions system in my opinion, but a separate issue.) At the end of the request processing, the publisher will see that no exception bubbled up, and the (new) transaction has no conflicts or whatever, and will commit the transaction. So, if any processing is done after the transaction.abort() in the code above, that causes actions or writes on a commit, these actions or writes will occur, even though this is not what is intended for the application.

I think the intention behind the form-processing code is to Doom the transaction. That is, to ensure that the transaction will not be committed at any future time, but to allow the transaction to proceed with its present state.

I proposed a transaction.doom() API a while ago.

  http://mail.zope.org/pipermail/zodb-dev/2004-January/006414.html

The changes would be:

 * Add transaction.doom() to the API.

 * Calling transaction.commit() on a doomed transaction will cause a DoomedTransaction exception to be raised.

 * The publication code catches this exception from its call to commit(), and handles it by aborting the transaction.

 * Fix the bug in EditView and FormView by changing abort() to doom(). (Also, check if there are other places inappropriately using transaction.abort()).

 * Document that code running inside a Zope request should never use transaction.abort(), but may use transaction.doom().

 * Optionally: extend the transaction API to allow you to ask "is this transaction doomed?"

[Steve Alexander (stevea)]

I think EJB has this API:

  EJBContext.setRollbackOnly();

Maybe they were worried about the religious connotations of "Doom".

[Florent Guillaume (efge)]

+1 on tm.setRollbackOnly()
(It's part of the Java Transaction API (JTA), quite lower than EJB)

Or we could call it tm.setAbortOnly() if we want our naming to be coherent.

[Steve Alexander (stevea)]

I'm actually more in favour of doom(). It is succinct, and describes well what it means.

I'll note that the article where I found the EJB or JTM API was entitiled "How to doom an EJB transaction".

http://www.theserverside.com/discussions/thread.tss?thread_id=39915

It was not entitiled "How to ensure a transaction will be rolled back instead of committed".

[Steve Alexander (stevea)]

Also, I'm against APIs that are gratuitously named "setSomething". Particularly when it doesn't make sense to unset it later.

I'm in favour of APIs that are named to describe the actions that will be taken. So, to my mind, "doom" (a specific verb) is better than "set" (a less specific verb) in the method name.

[Jim Fulton (jim-zope)]

+1

[Jim Fulton (jim-zope)]

Sorry, I was +1-ing the general idea of such an API.
I don't have time atm to have an opinion on what it's called.

I also suspect that such an API already exists, so the naming issue
may be moot. :)

[Florent Guillaume (efge)]

tm.status = Status.COMMITFAILED is enough to prevent any commit, but there's no API that I can see for it.

[Florent Guillaume (efge)]

I mean txn.status, not tm.status.

[Brian Sutherland (jinty)]

I've prepared some patches implementing this, but would like to ask for comments before committing something as core as this.

I also added the requirement that, except for not being able to commit, a doomed transaction behaves exactly the same as an active transaction.

The first patches implement a transaction.doom() function:

 http://lentejasverdes.ath.cx/stuff/zope/patch.transaction
 http://lentejasverdes.ath.cx/stuff/zope/patch.zopeapp

These two then implement an isDoomed() function:

 http://lentejasverdes.ath.cx/stuff/zope/patch.transaction2
 http://lentejasverdes.ath.cx/stuff/zope/patch.zopeapp2

[Brian Sutherland (jinty)]

The patch also contains some improvements suggested by stevea.

[Brian Sutherland (jinty)]

FYI, after over 2 weeks of silence, I committed the ZODB part of the patches I posted in revision 70066.

[Gary Poster (gary)]

what is the interaction of this patch with savepoints?

what is the desired interaction of this patch with savepoints?

We have code in which the following story would be desired:

sp = transaction.savepoint()
try:
    # ...do stuff that uses form machinery or something else that might
    # doom a transaction...
except SomeException:
    sp.rollback() # we actively don't want the whole transaction doomed (or anything
    # else that happened since we made the savepoint).
transaction.commit() # we want this to work.

What's the story here? I'm sorry no one has replied, and it certainly spurred me to comment.

[Tres Seaver (tseaver)]

I would say that savepoints are doomed to lose.

Applications which need to be able to "retry" part of the transaction after such an error can't use 'doom()', because the
rule violation which triggers the 'doom()' call is non-negotiable,
by design.

[Gary Poster (gary)]

I don't mind the semantics Tres proposes (which I expect reflect the patch's reality), but if that's used then I'm not OK with transaction.doom being used in the current form classes in formlib. That change in behavior isn't something you can deprecate, AFAICT. Make separate classes with this behavior, perhaps (maybe also in formlib?).

[Steve Alexander (stevea)]

In my opinion, a doom() is a promise never to commit the current transaction. It is semantically equivalent to an abort(), but the work gets done later. What is the interaction between a savepoint, a rollback and an abort?

I think formlib must be changed to use doom() because the current use of abort() is dangerous, and can lead to unwanted data being committed in an implicit new transaction.

[Gary Poster (gary)]

Very good point. I retract my concern. It's appropriate for formlib. FWIW:

gary@posterboard:~ $ ./bin/zopectl debug
>>> root['hello'] = 'there'
>>> import transaction
>>> sp = transaction.savepoint()
>>> root['foo'] = 'bar'
>>> transaction.abort()
>>> root.get('foo')
>>> root.get('hello')
>>> sp.rollback()
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
  File "/home/gary/classifieds/var/src/zope3/src/transaction/_transaction.py", line 744, in rollback
    raise interfaces.InvalidSavepointRollbackError
transaction.interfaces.InvalidSavepointRollbackError

[Steve Alexander (stevea)]

Thanks for doing the practical experiment, Gary.

So, for a web application, it seems to me that:

 - the publication is responsible for starting, committing and aborting transactions.

 - application code spread around wherever is responsible for dooming translactions. application code should never start, commit or abort a transaction.

 - self-contained units of application code can use savepoint and rollback, but the developer needs to be aware of the codepath that occurs within the savepoint->rollback, or expect that an InvalidSavepointRollbackError may be raised on rollback(). It will often be safe to simply ignore this error, because the publication will be aborting anyway.

It would be nice if there was a specific subclass of InvalidSavepointRollbackError that is a DoomedTransactionRollbackError. Then I could write code that does:

    try:
        transaction.rollback()
    except DoomedTransactionRollbackError:
        # hmm, was doomed during the savepoint.
        goto SKIP_OTHER_PROCESSING

Actually, we can do that anyway by saying:

    except InvalidSavepointRollbackError:
         if transaction.is_doomed():
             goto SKIP_OTHER_PROCESSING

[Brian Sutherland (jinty)]

I've seen that the version of the transaction module supporting doom() has made it to the zope trunk and have committed the last portion of the patch in 72976.

It seemed that all of the doubts about the use of doom() in zope.app.form were resolved. Indeed doom by design is only the promise never to commit the current transaction. I think of it as a bear trap at the finish line.

[Brian Sutherland (jinty)]

As I mentioned in earlier comments, I've committed fixes for this bug. Just never had the access in the collector to close it.

[Wolfgang Schnerring (wosc)]

has long been released