Message redelivery failing for BMT MDBs

[tmjkeeney]

I am running BMT MDBs under JBoss 3.2.3 using the JBossMQ Provider and have noticed a problem with BMT MDBs failing to redeliver JMS messages when the MDB throws a system exception, a condition that violates the EJB spec section 18.3.3. I have created a patch for the problem and tested it with both BMT and CMT MDBs (After patching the code, any system exception will force redelivery until max retries, then the message goes to the DLQ). Before I apply this patch to my production codebase, I wanted to confirm with the JBoss developers if this is an appropriate solution that will not impact other parts of the system.

I tracked the message redelivery failure for BMT MDBs to the exception handler for the code block beginning on line 1100 in the JMSContainerInvoker.onMessage() method:

 try
 {
 Transaction tx = tm.getTransaction();

 // DLQHandling
 if (useDLQ && // Is Dead Letter Queue used at all
 message.getJMSRedelivered() && // Was message resent
 dlqHandler.handleRedeliveredMessage(message, tx)) //Did the DLQ handler take care of the message
 {
 // Message will be placed on Dead Letter Queue,
 // if redelivered to many times
 return;
 }

 invoker.invoke(id, // Object id - where used?
 ON_MESSAGE, // Method to invoke
 new Object[]{message}, // argument
 tx, // Transaction
 null, // Principal
 null); // Cred

 }
 catch (Exception e)
 {
 log.error("Exception in JMSCI message listener", e);
 }

If you modify the exception handler to rethrow any system exceptions, the StdServerSession can set the transaction for rollback. So here's the patch:

 try
 {
 Transaction tx = tm.getTransaction();

 // DLQHandling
 if (useDLQ && // Is Dead Letter Queue used at all
 message.getJMSRedelivered() && // Was message resent
 dlqHandler.handleRedeliveredMessage(message, tx)) //Did the DLQ handler take care of the message
 {
 // Message will be placed on Dead Letter Queue,
 // if redelivered to many times
 return;
 }

 invoker.invoke(id, // Object id - where used?
 ON_MESSAGE, // Method to invoke
 new Object[]{message}, // argument
 tx, // Transaction
 null, // Principal
 null); // Cred

 }
 catch (Exception e)
 {
 log.error("Exception in JMSCI message listener", e);

> // rethrow system exceptions as an EJBException
> if (e instanceof RuntimeException || e instanceof RemoteException) {
> throw new EJBException("Encountered system exception.", e);
> }
 }

As said above, this patch works fine for CMT and BMT MDBs. The only problem I see is that CMT MDBs will setRollbackOnly twice: once in the AbstractInterceptor.invokeNext() method, line 105, and once in the StdServerSession.onMessage(), line 275. This could be fixed by wrapping line 275 in StdServerSession with a check to see if the transaction Status is already STATUS_MARKED_ROLLBACK. Correct?

Any comments on the problem/solution would be much appreciated!

Thanks in advance.
--Tom

[adrian.brock]

Your patch is not correct.

1) It is an error for onMessage to throw an unchecked exception and
is explicitly undefined behaviour, read the jms spec.

2) For BMT you are deliberatly suspending the jms delivery transaction so you
can either isolate or create your own user transaction. Failure in the EJB should
not lead to a jms rollback.

CMT and REQUIRED is what you should use if you want this behaviour.

[tmjkeeney]

Thank you for your evaluation. I want to elaborate on my reasoning:

1) CMT with REQUIRED would be preferable, but the MDB processes large amounts of data and the container-provided transaction could very well timeout. Also, the processing does not need to be wrapped in a transaction so I chose BMT. The problem with BMT is that there is no message redelivery and subsequent posting to the DLQ like in CMT.

2) O'Reilly's EJB book, 3rd ed. (pg. 428), recommends that a BMT MDB throw an EJBException if message redelivery is needed, since under this condition the container will not acknowledge the message. The patch modified JBoss so it would follow this recommendation.

3) The JMS spec does indeed recommend that onMessage() *not* throw a RuntimeException, but the EJB spec (Section 18.3.3) defines the container response if this happens. I see this as a conflict between the two specs.



I agree that the patch conflicts with the JMS spec. I can't say though that I haven't seen phrases like "screw the spec" in the src ;-). The patch does satisfy my application requirements and appears to have no impact on other components I use in my production environment. I will proceed with your warning duly noted and try some other approach if possible.

--Tom

[adrian.brock]

The JMS spec does allow redelivery after a RuntimeException, it's just that
you cannot rely on this behaviour (it is not portable).

e.g. JBoss effectively uses the rule: redeliver 0 (zero) times.

I'll look at incorporting your patch with an optional configuration in the
invoker-proxy-binding, something like:

<uncheckedhandler>org.jboss.ejb.plugins.jms.DefaultUncheckedHandler</>

Where you can add your own code.
The default will be as now for backwards compatibility.

Please post a patch at www.sf.net/projects/jboss and assign it to "ejort"

I see no need to handle the impossible RemoteException

e.g.

package org.jboss.ejb.plugins.jms;

public interface UncheckedHandler
{
 void handle(Throwable t);
}

public class DefaultUncheckedHandler implements UncheckedHandler
{
 public void handle(Throwable t)
 {
 // Do nothing
 }
}

public class RethrowUncheckedHandler implements UncheckedHandler
{
 public void handle(Throwable t)
 {
 if (t instanceof RuntimeException)
 throw (RuntimeException) t;
 else if (t instanceof Error)
 throw (Error) t;
 else
 throw new UndeclaredThrowableException(t);
 }
}

P.S. The code in JMSCI should read:

catch (Throwable t)
 {
 log.error("Exception in JMSCI message listener", t);
 handler.handle(t);
 }

[tmjkeeney]

I have posted the patch as requested. Apologies about the delay - I have been very busy preparing for the initial release of our JBoss environment and supporting software.

--Tom