Solution of JBREM-954 and ramifications into UnifiedInvokerH

[galder.zamarreno]

JBREM-954

First things firts, I believe that Remoting and AS as a whole, in the context of an EJB application, is a library and hence should not swallow the interruption request and instead rethrow it. I think everyone agrees on that.

I've meaning to start a topic on this due to the ramifications it has on the UnifiedInvokerHAProxy. In principle, resolving JBREM-954 should be simple, i.e. rethrowing InterruptedException (or some form of it - IE for abbreviation) instead of CannotConnectException.

Main issue here is that, from UnifiedInvokerHAProxy's point of view, it throws back an IE as it is. So, if UnifiedInvokerHAProxy did nothing about it and threw it back as it is, unless the EJB declared that it could throw an IE, the client would get a java.lang.reflect.UndeclaredThrowableException wrapping the IE.

This is not nice and I don't think we can expect for all EJBs to declare IE. The cleanest solution I can think of is for either Remoting or the Unified invoker layer to rethrow an IE wrapped inside a RuntimeException and let the users deal with it appropiately. The unified invoker ha proxy does not differenciate between IE or RE, so we would be able to cope with both situations. Seeing that we already do some exception laundering in the Unified invoker HA proxy, I'd probably go for the laundering of IE into RE wrapping IE to happen there.

Thoughts?

[galder.zamarreno]

godammit! i hate when this forum chops the subject and does not even tell you when you preview it

[galder.zamarreno]

Actually, if we made that change in UnifiedInvokerHAProxy, i.e laundering of IE, we'd have to do it in UnifiedInvokerProxy as well, so might be better if Remoting did it for us.

[dmlloyd]

I think Remoting should do it. Though I don't know offhand how big a change this is in the Remoting 2.x codebase. But there is a general need to differentiate between a client-side interruption and server-side interruption. This is a not unique requirement of the unified invoker, as far as I can see. And it certainly shouldn't break existing code, since it's already broken, right?

[galder.zamarreno]

Bugged, I messed up the link at the top again. Here's the proper link:

http://jira.jboss.com/jira/browse/JBREM-954

I think Remoting should do it. Though I don't know offhand how big a change this is in the Remoting 2.x codebase. But there is a general need to differentiate between a client-side interruption and server-side interruption. This is a not unique requirement of the unified invoker, as far as I can see. And it certainly shouldn't break existing code, since it's already broken, right?

It has a very nasty side effect on the HA version: As an IE is wrapped in a CannotConnectException, that destination is not longer available in the proxy. So a client side interruption results on the client not being able to connect to the server it was trying to connect to until the proxy is regenerated.

[ron_sigal]

I'm going to regurgitate all of this just to verify that I understand the issues.

1. We don't want Remoting turning an InterruptedException into a CannotConnectException because that has a negative effect in a clustered context.

2. We don't want Remoting rethrowing an InterruptedException because that could result in the application code getting an UndeclaredThrowableException.

So, we want to wrap the InterruptedException in something, other than a CannotConnectException, that will not result in an UndeclaredThrowableException .
Is that the kernel of the matter, so far?

From the point of Remoting, I agree that the current situation is misleading, but I'm slightly uncomfortable adding a RuntimeException to the API. UnitedInvokerProxy and UnitedInvokerHAProxy are controlled by JBoss, so we can handle any API changes, but for standalone Remoting users, we would be springing on them a stealth change in the API.

Here's an alternative proposal. Remoting could turn an InterruptedException into an InterruptedIOException, which is a subclass of IOException.

1. You could argue that it's still an API change, but MicroSocketClientInvoker.transport() already declares that it throws an IOException, so it's just adding an IOException with a different message.

2. Isn't it likely that application code, EJB or otherwise, already catches and copes with IOExceptions?

Well, I guess you could argue that an IOException would be unexpected in the case of a local EJB.

I'm torn.

WDYT?

[galder.zamarreno]

I'm going to regurgitate all of this just to verify that I understand the issues.

1. We don't want Remoting turning an InterruptedException into a CannotConnectException because that has a negative effect in a clustered context.

