SpringBoot JmsHealthIndicator doesn't work properly for ActiveMQ failover transport

For failover transport:

spring.activemq.broker-url: failover:(tcp://localhost:61616,tcp://127.0.0.1:61616)?randomize=false&nested.daemon=true

when ActiveMQ become unavailable JmsHealthIndicator reports UP. Our solution for this issue is to use TransportListener. See http://activemq.apache.org/how-can-i-monitor-the-connection-with-the-broker.html

public class TransportInteruptionListener extends DefaultTransportListener {
    private boolean interupted;

    @Override
    public void transportInterupted() {
         interupted = true;
    }
    @Override
    public void transportResumed() {
         interupted = false;
    }

    public boolean isInterupted() {
        return interupted;
    }
}

public class CustomJmsHealthIndicatorAutoConfiguration {

    @Bean
    public TransportInteruptionListener daivbTransportInteruptionListener() {
        return new TransportInteruptionListener();
    }

    @Bean
    @ConditionalOnMissingBean(name = "customJmsHealthIndicator")
    customJmsHealthIndicator customJmsHealthIndicator(ApplicationContext ctx, ConnectionFactory connectionFactory,
            TransportInteruptionListener l) {
        return new CustomJmsHealthIndicator(ctx, connectionFactory, l);
    }


    @Bean
    public ActiveMQConnectionFactoryCustomizer customActiveMQConnectionFactoryCustomizer(TransportInteruptionListener l) {
        return factory -> factory.setTransportListener(l);
    }
}

public class CustomJmsHealthIndicator extends AbstractHealthIndicator {
    ...
    protected void doHealthCheck(Health.Builder builder) throws Exception {
         ...
         connection.start();
          if (transportInteruptionListener.isInterupted()) {
             throw new IllegalStateException("Transport interupted");
         }
         ...
}

**Comment From: philwebb**

Thanks for the suggestion. Currently `JmsHealthIndicator` only uses `javax.jms` package imports so I think we'll need an enhanced ActiveMQ specific version.

**Comment From: ashastr**

I would like to work on this enhancement. Please let  me know if I can contribute.


**Comment From: wilkinsona**

Yes please, @anandshastri1990. Thanks very much for the offer. Please let us know if you have any questions.

**Comment From: ashastr**

hi @wilkinsona , 
I am planning to add,
- a new health indicator called ActiveMQHealthIndicator which uses ActiveMQDefaultTransportListener under package: 'org.springframework.boot.actuate.jms.activemq' 
- a new auto-configuration ActiveMQHealthIndicatorAutoConfiguration under package: 'org.springframework.boot.actuate.autoconfigure.jms.activemq'
- 'activemq-broker' dependency to spring-boot-actuator pom.xml

Please let me know if this would be the right design to go about it.

**Comment From: wilkinsona**

That sounds like a good direction to me. Thanks.

One thing we'll have to think about is what happens to the generic JMS health indicator when the ActiveMQ-specific one is being used. My first thought is that the generic one should probably back off. Perhaps the two health indicators should share a common base class or implement the same interface. We can figure out how to do that once the ActiveMQ-specific implementation has been written and we have some code to look at.

**Comment From: ashastr**

I understand. For now, I have just added the files as per the design I had shared. Below is the commit on the forked repo for your review. Please let me know what you are thoughts are on refactoring this for better implementation.

https://github.com/anandshastri1990/spring-boot/commit/76195bd5bd76e3e3eb51b6f4c7701517a7b901fa


**Comment From: ashastr**

@wilkinsona forgot to tag you in the earlier comment.

**Comment From: wilkinsona**

@anandshah123 What you've got looks pretty good to me. Thank you. I think we'll need to try and test this before we could merge the changes. Would you like to take a look at that and then open a pull request? Hopefully it'll be possible to start an appropriately configured broker, check that the health indicator reports up, stop the broker, and then check that the indicator now reports down.

**Comment From: ashastr**

@wilkinsona In this latest push https://github.com/anandshastri1990/spring-boot/commit/b6c52098004eebdc3d2bf667ac1e91593aea4ba7 I have made changes to create only ActiveMqHealthIndicator if ActiveMq has been configured and JmsHealthIndicator would back off. I have added some unit tests as well. I was able to do some testing, and the latest changes to seems to work fine. 

However I think I found something interesting! When I used the master branch to perform the same test, JmsHealthIndicator reported the status as down when the broken was not active. Looks like every time we perform a health check JmsHealthIndicator tries to connect to the broker, since the broker was not active, it reported the status as down. (`Connection connection = this.connectionFactory.createConnection()`) Now I am not sure if there is a need to fix this issue at all! 

**Comment From: wilkinsona**

@anandshastri1990 Thanks for the update and sorry for the possibly wasted effort. Which version of ActiveMQ did you test against?

@tjuchniewicz Can you please confirm the versions of Spring Boot and ActiveMQ with which you observed the health indicator reporting the wrong status and the precise steps required to reproduce it?

**Comment From: ashastr**

@wilkinsona I had ActiveMQ 5.11.1 broker running for test. No worries about the effort, I got to learn few things :)

**Comment From: spring-projects-issues**

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.


**Comment From: spring-projects-issues**

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

**Comment From: MarcoFierimonte**

Any update about this issue? There is some standard solution for it? Thanks.

**Comment From: snicoll**

@MarcoFierimonte reading through the history of this issue it looks like the original issue has been fixed by a more recent version of Spring Boot. If you're still affected with a recent version of Spring Boot, please attach a small sample that helps us reproduce the problem and we can take another look.

**Comment From: MarcoFierimonte**

thanks @snicoll .
Yes I have the same error using Spring Boot version 2.2.6.RELEASE (the same for Spring Boot ActiveMQ starter).
To manage the jms connection pool I use the library: **org.messaginghub-->pooled-jms-->1.1.1** 

The Spring Boot Actuator configuration is working, and as expected it returns me the JMS status **UP** at rest endpoint: http://localhost:8080/actuator/health. After I removed the activemq cluster completely from the k8s cluster the application throw the WARN logs:
`2020/11/23 14:40:44.986 WARN  [ActiveMQ Task-5] |  | o.a.a.t.f.FailoverTransport | Failed to connect to [tcp://internal-broker-0.internal-broker:61616, tcp://internal-broker-1.internal-broker:61616, tcp://internal-broker-2.internal-broker:61616] after: 1530 attempt(s) continuing to retry.`
**but the Actuator JMS Health is still UP** 
This with the following broker url connection:

failover:(\ tcp://internal-broker-0.internal-broker:61616,\ tcp://internal-broker-1.internal-broker:61616,\ tcp://internal-broker-2.internal-broker:61616)?\ backup=true&maxReconnectAttempts=-1&maxReconnectDelay=5000&startupMaxReconnectAttempts=10 ``` Instead if I set up maxReconnectAttempts to a value different from -1 the Health rest endpoint correctly throw a DOWN result but after the broker restore the application can't connect anymore to it because it ends the maxReconnectAttempts.

Any idea to solve this? I'd like have the application DOWN for the Spring Boot Actuator health framework with infinite maxReconnectAttempts to be able to reconnect to the restored broker.

Thanks. Marco

Comment From: snicoll

I don't think that's strictly the same issue (rather the same title). If the actuator provides the expected outcome with any other value that -1, I think this is something you should be raising with ActiveMQ. If you do so, feel free to add a link here so that we can follow along.

Comment From: MarcoFierimonte

Thanks for helping @snicoll . What do you mean for "I think this is something you should be raising with ActiveMQ", sorry I don't understand. I thought that if there's no connection with the broker (because I remove it completely from the cluster) the Actuator should return an error result independently from the maxReconnectAttempts value.

Comment From: snicoll

the Actuator should return an error result independently from the maxReconnectAttempts value.

We're merely trying to create to connect and check the health of the connection. If ActiveMQ provides the right answer when maxReconnectAttempts is not equal to -1 but doesn't when it's equal to -1 , I'd argue that might a problem in the client and nothing we can do about it.

Comment From: MarcoFierimonte

ok thanks for clarifications...I'll try to better configure the client to solve this issue.