Sirs,

I noticed this post on the Spring Framework developer list. It looked serious enough to warrant posting here for your view...

Regards
N


From: j

Thanks for posting it :-) I would have done that myself after waiting for initial reactions on the Spring developer list, but here we go...

Of course, this is not necessarily a Hibernate bug but rather a semantic issue. Most callers will assume that if commit throws an exception that a database commit attempt did fail and that thus the database transaction has been rolled back.

Both Hibernate's JDBCTransaction and JTATransaction do not behave that way but rather leave the database transaction uncompleted in case of a flushing failure. One could of course try to use the following idiom, taken from the Hibernate reference docs:



That will cause an explicit rollback if commit throws an exception. It doesn't differentiate between flushing failures and commit failures, though: In case of a flushing failure, the rollback is appropriate; but in case of a commit failure, the rollback might throw an exception itself because the failed commit has already completed the transaction.

Especially when using JTA, the rollback will definitely throw an IllegalStateException after a commit failure as the thread is no longer associated with a JTA transaction then. With JDBC, the behavior probably depends on the driver.

There's a further minor issue: A commit failure will have invoked session.afterTransactionCompletion, therefore an additional rollback call will cause another afterTransactionCompletion invocation. This might not cause any misbehavior, but it indicates that the workflow is not right.

A programming idiom that works around the issue might look as follows. The main issue is to separately deal with flushing failures and commit failures.



Not particularly elegant, is it? It would probably be more appropriate to change the JDBCTransaction/JTATransaction commit implementation so that it automatically rolls back the database transaction in case of a flushing failure on commit. The programming idiom would still have to change though, as rollback must not be invoked after a failed commit but only on previous failures.



Not too elegant either? But it won't get better than that if one cares about proper behavior on both flushing failures and commit failures. At least with manual coding; Spring's generic transaction management does not confront you with those nasty details, besides making transaction demarcation more high-level in general of course :-)

As indicated, this is straightforward to fix without any Hibernate implementation changes by causing an explicit flush before attempting to commit (like also shown above). Fortunately, there is only one place in Spring code that needs to be touched: HibernateTransactionManager's doCommit implementation. So Spring/Hibernate applications do not have to worry but just update Spring to the upcoming 1.0 M2!

Juergen

In the JDBCTransaction case, invoking rollback after a failed commit does not really matter as it just rollbacks a new empty transaction. The problem occurs in the JTATransaction case where UserTransaction.rollback will throw an IllegalStateException if UserTransaction.commit has failed.

Hibernate's Transaction abstraction does not cover those differences yet. There should be clear semantics, i.e. consistent behavior of Transaction.commit and Transaction.rollback. As JTA does not allow you to invoke rollback after a commit attempt, the only behavior that can be implemented with both JDBC and JTA seems to be a commit method that causes an implicit rollback in case of failure.

This means:
- JTATransaction.commit should invoke UserTransaction.commit if flushing succeeds, UserTransaction.rollback else.
- JDBCTransaction.commit should invoke Connection.commit if flushing succeeds, Connection.rollback else.

The following refined idiom would match the new Transaction semantics:



The following should work with the current JTATransaction implementation, and with JDBC drivers that cause an implicit rollback on commit failure:



What I'm not sure about is how common JDBC drivers behave in case of a Connection.commit failure. Do they implicitly roll back the transaction, or do you need an explicit rollback call afterwards? In case of the latter, JDBCTransaction's commit implementation needs to call rollback in case of a commit failure too, not just in case of a flushing failure. JTATransaction still must not call UserTransaction.rollback after a commit attempt, though.

Juergen

P.S.: Fortunately, such a change in Hibernate's Transaction semantics should not break existing code. JTATransaction rollback callers in case of commit failure already have IllegalStateException problems with the current implementation. JDBCTransaction rollback callers would just trigger a rollback of a new empty transaction if the failed JDBCTransaction.commit call had implicitly caused a rollback already.

Gavin,



