Sonar asks to "Use try-with-resources or close this "Connection" in a "finally" clause."
Asked Answered
M

5

6

I want to have a clean project. So I used Sonar to detect potential defects, ...

On the below method, Sonar asks to : Use try-with-resources or close this "Connection" in a "finally" clause..

private Connection createConnection() throws JMSException {
    MQConnectionFactory mqCF = new MQConnectionFactory();
    ...

    Connection connection = mqCF.createConnection(...);
    connection.start();

    return connection;
}

Can you explain me what I did wrong and how to do to avoid Sonar message? Thank you.

Magness answered 28/1, 2020 at 9:21 Comment(1)
One option would be to use try-with-resourcesLatterly
F
6

Connection implements AutoCloseable which Sonar is detecting (it doesn't care whether you're using a connection or some other auto closeable thingy).

Not closing such a resource could lead to a resource leak so Sonar wants you to do something like this:

//try-with-resources - the connection will be closed after this block
try(Connection connection = mqCF.createConnection(...)) {
  //use connection here
}

//traditional try - the connection will be closed after the finally block
Connection connection = mqCF.createConnection(...);
try {
  //use connection here
} finally {
   connection.close();
}

The problem in your case is that you're just creating the connection and then return it - and I strongly suspect you want to keep it open :)

Sonar can't be sure that you'll ever close the connection so it will complain. There might be ways to make Sonar ignore things like this (I don't use it that much so I'm not too familiar with the options here) but it's still a potential risk that you'd have to mitigate somehow.

Fantinlatour answered 28/1, 2020 at 9:27 Comment(3)
Thank you for your comment. Yes exactly I want to keep it open :), so I guess here's no solution in this caseMagness
And do return connection; in try-with-resources does not solve it?Magness
No, try-with-resources is meant to close the auto closeable resources once you leave that block (which returning would do).Fantinlatour
B
3

In java if you are using resource like FileInptStream, Connection, ResultSet, Input/OutputStream, BufferedReader, PrintWriter you have to close it before garbage collection happens. so basically whenever connection object no longer in use you have to close it.

try below snippet

Connection c = null;
    try {
        c = mqCF.createConnection(...);
        // do something
    } catch(SomeException e) {
        // log exception
    } finally {
      try {
        c.close();
      } catch(IOException e1){
        // log something else
      }
    }

//try-with-resources
try(Connection connection = mqCF.createConnection(...)) {
  //use connection here
}

In the try with resource case connection will automatically close by jvm, but Connection interface must be extends with AutoCloseable / Closable interface.

Burdock answered 28/1, 2020 at 9:39 Comment(0)
B
2

The code you have above has the potential to leave the Connection object open, which can cause significant issues.

You can guarantee that the Connection is closed when you are finished with it in a few ways:

public Connection createConnection() {
    return connectionFactory.createConnection(...);  // note that this method does not open the connection
}

Then, when you want to use the connection, either use try-catch-finally:

try {
    Connection c = createConnection();
    doSomethingWithPossibleException(c);
} catch(PossibleException e) {
    handleException(e);
} finally {
    // now close the connection
    c.close();
}

or you can use try-with-resources (from Java 7 onwards):

try (Connection c = getConnection()) {
    doSomethingWithPossibleException(c);
} catch (PossibleException e) {
    handle(e);
}

IMO try-with-resources is a little more readable, although tastes differ on this matter. Note, the object you create in the try-with-resources must implement the AutoCloseable interface.

Bregma answered 28/1, 2020 at 9:30 Comment(2)
Thank you. However, I don't want to close it because I want to use it later.Magness
You may want to look into "connection pooling". There are a few issues with using a single database connection for the entire lifespan of the application. Pooling can help eliminate those issues. In your code, you treat your connection pool like your current connection factory object, and close the connections when you are done with them.Bregma
H
1

You are creating a connection in a method called createConnection(). Specifically, of a JMS/MQ connection type.

It looks like your class expects the connection to be long-running and reusable. — In classes like this, createConnection() method(s) should be paired with a method to close the opened connection.

The right approach would be: your class should also implement the Closeable or AutoCloseable interface to hint that the caller have the responsibility to call the close method once they are finished using it.

This approach would also have the side effect that the caller of your class will have Sonar (if enabled) warn them to: Use try-with-resources or close this "Connection" in a "finally" clause if they forgot to do so.

Which interface to implement?

In your case, you use MQConnectionFactory which logically will create connections of type MQConnection; and from its javadoc spec, MQConnection implements AutoCloseable. We can copy MQConnection's close() method signature as a reference for our close method, and to match the handled connection type.

So, in your class, it would be something like this:

public class MyMQHelper implements AutoCloseable {

    private MQConnection connection; // result from createConnection() should be set to this variable

    private Connection createConnection() throws JMSException { /* ... */ }

    @Override
    public void close() throws JMSException {
        if (connection != null) connection.close();
    }

}

Sonar warning

The Sonar rule is for general use case using the open-use-close mechanism. When you have specialized use case like this, you can choose to ignore the error further, either by marking it as WONTFIX in SonarQube, or by using suppression hint to block the specific warning (you need to know the Sonar rule id):

private Connection createConnection() throws JMSException {
    MQConnectionFactory mqCF = new MQConnectionFactory();
    ...

    @SuppressWarnings("java:S2095") // by design: Connection is for use later
    Connection connection = mqCF.createConnection(...);
    connection.start();

    return connection;
}

Closing

Having the createConnection() as a private method may signify that your class handles much more than a connection management role, and may be part of a long-running service or job.

However, there should be an orchestrator class whose job is to initialize the required classes before going to the main service or job loop. This is where the (direct/indirect) call to createConnection() should happen and should be paired with the close() call (either explicitly or implicitly using try-with-resources statement).

Hyps answered 5/7, 2024 at 5:7 Comment(0)
C
0

I find out one solution to resolve this issue. you can override the existing DBManger like this:

@Override
    public Connection getConnection() throws SQLException {
        Connection conn = new ProxyConnection(DriverManager.getConnection(...));
        return conn;
    }

    private static class ProxyConnection implements Connection {

        Connection connection;

        private ProxyConnection(Connection connection) {
            this.connection = connection;
        }

        /**
         * Standard method, add logging.
         */
        @Override
        public void close() throws SQLException {
            logger.debug("Connection to database was released");
            connection.close();
        }
    }
Complemental answered 17/12, 2020 at 7:48 Comment(1)
SonarQube will not show you any violation if you use this.Complemental

© 2022 - 2025 — McMap. All rights reserved.