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

[GitBox]

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:
us...@infra.apache.org


With regards,
Apache Git Services