[GitHub] [cxf] reta commented on issue #603: [CXF-8161] fix memory leak and thread leak in JMSDestination

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

reta commented on issue #603: [CXF-8161] fix memory leak and thread leak in JMSDestination
URL: https://github.com/apache/cxf/pull/603#issuecomment-562993379
 
 
   Hi @steingebein, sorry for being quiet for a while, I was trying to understand the issue / solution more profoundly while also committing a number of test cases (in master, will try to backport to 3.3.x / 3.2.x shortly). 
   
   I think the solution we have with shutting down the thread pool from one of its threads (inside `onException` callback) kind of works but it does not look right (I think you would agree). On the faulty path, we always stack at `awaitTermination` ... followed by interruption. 
   
   Now, obviously, the question is what the alternative. I think a more reliable option would be to spin off the thread for `restartConnection`:
   ```
   ExceptionListener exListener = new ExceptionListener() {
                   public void onException(JMSException exception) {
                       if (!shutdown) {
                           LOG.log(Level.WARNING, "Exception on JMS connection. Trying to reconnect", exception);
                           if (jmsListener == null) {
                               restartConnection();
                           } else {
                               new Thread(new Runnable() {
                                   @Override
                                   public void run() {
                                       restartConnection();
                                   }
                               }).start();
                           }
                       }
                   }
               };
   ```
   
   It is definitely far from ideal, but taking the complexity of the `JMSDestination` & related components, it looks like an acceptable tradeoff. Yes, we may create a number of short-lived threads at peak, but only one would be doing useful work in fact. With this approach, we eliminate self-blocking at a cost of wasting more resources. It also means we should not call `stop` in `onException` but let `JMSDestination` to clean things up (it does inside the `deactivate`).
   
   This is obviously a patch, not a proper solution, the thoughtful redesign would make more sense in the long run. What do you think? Thanks for your patience.

----------------------------------------------------------------
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


With regards,
Apache Git Services