2. We don't want Remoting rethrowing an InterruptedException because that could result in the application code getting an UndeclaredThrowableException.

So, we want to wrap the InterruptedException in something, other than a CannotConnectException, that will not result in an UndeclaredThrowableException .
Is that the kernel of the matter, so far?

Yeah

From the point of Remoting, I agree that the current situation is misleading, but I'm slightly uncomfortable adding a RuntimeException to the API. UnitedInvokerProxy and UnitedInvokerHAProxy are controlled by JBoss, so we can handle any API changes, but for standalone Remoting users, we would be springing on them a stealth change in the API.

Client.invoke() is the interface Unified invokers and others deal with and this method throws Throwable so your users have to be prepared for anything anyway... In fact, the return value of Client.invoke() can even be an Exception...

Here's an alternative proposal. Remoting could turn an InterruptedException into an InterruptedIOException, which is a subclass of IOException.

1. You could argue that it's still an API change, but MicroSocketClientInvoker.transport() already declares that it throws an IOException, so it's just adding an IOException with a different message.

2. Isn't it likely that application code, EJB or otherwise, already catches and copes with IOExceptions?

Well, I guess you could argue that an IOException would be unexpected in the case of a local EJB.

I'm torn.

WDYT?

In EJB2, methods in remote business/home interfaces had to declare java.rmi.RemoteException, which is a subclass java.io.IOException, so your trick would work fine there.

In EJB3 this is not a requirement though. "The methods of the business interface may only throw the java.rmi.RemoteException if the interface extends java.rmi.Remote.". You can still have remote business interfaces ala EJB 2.x, but not a requirement. Just annotate your business method as @Remote and done. So, in the EJB3 world, there's a strong possibility of getting a UndeclaredThrowableException. Then again, in an EJB3 world, if you have a business interface which is gonna be Remote, I'm sure you'll declare that it can throw some kind of Exception, otherwise any IOExceptions would blow as UndeclaredThrowableException.

By the way, how would you turn InterruptedException into an InterruptedIOException? You can't set a cause Throwable to InterruptedIOException, so you can't really wrap IE around a InterruptedIOException,

[ron_sigal]

Client.invoke() is the interface Unified invokers and others deal with and this method throws Throwable so your users have to be prepared for anything anyway...

Good point. That relieves my unfounded anxiety.

By the way, how would you turn InterruptedException into an InterruptedIOException? You can't set a cause Throwable to InterruptedIOException, so you can't really wrap IE around a InterruptedIOException,

I was just going to do

catch (InterruptedException e)
{
 throw new InterruptedException(e.getMessage);
}

I'm looking at JBossMessaging as a test case. org.jboss.jms.client.delegate.DelegateSupport filters any Throwable thrown by org.jboss.remoting.Client.invoke() through the following code:

 public JMSException handleThrowable(Throwable t)
 {
 // ConnectionFailedException could happen during ConnectionFactory.createConnection.
 // IOException could happen during an interrupted exception.
 // CannotConnectionException could happen during a communication error between a connected
 // remoting client and the server (what means any new invocation).

 if (t instanceof JMSException)
 {
 return (JMSException)t;
 }
 else if (t instanceof CannotConnectException)
 {
 boolean failover = true;
 CannotConnectException cc = (CannotConnectException)t;
 Throwable underlying = cc.getCause();
 if (underlying != null && underlying instanceof SocketException)
 {
 //If remoting fails to find a connection because the client pool is full
 //then it throws a SocketException! - in this case we DO NOT want to failover
 //See http://jira.jboss.com/jira/browse/JBMESSAGING-1114
 if (underlying.getMessage() != null &&
 underlying.getMessage().startsWith("Can not obtain client socket connection from pool"))
 {
 log.warn("Timed out getting a connection from the pool. Try increasing clientMaxPoolSize and/or numberOfRetries " +
 "attributes in remoting-xxx-service.xml");
 failover = false;
 }
 }
 if (failover)
 {
 return new MessagingNetworkFailureException(cc);
 }
 }
 else if ((t instanceof IOException) || (t instanceof ConnectionFailedException))
 {
 return new MessagingNetworkFailureException((Exception)t);
 }
 //This can occur if failure happens when Client.connect() is called
 //Ideally remoting should have a consistent API
 else if (t instanceof RuntimeException)
 {
 RuntimeException re = (RuntimeException)t;

 Throwable initCause = re.getCause();

 if (initCause != null)
 {
 do
 {
 if ((initCause instanceof CannotConnectException) ||
 (initCause instanceof IOException) ||
 (initCause instanceof ConnectionFailedException))
 {
 return new MessagingNetworkFailureException((Exception)initCause);
 }
 initCause = initCause.getCause();
 }
 while (initCause != null);
 }
 }

 return new MessagingJMSException("Failed to invoke", t);
 }

