Why does com.arjuna.ats.internal.jts.orbspecific.coordinator.ArjunaTransactionImple throw INVALID or Inactive when we know the status is ABORTED?

[tomekadamski]

I have hit a race condition where the EJB interceptors and the reaper are both aborting a transaction simultaneously. If the reaper doesn't finish rollback before ejb thread enters it then the ejb thread gets INVALID_TRANSACTION (via ArjunaTransactionImple) (even though the transaction is still on the thread). ArjunaTransactionImple knows that the status is ActionStatus.ABORTED but elects to throw INVALID_TRANSACTION. By changing the code to throw TRANSACTION_ROLLEDBACK instead the EJB interceptor thread can correctly reason about the aborted transaction.

More specifically, here are the two possible outcomes depending on the order in which both threads execute com.arjuna.ats.internal.jts.ControlWrapper code:

Scenario 1:

* reaper executes ControlWrapper#rollback method first

* server executes ControlWrapper#rollback; line _controlImpl.getImplHandle().rollback() throws an NullPointerException as ArjunaTransactionImple has been already detached exception is caught and TransactionRolledbackException is thrown

Scenario 2:

* both threads enter ControlWrapper#rollback at the same time

* server thread obtains ArjunaTransactionImple; transaction is in ABORTED state so InvalidTransaction exception is thrown

Attempt to synchronize the code:

synchronized (_controlImpl) {

        _controlImpl.getImplHandle().rollback();

}

on _controlImpl object doesn't work as it introduces possible deadlock to the code. Another possibility would be to check transaction status inside ArjunaTransactionImple#rollback method and throw TransactionRolledbackException when transaction is already aborted. I have tested such fix in my branch and it fixes the initial error - TransactionRolledbackException is thrown consistently.

Is such fix possible to introduce? Are there some problems that it may introduce?

[mmusgrov]

... Another possibility would be to check transaction status inside ArjunaTransactionImple#rollback method and throw TransactionRolledbackException when transaction is already aborted. I have tested such fix in my branch and it fixes the initial error - TransactionRolledbackException is thrown consistently.

Is such fix possible to introduce? Are there some problems that it may introduce?

I think it is safe to just return from the rollback call if it is already rolled back. In fact there is already a comment in the method that says that was the intention so it's an oversight. The comment says:

/*
* If the transaction has already rolledback then silently ignore
* the multiple rollback attempts.
*/

[tomekadamski]

This is possible but if we return from ArjunaTransactionImple#rollback without any exception the there will be exception in the server thread - so again we have inconsistency here, because if reaper executes the whole rollback method before server threads gets there then TransactionRolledback exception is thrown.

[mmusgrov]

Do you mean the code in ControlWrapper#rollback that catches NPE (narayana/ControlWrapper.java at master · jbosstm/narayana · GitHub) when looking up the control and interprets it as rollback (presumed abort) so throws the TransactionRolledback exception. If so then I agree we should be consistent and just return from that method call normally (ie without an exception).

[tomekadamski]

Yes this may be a solution but I think that TransactionRolledback exception is good in this scenario as user is notified what was the reason why his invocation has failed.

[mmusgrov]

The EJB interceptor has called rollback so it should generate a new exception back to the user to notify him that his transaction has rolled back.

[tomjenkinson]

I might be missing something but if the user (directly or indirectly) asks for a TX to rollback and a rollback happened (even if it was instantaneously with the reaper) would there be anything that the user would do differently?

[tomekadamski]

In this scenario user uses CMT transaction. Transaction is not marked as rollback_only and no exception is thrown so user is not asking for rollback. The interceptor code finds out that the transaction is not in active state and performes rollback to disassociate it from the thread. (The relevant code is CMTTxInterceptor#endTransaction).

[tomjenkinson]

Understood - +1 to Mikes suggestion of throwing your own exception then.

[tomjenkinson]

But we do need the patch to allow the multiple rollback calls I think. It seems you would not want the INVALID_TRANSACTION.

Note that the JTS implementation has to work with OTS too, so you do occasionally get differences in behaviour. It would be useful if you can add this difference to our issue that records these: [JBTM-2122] Go through JTS code and compare/contrast with capabilities missing in other dist tx code we've got - JBoss I…

[tomekadamski]

CMTTxInterceptor catches every possible exception and rethrows either EJBTransactionRolledbackException if RollbackException has been thrown or EJBException otherwise. As there is possibility that corba's TRANSACTION_ROLLEDBACK exception would be thrown by ArjunaTransactionImple I would have to treat this exception the same as RollbackException for this solution to work. I don't like this idea though - it is not very elegant .

[mmusgrov]

My suggestion was that we do not throw any exceptions if you call rollback when the transaction is already aborted. Then it is your responsibility to tell the user thread that the transaction rolled back.

The only caveat would be if you called rollback when no transaction is associated in which case we would throw an exception.

[tomekadamski]

Yes, scenario in which null is returned works correctly, the problem is the second possibility that is TRANSACTION_ROLLEDBACK thrown by ControlWrapper#rollback.

[mmusgrov]

In comment Re: Why does com.arjuna.ats.internal.jts.orbspecific.coordinator.ArjunaTransactionImple throw INVALID or Inactive when we know the status is ABORTED? above I said

  {quote}If so then I agree we should be consistent and just return from that method call normally (ie without an exception).{quote}

[tomekadamski]

Sorry, missed it. This seems a good solution. I'm making the change and start running tests.