Hi, I noticed that Zope 2.11 includes a recent version of the transaction module including the transaction.doom() method. But I don't see any check for it in ZPublisher. So, if any code calls transaction.doom(), the publisher will raise a user-visible exception when it tries to call commit(). This seems less than useful :-) In the discussion I've seen of this method, eg. https://bugs.launchpad.net/zope3/+bug/98382 and http://markmail.org/message/3yshpmltvhevnrff it sounds like other people share my expectation... namely, that the developer should not have to do anything special after calling transaction.doom(); the transaction is not committed, and the user won't see an exception. Is that the concensus? If so, there's an easy solution - apply something like the attached patch. Anybody disagree? -- Paul Winkler http://www.slinkp.com
On Thu, Jul 10, 2008 at 12:12:06AM -0400, Paul Winkler wrote:
Hi,
I noticed that Zope 2.11 includes a recent version of the transaction module including the transaction.doom() method. But I don't see any check for it in ZPublisher.
So, if any code calls transaction.doom(), the publisher will raise a user-visible exception when it tries to call commit(). This seems less than useful :-)
In the discussion I've seen of this method, eg. https://bugs.launchpad.net/zope3/+bug/98382 and http://markmail.org/message/3yshpmltvhevnrff it sounds like other people share my expectation... namely, that the developer should not have to do anything special after calling transaction.doom(); the transaction is not committed, and the user won't see an exception.
Yes.
Is that the concensus? If so, there's an easy solution - apply something like the attached patch.
Anybody disagree?
I havn't investigated properly, but it may be necessary to do the isDoomed() check at a higher level where you can abort. Code running after the commit() expects a new transaction and now will not get that. Other than that, an explicit abort() if the transaction is doomed may be sufficient. Zope3 does: def afterCall(self, request, ob): txn = transaction.get() if txn.isDoomed(): txn.abort() else: self.annotateTransaction(txn, request, ob) txn.commit()
--
Paul Winkler http://www.slinkp.com
--- Zope2/App/startup.py~ 2008-06-14 02:50:23.000000000 -0400 +++ Zope2/App/startup.py 2008-07-10 00:08:02.000000000 -0400 @@ -267,7 +267,8 @@ transaction.begin()
def commit(self): - transaction.commit() + if not transaction.isDoomed(): + transaction.commit()
def abort(self): transaction.abort() --- ZPublisher/Publish.py~ 2008-06-14 02:50:48.000000000 -0400 +++ ZPublisher/Publish.py 2008-07-10 00:08:08.000000000 -0400 @@ -314,7 +314,8 @@ def begin(self): transaction.begin() def commit(self): - transaction.commit() + if not transaction.get().isDoomed(): + transaction.commit() def abort(self): transaction.abort() def recordMetaData(self, object, request): --- ZPublisher/WSGIPublisher.py~ 2008-06-14 02:50:48.000000000 -0400 +++ ZPublisher/WSGIPublisher.py 2008-07-09 23:59:57.000000000 -0400 @@ -401,7 +401,8 @@ def begin(self): transaction.begin() def commit(self): - transaction.commit() + if not transaction.get().isDoomed(): + transaction.commit() def abort(self): transaction.abort() def recordMetaData(self, object, request):
_______________________________________________ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )
-- Brian Sutherland
--On 10. Juli 2008 17:09:36 +0200 Brian Sutherland <brian@vanguardistas.net> wrote:
On Thu, Jul 10, 2008 at 12:12:06AM -0400, Paul Winkler wrote:
Hi,
I havn't investigated properly, but it may be necessary to do the isDoomed() check at a higher level where you can abort.
What do you mean by "higher level"? I think that the check within the ZPublisher is the highest and right place.
Code running after the commit() expects a new transaction and now will not get that.
You refer to code executed as part of a ZODB post-commit handler? If a transaction is doomed then such handlers should never be executed - right? Andreas
Andreas Jung wrote at 2008-7-12 07:17 +0200:
... What do you mean by "higher level"? I think that the check within the ZPublisher is the highest and right place.
Code running after the commit() expects a new transaction and now will not get that.
You refer to code executed as part of a ZODB post-commit handler? If a transaction is doomed then such handlers should never be executed - right?
The problem is that a doomed transaction prevents "joining". This means that any operation that causes a join during error handling will fail. Examples are: accessing a session, accessing a relational database. The bug is in the ZODB ("transaction") code. A doomed transaction should not prevent joining. The only justification for the prevention is that "transaction" knows that any modification (potential cause of the join) will not be able to be committed (thus, "transaction" can indicate a problem earlier). *HOWEVER*, transactions can be joined for other purposes as modifications (example relational database access) and sometimes modifications should be possible even then they cannot be (and are not expected to be) committed (example error handling). Thus, please let us fix "transaction". -- Dieter
On Sun, Jul 13, 2008 at 09:05:16AM +0200, Dieter Maurer wrote:
Andreas Jung wrote at 2008-7-12 07:17 +0200:
... What do you mean by "higher level"? I think that the check within the ZPublisher is the highest and right place.
Code running after the commit() expects a new transaction and now will not get that.
You refer to code executed as part of a ZODB post-commit handler? If a transaction is doomed then such handlers should never be executed - right?
The problem is that a doomed transaction prevents "joining".
This means that any operation that causes a join during error handling will fail. Examples are: accessing a session, accessing a relational database.
The bug is in the ZODB ("transaction") code. A doomed transaction should not prevent joining.
Do you have an example of this bug? It should be fixed. It is already tested in doom.txt like this: A doomed transaction should act the same as an active transaction, so we should be able to join it: >>> txn = transaction.begin() >>> txn.doom() >>> dm2 = DataManager() >>> txn.join(dm2) But perhaps there is another way to make it fail?
The only justification for the prevention is that "transaction" knows that any modification (potential cause of the join) will not be able to be committed (thus, "transaction" can indicate a problem earlier). *HOWEVER*, transactions can be joined for other purposes as modifications (example relational database access) and sometimes modifications should be possible even then they cannot be (and are not expected to be) committed (example error handling).
Yep, you should still be able to join doomed transactions. According to the first line of doom.txt, the intention of doom is: A doomed transaction behaves exactly the same way as an active transaction but raises an error on any attempt to commit it, thus forcing an abort.
Thus, please let us fix "transaction".
-- Dieter
-- Brian Sutherland
Brian Sutherland wrote at 2008-7-13 12:41 +0200:
On Sun, Jul 13, 2008 at 09:05:16AM +0200, Dieter Maurer wrote:
Andreas Jung wrote at 2008-7-12 07:17 +0200:
... What do you mean by "higher level"? I think that the check within the ZPublisher is the highest and right place.
Code running after the commit() expects a new transaction and now will not get that.
You refer to code executed as part of a ZODB post-commit handler? If a transaction is doomed then such handlers should never be executed - right?
The problem is that a doomed transaction prevents "joining".
This means that any operation that causes a join during error handling will fail. Examples are: accessing a session, accessing a relational database.
The bug is in the ZODB ("transaction") code. A doomed transaction should not prevent joining.
Do you have an example of this bug? It should be fixed. It is already tested in doom.txt like this:
Thus, maybe, someone already has fixed this problem. In this case, executing the error handling in the same transaction as the main request should no longer make problems (this was where I have seen the bug). -- Dieter
On Sat, Jul 12, 2008 at 07:17:31AM +0200, Andreas Jung wrote:
--On 10. Juli 2008 17:09:36 +0200 Brian Sutherland <brian@vanguardistas.net> wrote:
On Thu, Jul 10, 2008 at 12:12:06AM -0400, Paul Winkler wrote:
Hi,
I havn't investigated properly, but it may be necessary to do the isDoomed() check at a higher level where you can abort.
What do you mean by "higher level"? I think that the check within the ZPublisher is the highest and right place.
Ok, as I said, I havn't investigated properly.
Code running after the commit() expects a new transaction and now will not get that.
You refer to code executed as part of a ZODB post-commit handler? If a transaction is doomed then such handlers should never be executed - right?
If I see a function named "commit" I expect that calling it will cause a commit, or raise an error. I don't expect that nothing will happen and I will have the same transaction after as before. So this part of the patch: def commit(self): - transaction.commit() + if not transaction.get().isDoomed(): + transaction.commit() Changes the meaning of the commit() function from "Commit or error" to "maybe commit, or maybe do nothing, or maybe error". That raises lots of warning bells in my head. But, it's probably OK, depending on what the code calling commit() expects.
Andreas
-- Brian Sutherland
participants (4)
-
Andreas Jung -
Brian Sutherland -
Dieter Maurer -
Paul Winkler