I took your quote from the developer list: I'm of course aware of that being in the docs. I don't think that you've understood my point, so I'll retry.

-----

Hibernate's Transaction.commit/rollback semantics are notably different from JDBC's Connection.commit/rollback and JTA's UserTransaction.commit/rollback: A failed commit has already *completed* the transaction with the latter, no need to call rollback anymore. With JTA, that's explicitly stated in the spec; with JDBC, it's rather de facto, as the spec is unclear in that respect.

In the JDBC case, an additional rollback call will silently roll back a new emtpy transaction (which doesn't really hurt). But in the JTA case, an additional rollback call will cause an IllegalStateException as there is no active transaction anymore (at least with proper JTA implementations that stick to the spec). Unfortunately, Hibernate's JTATransaction.commit can also fail due to flushing; in that case, rollback still needs to be called.

So from an application's point of view, it's not obvious what to do on commit failure with JTATransaction: in case of a flushing failure, need to call rollback to avoid dangling transaction; but in case of a JTA commit failure, must not call rollback to avoid IllegalStateException. As I've outlined, JDBCTransaction will just silently roll back a new empty transaction, so it's safe to call rollback in any case there.

Funnily enough, Hibernate's Session.afterTransactionCompletion gets called in a finally block in both JDBCTransaction's and JTATransaction's commit implementations. So even if commit fails due to flushing, afterTransactionCompletion is triggered; an additional rollback call by the aplication will then trigger *another* afterTransactionCompletion invocation. That also shows that there's some issue with API semantics.

-----

The only way I see to fix this is to redefine the semantics of Hibernate's Transaction interface: i.e. do not call rollback after a failed commit. Both JDBCTransaction and JTATransaction would have to trigger a rollback themselves then on a flushing failure before the actual commit. When commit returns, the transaction would have to be finished in any case, no matter if correctly or with an exception.

Admittedly, it's a bit hard to redefine semantics of such core API at that stage. But as far as I can consider, the current "call rollback after failed commit" idiom is broken with JTATransaction anyway. When used with JDBCTransaction, an additional rollback call wouldn't hurt, so existing code would not break after such a change in semantics. The new recommended way would be "no rollback after failed commit" though.

The only workaround for application code that I can think of with the current semantics is to explicitly call Session.flush before attempting a commit. In case of flushing failure (or any other data access failure), invoke Transaction.rollback; only call Transaction.commit when everything else succeeded. If commit itself fails, do nothing special -- just propagate the exception.

Of course, the latter workaround will cause double flushing of the Session at transaction commit time: once by application code, doing the actual work; once by the Transaction implementation, rechecking but not finding anything to flush (as intended). To avoid that inefficiency, one can set the Session to FlushMode.NEVER after having explicitly invoked flush; the Transaction implementation will then not even attempt to recheck.

-----

So there *is* an issue involved, although one can live with it if just using JDBCTransaction (accepting silent rollback of new empty transaction and double afterTransactionCompletion invocation), or by explicitly calling flush before commit. Please think it through again, I'm 100% sure that the semantics of the Transaction interface and the current implementations aren't clean -- particularly if you let them do the flushing of the Session.

Juergen

OK, I actually had "reply to Juergen's thread" on my TODO list. (Yes, really, its right there in TODO.txt.)

As I see it, the JTA-style semantics completely suck, since there is no way to implement a correct idiom without two try blocks. So, our style is better ;-)

The problem you have identified is not especially a problem, since all that happens in the worst case ( failure of underlyingTransaction.commit() ) is that we get an exception saying "No transaction in progress", instead of one saying "could not commit transaction".

But anyway, it is incredibly easily fixed by simply inspecting the JTA UserTransaction to see what the status of the transaction is, before the "second" rollback, or by adding an extra boolean instvar isAlreadyRolledBack to the Hibernate wrapper class.

I will do this today.

Happy?

P.S. Are you sure that all JDBC drivers try to roll back a transaction when commit fails?

