SpringBoot closeConnection() should be invoked in the finally block rather than try block

In the org.springframework.boot.actuate.jms.JmsHealthIndicator

its inner class MonitoredConnection

void start() throws JMSException {
    new Thread(() -> {
                try {
                    if (!this.latch.await(5, TimeUnit.SECONDS)) {
                        JmsHealthIndicator.this.logger
                                .warn("Connection failed to start within 5 seconds and will be closed.");
                        -- closeConnection();
                    }
                }
                catch (InterruptedException ex) {
                    Thread.currentThread().interrupt();
                }finally{
                    ++ closeConnection();
                }
            }, "jms-health-indicator").start();
    this.connection.start();
    this.latch.countDown();

Comment From: philwebb

I'm not sure I understand the issue as described, could you please paste the suggested changes in this issue? I believe that the thread in MonitoredConnection should close if connection.start does not respond within 5 seconds.

Comment From: cuixiaoyiyi

Sorry, my redundant presentation is misleading.

current:

void start() throws JMSException {
    new Thread(() -> {
                try {
                    if (!this.latch.await(5, TimeUnit.SECONDS)) {
                        JmsHealthIndicator.this.logger
                                .warn("Connection failed to start within 5 seconds and will be closed.");
                        closeConnection();  // be moved into finally block?
                    }
                }
                catch (InterruptedException ex) {
                    Thread.currentThread().interrupt();
                }
            }, "jms-health-indicator").start();
    this.connection.start();
    this.latch.countDown();

suggested changes:

void start() throws JMSException {
    new Thread(() -> {
                try {
                    if (!this.latch.await(5, TimeUnit.SECONDS)) {
                        JmsHealthIndicator.this.logger
                                .warn("Connection failed to start within 5 seconds and will be closed.");
                    }
                }
                catch (InterruptedException ex) {
                    Thread.currentThread().interrupt();
                }finally{
                       closeConnection();  // be moved into here?
                }

            }, "jms-health-indicator").start();
    this.connection.start();
    this.latch.countDown();

Comment From: bclozel

@cuixiaoyiyi I don't understand the suggested change either. This closeConnection call is meant to try to use close the connection in case it couldn't be started. The connection itself is always closed here in case things work properly.

Could you explain in which case the connection would not be closed properly with the current implementation?

Comment From: cuixiaoyiyi

The suggested changes before may be inappropriate. I mean that if the InterruptedException happens, then the connection will never be closed, and ​the connection resource leak will happen.

The "closeConnection()" should be added in the catch block.

Updated suggested changes:

void start() throws JMSException {
    new Thread(() -> {
                try {
                    if (!this.latch.await(5, TimeUnit.SECONDS)) {
                        JmsHealthIndicator.this.logger
                                .warn("Connection failed to start within 5 seconds and will be closed.");
                        closeConnection(); 
                    }
                }
                catch (InterruptedException ex) {
                    closeConnection();  // be added here?
                    Thread.currentThread().interrupt();
                }

            }, "jms-health-indicator").start();
    this.connection.start();
    this.latch.countDown();

Comment From: bclozel

We're using a try with resources block here - shouldn't that close the connection in all cases?

Comment From: cuixiaoyiyi

I do not think so. The try-with-resources statement works for local variables as a syntactic sugar with format try(local variable){}catch{}.

But it does not work with fields.
Why doesn't try-with-resources work with field variables?

Comment From: bclozel

But it does not work with fields.

This is a local variable, not a field.

try (Connection connection = this.connectionFactory.createConnection()) {
    new MonitoredConnection(connection).start();
    builder.up().withDetail("provider", connection.getMetaData().getJMSProviderName());
}

The link you've shared would apply here if the code looked like

Connection connection = null
try (connection = this.connectionFactory.createConnection()) {
    new MonitoredConnection(connection).start();
    builder.up().withDetail("provider", connection.getMetaData().getJMSProviderName());
}

Comment From: cuixiaoyiyi

The object connection here

try (connection = this.connectionFactory.createConnection()) {
}

will be closed in any case in current thread. If it is necessary to be closed in another thread, I think the closeConnection(); should also be invoked in the catch block for the InterruptedException. If you mean if the InterruptedException happens, then the closeConnection(); needs not to be invoked again, I agree.

Btw, is there a problem with closing the same connection asynchronously in different threads?

Comment From: bclozel

I'm closing this issue as we're entering a theoretical debate. Please reopen this issue if you manage to find cases where we're leaking connections, providing a sample application showing the problem.