Regression on setRollbackOnly

[wolfc]

With the introduction of https://jira.jboss.org/browse/JBTM-234 we see regression on EJB 3 testsuites when setRollbackOnly is called from afterCompletion. We expect to get an IllegalStateException, instead a SystemException is thrown.

As the issue states OTS raises Inactive:

OTS 1.4 2.6.14 rollback_only
The transaction associated with the target object is modified so that the only possible
outcome is to rollback the transaction. The Inactive exception is raised if the
transaction has already been prepared.

While the JTA spec doesn't explicitly account for such a scenario.

To recount:

void beforeCompletion()

The beforeCompletion method is called by the transaction manager  prior  to the start of the two-phase transaction commit process. This call is  executed with the transaction context of the transaction that is being  committed.

void afterCompletion(int status)

This method is called by the transaction  manager after the transaction is committed or rolled back.

And

The aftercompletion callback will be invoked in an undefined context.

void setRollbackOnly() throws java.lang.IllegalStateException, SystemException

IllegalStateException - Thrown if the target object is not associated with any transaction.
SystemException - Thrown if the transaction manager encounters an unexpected error condition.

Now it really becomes a choice which exception should be thrown.

public class SystemException extends java.lang.Exception

The SystemException is thrown by the transaction manager to   indicate that it has encountered an unexpected error condition  that prevents future transaction services from proceeding.

In principal both the state of the transaction and transaction services are not compromised and transaction services proceeds in a defined manner. The outcome of the transaction is the status as passed into the afterCompletion.

Based on the description in SystemException I would say that TS should not throw that exception, but instead the IllegalStateException.

[marklittle]

Are you sure it's SystemException and not something like InactiveTransactionException or InvalidTerminationStateException, both of which are derived from SystemException?

I'll ignore the opportunity to ask why something would want to call setRollbackOnly in afterCompletion ;-) 'cause that's a bit like putting your seatbelt on after the car has crashed.

[wolfc]

Are you sure it's SystemException and not something like InactiveTransactionException or InvalidTerminationStateException, both of which are derived from SystemException?

Ehr, I get an InactiveTransactionException which is a SystemException. To me InactiveTransactionException doesn't really exist as it's not part of the spec.

I'll ignore the opportunity to ask why something would want to call setRollbackOnly in afterCompletion ;-) 'cause that's a bit like putting your seatbelt on after the car has crashed.

Ignoring your ignorance :-) , I completely agree and it's not an user scenario nor a support case. Some test tool that likes splitting straws.

[marklittle]

OK, so the reason we now throw InactiveTransactionException is because (as the JIRA states) IllegalStateException has a well defined meaning according to JTA "Thrown if the current thread is not associated with a transaction." In this case there is a transaction associated with the calling thread, it's just that it's no longer active. OTS correctly differentiates these cases, but once again JTA is broken in this regard

So what we did was provide a new SystemException to try to allow for the differentiation in the case someone codes against our specific exceptions as well as those who don't care and only catch SystemExceptions (or not). I didn't like the idea of deriving from IllegalStateException because in the case of users who only catch that exception rather than, say, an InactiveTransactionException that is derived from ISE, their code may assume one thing (no transaction) when in fact that's not the case. At least with an exception derived from SE code cannot make any assumption about the thread-to-transaction association.

[marklittle]

Replying to my own reply ;-)

It's clear that the brain-dead JTA specification text around Transaction.setRollbackOnly doesn't consider the case of an inactive transaction when in fact it should, since to have a Transaction reference it doesn't make sense for IllegalStateException to mean "Thrown if the current thread is not associated with a transaction." If that were the case then Transaction should be null.

Plus it doesn't work in a multi-threaded environment, particularly if checked transaction semantics are disabled.

So it may make sense for derive from ISE rather than SE.

Let me sleep on this, or at least have a few beers.

[marklittle]

OK, I made a change (https://jira.jboss.org/browse/JBTM-754). You'll need to check with Jonathan as to when this can be made available.