Good point. We've actually researched that quite a bit and not found any JDBC driver that doesn't behave that way. Commit calls seem to only fail because the database could not be accessed at all; all checks regarding constraint violation etc have already happended on statement execution. Can you think of any other reason why a JDBC commit could fail?

With Hibernate, JDBCTransaction.commit will almost always fail because of a flushing failure. With the current idiom, the underlying JDBC Connection still needs to be rolled back then.

In the JTA case, UserTransaction.commit has a much higher chance of failing, as it potentially needs to perform a 2PC across multiple distributed databases. If any of those fails for some reason, commit will cause an implicit rollback and throw a respective exception (that would get swallowed by an additional rollback's IllegalStateException).

Fixing the JTATransaction.rollback implementation by checking whether the transaction is still active is a viable idea. It's not hyper-elegant in terms of semantics, as you'll rather work around the issue than clarify the exact meaning of the commit call (which still may or may not have already rolled back the transaction) -- but that's not a problem I guess.

Regarding two try blocks: That's because of the Session.close in the finally block. You just need two blocks if there are manually managed resources involved. With a plain UserTransaction, you need only one:



We've adopted the same semantics for Spring's PlatformTransactionManager interface, as there are no obvious resources involved at that abstraction level. And hardly anyone works with that interfact directly anyway, as TransactionTemplate or TransactionInterceptor are more convenient ways of demarcation.

Fortunately, IoC can help a lot in terms of try-catch blocks, as you don't need to write any such block at all with Spring's HibernateTemplate, or just one with direct Hibernate access code running inside Spring's transaction management... :-)

Juergen

Well, I can't but it doesn't sound impossible. If the JDBC spec is vague, is it better to be on the safe side?



Yes; I'm inclined to leave it the way it is.




I agree that this is suboptimal. But certainly not a huge problem.



I can't think of any reason why the user needs to know for sure the status of the transaction between the commit() and rollback() calls. But, anyway, they can always discover this by calling Transaction.wasRolledBack().



Yes, exactly. But don't you have a managed resource in ANY transaction?! Even in JTA, you need to release the JDBC connection. I certainly wanna stick with the single try/catch/finally. (OTOH, the alternative idiom avoids the tx==null check, so thats one thing in its favor.)

Anyway, I've got a fix I'm committing to CVS now.

Normally, JDBC drivers just send SQL "commit" or "rollback" statements to the database. In SQL scripts, you either issue a commit or a rollback but you don't check if a commit failed and issue a rollback in that case too. I have not been able to find any spec or docs on what to do in case of an SQL commit failure; it seems to be just natural to not do anything special but just propagate the error.

AFAIK, there is no reason why an SQL commit could leave the transaction in an unfinished state except that the connection to the database failed - in that case, issuing a rollback will fail too. Judging by all available info on the matter, it should *not* be necessary to invoke an additional rollback. That said, most JDBC examples still have the commit call within the try block and invoke rollback in the catch block.



Of course. I never wanted to imply that there is a severe problem involved. It's rather about API semantics that can easily be overcome by following a certain idiom to be on the safe side -- but are nevertheless not optimal.



It does make sense to separate between data access and transaction exceptions, e.g. for converting to distinct application-level exceptions or (far-fetched, I know ;-) application framework exceptions. One could use wasRolledBack to determine this too; I didn't consider that.



With JTA, the UserTransaction is not bound to the lifetime of a JDBC Connection, in contrast to Hibernate's Transaction that is bound to the Session. So you can close the resource via a finally block *within* the transaction, e.g. doing this in a DAO but demarcating the transaction in a business object. Strictly speaking, there are still two try blocks, but in different methods -- that is, only one at the transaction demarcation level.

BTW, have a look at SolarMetric's docs on JDO transaction handling at:
http://www.solarmetric.com/Software/Documentation/latest/docs/jdo_overview_trans_jdo.html.

With JDO, it's different to both JDBC and JTA again: If a JDOFatalException gets thrown (possibly a database failure on JDBC commit), the transaction has already been rolled back; on any other exception (like a flushing failure), you need to manually invoke rollback. But that's just what SolarMetric says: The JDO spec does not contain that clarification but leaves this unspecified just like JDBC.

Juergen

Its interesting to consider that, since all Hibernate exceptions are considered fatal, a possible design decision would have been to always roll back the transaction inside Hibernate. No catch block at all!

But, in reality this is too limiting since even though the exception might be fatal to the Hibernate session, that doesn't mean its fatal to the whole (distributed) transaction.

I'm happy with our idiom + semantics. Its simple and flexible.

Actually, that is what some JDBC drivers do: They turn the transaction rollback-only internally in case of SQLExceptions. I agree that it is too limiting in general, though.



We specifically chose it for Spring because it's the most clearest in terms of the meaning of commit and rollback methods, and the only one that maps to JTA's semantics without workarounds. But just because it is the most appropriate one for a high-level transaction manager interface does not necessarily mean that it is the most appropriate one for Hibernate's Session-level native transactions... ;-)

We're trying to abstract the native transaction behavior and provide consistent semantics across all transaction strategies. This means that our HibernateTransactionManager will cause a necessary rollback on flushing failure in its doCommit implementation, and JdoTransactionManager will implicitly cause a rollback in case of a Non-JDOFatalException on commit.

-----

I just checked whether wasRolledBack or wasCommitted would generally help to identify whether flushing or actual committing failed: The former doesn't, as it is just set by an explicit rollback, neither does the latter, as it just set by a successful commit.

One difference remains: An actual commit failure will cause a TransactionException while a flushing failure will cause any other HibernateException. In contrast to my initial assumption, both JDBCTransaction and JTATransaction will *not* wrap a flush exception in a TransactionException but just let it through as the original HibernateException.

One could use that check to determine whether to explictly roll back (and what framework exceptions to generate). The following is an adapted version of the idiom from the Hibernate docs:



Wouldn't this be appropriate for the current semantics of Hibernate's Transaction, assuming that a JDBC commit failure does not require an additional rollback call (in the JDBCTransaction case)? This would even work with the current JTATransaction, without the need to silently suppress an additional rollback call.

Juergen

Actually, these semantics are pretty clean: Transaction.commit will throw a TransactionException if the actual commit failed (no rollback necessary), and any other HibernateException if flushing failed (rollback necessary). If strictly defined that way, recommending the adapted idiom in the docs would already solve the issue.

Now there's only one thing left: Session.afterTransactionCompetion just needs to be called if flushing succeeded, not in a general finally block. As a flushing failure requires an explicit rollback call, afterTransactionCompletion will be triggered there anyway. I'm aware that double-completing doesn't hurt, but it would be cleaner.

Juergen

Have a look at the current CVS (v21branch).

Just did. Looks better now :-)

There's one little issue left though: To support the only-rollback-on-HibernateException-but-not-on-TransactionException idiom, JDBCTransaction.commit would need an afterTransactionCompletion(false) call in the catch(SQLException) block around Connection.commit.

If using that idiom, rollback will not be called on a failed actual commit that throws TransactionException -- therefore the Transaction.commit should call afterTransactionCompletion(false) in that case. If the flush fails, afterTransactionCompletion should not be called, as rollback will be invoked afterwards.

If using the classic idiom of calling rollback on any HibernateException, afterTransactionCompletion(false) would be called twice then but that wouldn't hurt. You could also add a "afterTransactionCompletionCalled" flag to avoid that. I vote for suggesting both idioms, guaranteeing at least one afterTransactionCompletion calls in any case.

I clearly prefer the idiom with no rollback on TransactionException but just on any other HibernateException. It's semantically clearer what happens there: One does not need to rely on the silent JTATransaction.rollback swallowing then, and does not invoke a Connection.rollback after a failed Connection.commit.

But of course, everyone is free to choose the other idiom -- as long as both are supported. I would recommend the no-rollback-on-TransactionException one though.

Juergen