[GitHub] [activemq-artemis] jbertram commented on pull request #2655: ARTEMIS-2327 ExceptionListener invoked when connection level exception

[GitBox <gi...@apache.org>]

jbertram commented on pull request #2655:
URL: https://github.com/apache/activemq-artemis/pull/2655#issuecomment-624786762


   I'm not sure where I was when this PR was originally sent, but I came across these changes while investigating another issue. I'm considering reverting these changes for a handful reasons:
   
   1. As Robbie pointed out originally, this doesn't fit with the JMS specification which clearly indicates that an `ExceptionListener` should *only* be invoked when the exception has nowhere else to go. The main use-case here is when a client implements `MessageListener` (which is invoked asynchronously by the underlying implementation) but still wants to know when its connection dies. Given that, in this situation, the client itself isn't invoking e.g. `javax.jms.Consumer#receive()` it has no way to receive the exception other than through the `ExceptionListener`. I'll quote (again) the salient part section 6.1.7 from the JMS 2 specification:
   
       > The exceptions delivered to `ExceptionListener` are those that have no other place to be reported. If an exception is thrown on a JMS call it, by definition, must not be delivered to an `ExceptionListener` (in other words, **`ExceptionListener` is not for the purpose of monitoring all exceptions thrown by a connection**). [emphasis mine]
   
      As far as I can tell by looking through the code-base `sendBlocking` is only ever called from a JMS client application in response to a specific JMS API call, and it is from these calls that the exception must be thrown to the caller and *not* to the `ExceptionListener`.
   
   2. I'm not convinced by the counter-argument that Spring et al. rely on this. The specification clearly says that the `ExceptionListener` is not a way to monitor all connection exceptions. Therefore, well-written frameworks/wrappers should have other ways to detect or be notified of connection problems. If the wrappers don't implement such functionality I see that as their problem, not ours. We shouldn't violate the JMS specification to suit their deficiencies.
   3. The "ping" functionality was mentioned as the example to follow here. However, I don't think that comparison is apt in this case. The pinging functionality is an implementation specific behavior to monitor the validity of the connection. If a ping fails then by definition the connection has failed. This functionality executes without any action from the client application. Therefore, a ping failure has *nowhere else* to be reported but in an `ExceptionListener`. This is categorically different from a call to `sendBlocking()` which is the result of an explicit JMS API call from the client application.
   4. The underlying issue here is that the client did not receive a response from the broker for a single "packet" within the specified timeout. That's all. There are a number of reasons this situation might arise. Perhaps the broker is under heavy load and simply didn't have time to respond. Perhaps the network is slow for some reason. Perhaps the timeout is simply too low for the use-case. The connection is *not* necessarily dead. It's plausible that the client might, in fact, catch the `JMSException` and retry the failed operation or eventually make another call. The comment on [`o.a.a.a.c.p.c.i.ChannelImpl#sendBlocking(Packet, int, byte)`](https://github.com/apache/activemq-artemis/blob/master/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ChannelImpl.java#L365) even notes this. If the connection actually is dead then the underlying pinger/monitor should detect that.
   5. This change gives the impression to users that exceptions like this which get thrown to a caller should also invoke the `ExceptionListener` which is incorrect per the specification.
   6. I looked through the client code-base for uses of `o.a.a.a.s.c.p.RemotingConnection#fail(o.a.a.a.a.c.ActiveMQException)` aside from the one added via this PR, and I only found 3 others which were all truly asynchronous use-cases where the exception had nowhere else to go.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org