EJBTHREE-1142 - Incomplete fix - Generic problem

[adrian.brock]

I linked this issue to where it was originally reported
http://jira.jboss.com/jira/browse/JBAS-4980

But the fix shows what is generally wrong with a lot of the EJB3 code
(especially the deployment stuff).

 // Commit or rollback depending on the status
 if (commit == false || transaction.getStatus() == Status.STATUS_MARKED_ROLLBACK)
 {
 if (trace)
 log.trace("MessageEndpoint " + getProxyString(proxy) + " rollback");
 tm.rollback();
 }
 else
 {
 if (trace)
 log.trace("MessageEndpoint " + getProxyString(proxy) + " commit");
 tm.commit();
 }

 // We're done with the transaction
 transaction = null;

So if tm.commit() throws an error, the transaction variable is not cleared
meaning the problem is not fixed.
The nulling of the transaction variable should be in the finally block further down:

 finally
 {
+ transaction = null;

 // Resume any suspended transaction
 if (currentTx != null)
 {
 try
 {
 tm.resume(currentTx);
 }
 catch (Throwable t)
 {
 log.warn("MessageEndpoint " + getProxyString(proxy) + " failed to resume old transaction " + currentTx);

 }
 }
 }

Basically, the code works great under normal circumstances
but falls apart on any error.

[wolfc]

I was thinking about putting a TODO there.

I don't want to disassociate when there is an error on rollback or commit. In that case we have bigger problems. The whole MessageInflowLocalProxy should be invalidated then. Hmm, except for heuristic exceptions.

We then should resume the suspended tx. Oh bugrit, this will get complicated with the tidy up hack in there. What do we do if we have a suspended and a 'wrong' tx?

[adrian.brock]

"wolfc" wrote:

We then should resume the suspended tx. Oh bugrit, this will get complicated with the tidy up hack in there. What do we do if we have a suspended and a 'wrong' tx?

Are you talking about

 // Suspend any bad transaction - there is bug somewhere, but we will try to tidy things up
 if (currentTx != null && currentTx.equals(transaction) == false)
 {
 log.warn("Current transaction " + currentTx + " is not the expected transaction.");
 tm.suspend();
 tm.resume(transaction);
 }

The log message should really be something like:

 log.warn("SOMEBODY HAS PLAYING WITH THINGS THEY DON"T UNDERSTAND!");

So there are more serious problems anyway.
This has nothing to do with the endpoint's state so there is no need to invalidate it.
A rollback exception when you attempt commit() is expected
e.g. optimistic failure in the database, etc.