I gather that a MessagingNetworkFailureException indicates the need to try to do a failover.

1. In its current state, this code would take an InterruptedException wrapped in a CannotConnectException and initiate a failover.

2. If Remoting replaced InterruptedExceptions by InterruptedIOExceptions, JBM would see an IOException and initiate a failover.

3. If Remoting wrapped InterruptedExceptions in RuntimeExceptions, JBM would return a MessagingJMSException and, presumably, not initiate a failover.

So, it turns out that strategy 3 is best for JBossMessaging. But it looks like good luck more than anything else. The treatment of CannotConnectException looks for one known anomaly (which, by the way, no longer exists) and does a failover by default. The treatment of RuntimeException looks for three particular cases and *doesn't* do a failover by default.

The fact is that none of us anticipated an InterruptedException in these circumstances. It seems to me that any existing code is equally likely to react well or badly in response to either wrapping InterruptedExceptions in RuntimeExceptions or replacing InterruptedExceptions by InterruptedExceptions.

The bottom line is that (1) something has to be done, and (2) there doesn't seem to be a solution that leaves the Remoting API unchanged and guarantees that no code suffers.

Is there any reason to prefer one solution over the other? One could argue that an interruptible EJB or JMS client *should* declare that it throws an InterruptedException, but that discussion might be more theological than technical. I'm leaning towards wrapping in RuntimeExceptions, which seems more straightforward and avoids the UndeclaredThrowableException problem.

There's one more concern. Suppose a Remoting user detected the InterruptedException possibility and their code looks for InterruptedExceptions wrapped in CannotConnectExceptions. Then replacing CannotConnectExceptions with RuntimeExceptions might break their code. I'm leaning towards

1. for Remoting 2.2.2.SP8

a) leaving the current behavior in place as the default behavior, and

b) making the new behavior a configurable alternative.

2. for Remoting 2.4.0, replacing the current behavior with the new behavior.

Now WDYT?

[galder.zamarreno]

Is there any reason to prefer one solution over the other? One could argue that an interruptible EJB or JMS client *should* declare that it throws an InterruptedException, but that discussion might be more theological than technical.

[theological] EJB business interface methods should not have to declare IE just because the underlying library can be interrupted. You could use plain RMI that does not require interface methods to declare InterruptedException but they do need to declare RemoteException. Then again, plain RMI is neither performant and nor flexible, which is why we created Remoting or Pooled Invoker...etc :)

I'm leaning towards wrapping in RuntimeExceptions, which seems more straightforward and avoids the UndeclaredThrowableException problem.

Sounds fine to me :).

There's one more concern. Suppose a Remoting user detected the InterruptedException possibility and their code looks for InterruptedExceptions wrapped in CannotConnectExceptions. Then replacing CannotConnectExceptions with RuntimeExceptions might break their code. I'm leaning towards

1. for Remoting 2.2.2.SP8

a) leaving the current behavior in place as the default behavior, and

b) making the new behavior a configurable alternative.

+1

2. for Remoting 2.4.0, replacing the current behavior with the new behavior.

Now WDYT?

+1 as well.

I might a test on the AS side that replicates this and see how assert that the correct thing is being thrown back to the client and whether the client can make further invocations. I'd probably use some mocking for this.

[galder.zamarreno]

I've created http://jira.jboss.com/jira/browse/JBAS-5484 to track down the creation of some UTs to verify that AS side proxy/interceptors handle this properly. They look Ok now so I might not do them straight away.