[Bug] Jdbi3-core - connection object can be stuck in a connection pool forever if connection was closed.

[kristoffSC]

Hi,
it seems that jdbi3-core might have a code path that can make broken connection objects stuck in the connection pool forever in "ALLOCATED" state. This can lead to max out the connection pool capacity. This is what most likely have happen in our system.

I have created a reproducer for this issue with detailed instructions - here

In Handle's constructor there is a check, that verifies if used connection for this handle is live. The Cleanable is added to the created Handle instance ONLY if connection is Live.

addCleanable(() -> {
            // shut down statement builder
            if (checkConnectionIsLive()) {
                statementBuilder.close(connection);
            }
        });

If the connection was closed or had any connection issue (client or server side) after it was borrowed from the pool and before this check, then no Cleanable will be registered making this connection object never be returned to the pool.

###### UPDATE ######
After deeper analysis it seems that problem is more complex.

The generalization is that if there will be an exception thrown anywhere in org.jdbi.v3.core.Jdbi::open() method, after the connection was acquired from pool, then the connection object will not be returned to the pool.

I would say that org.jdbi.v3.core.Jdbi::open() should have exception handling logic which will guaranty that connection object will be properly returned to the pool in case of exception. Currently it is not the case.

Specific use cases:

1. The connection is closed from the client side, as described in this bug report.
   In this case it turns out that exception will be thrown by this line in Handle's constructor.
   this.transactionHandler = transactionHandler.specialize(this); so even before registering any Cleanable objects

In this case, since exception was thrown from Handle constructor, there is no Handle instance to close/clean at the end.

2. The second use case is where the connection is closed from server side, for example Data Base restart.
   In our system we are using Jdbi's PostgresPlugin. In org.jdbi.v3.core.Jdbi::open() method, after Handle object is created there is this call:

for (JdbiPlugin p : plugins) {
        h = p.customizeHandle(h);
}

This will throw an exception for PostgresPlugin.
In result the open() method will not return any Handle reference so no Handle will be closed/clean up hence connection object will not be returned to the pool. The stack trace for this case -
JdbiTrace.txt

To reproduce this, please pull my repository (I added PostgresPlugin) and simply restart Postgresql container on a break point just after where connection object is acquired from the pool.

The PoC patch for this could be something like this:

public Handle open() {
    Connection conn = null;
    Handle h = null;
    try {
      final long start = System.nanoTime();
      conn = Objects.requireNonNull(connectionFactory.openConnection(),
          () -> "Connection factory " + connectionFactory + " returned a null connection");
      final long stop = System.nanoTime();

      for (JdbiPlugin p : plugins) {
        conn = p.customizeConnection(conn);
      }

      StatementBuilder cache = statementBuilderFactory.get().createStatementBuilder(conn);

      h = Handle.createHandle(this,
          connectionFactory.getCleanableFor(conn), // don't use conn::close, the cleanup must be done by the connection factory!
          transactionhandler.get(),
          cache,
          conn);

      for (JdbiPlugin p : plugins) {
        h = p.customizeHandle(h);
      }
      LOG.trace("Jdbi [{}] obtain handle [{}] in {}ms", this, h, MILLISECONDS.convert(stop - start, NANOSECONDS));
      return h;
    } catch (Exception e) {
     // NEW EXCEPTION HANDLING TO GUARANTEE THAT CONNECTION OBJECT IS WILL BE RETURNED TO THE POOL.
      if (h != null) {
        // close Handle if we have one in case of Exception, connection will be clean up by this.
        h.close();
      } else if (conn != null) {
        try {
         // clean up a connection if we have one since exception must have been thrown before Handle instance was created
          this.connectionFactory.closeConnection(conn);
        } catch (SQLException ex) {
          throw new ConnectionException(e);
        }
      } if (e instanceof SQLException) { // not sure about this part, whatever Exception type we would like to have here
        throw new ConnectionException(e);
      }
      throw new RuntimeException(e);
    }
  }

[stevenschlansker]

Thank you for the very detailed bug report. We'll take a look at this as soon as we can.

[stevenschlansker]

Hi @kristoffSC , I have proposed a change that closes the connection without first checking to see if it is already closed in #2451. Could you see if this fixes the problem for you?

[kristoffSC]

Hi @kristoffSC , I have proposed a change that closes the connection without first checking to see if it is already closed in #2451. Could you see if this fixes the problem for you?

Hi Thanks for looking at this issue.
I will re-test it as soon as I can. I'm on vac to the end of this week with limited access to the Internet.

[kristoffSC]

Hi @stevenschlansker
I've test your patch and it seems that issue is still there. I did further analysis and it seems that problem is more complex.

The generalization is that if there will be an exception thrown anywhere in org.jdbi.v3.core.Jdbi::open() method, after the connection was acquired from pool, then the connection object will not be returned to the pool.

I would say that org.jdbi.v3.core.Jdbi::open() should have exception handling logic which will guaranty that connection object will be properly returned to the pool in case of exception. Currently it is not the case.

Specific use cases:

1. The connection is closed from the client side, as described in this bug report.
   In this case it turns out that exception will be thrown by this line in Handle's constructor.
   this.transactionHandler = transactionHandler.specialize(this); so even before registering any Cleanable objects

In this case, since exception was thrown from Handle constructor, there is no Handle instance to close/clean at the end.

2. The second use case is where the connection is closed from server side, for example Data Base restart.
   In our system we are using Jdbi's PostgresPlugin. In org.jdbi.v3.core.Jdbi::open() method, after Handle object is created there is this call:

for (JdbiPlugin p : plugins) {
        h = p.customizeHandle(h);
}

This will throw an exception for PostgresPlugin.
In result the open() method will not return any Handle reference so no Handle will be closed/clean up hence connection object will not be returned to the pool. The stack trace for this case -
JdbiTrace.txt

To reproduce this, please pull my repository (I added PostgresPlugin) and simply restart Postgresql container on a break point just after where connection object is acquired from the pool.

The PoC patch for this could be something like this:

public Handle open() {
    Connection conn = null;
    Handle h = null;
    try {
      final long start = System.nanoTime();
      conn = Objects.requireNonNull(connectionFactory.openConnection(),
          () -> "Connection factory " + connectionFactory + " returned a null connection");
      final long stop = System.nanoTime();

      for (JdbiPlugin p : plugins) {
        conn = p.customizeConnection(conn);
      }

      StatementBuilder cache = statementBuilderFactory.get().createStatementBuilder(conn);

      h = Handle.createHandle(this,
          connectionFactory.getCleanableFor(conn), // don't use conn::close, the cleanup must be done by the connection factory!
          transactionhandler.get(),
          cache,
          conn);

      for (JdbiPlugin p : plugins) {
        h = p.customizeHandle(h);
      }
      LOG.trace("Jdbi [{}] obtain handle [{}] in {}ms", this, h, MILLISECONDS.convert(stop - start, NANOSECONDS));
      return h;
    } catch (Exception e) {
     // NEW EXCEPTION HANDLING TO GUARANTEE THAT CONNECTION OBJECT WILL BE RETURNED TO THE POOL.
      if (h != null) {
        // close Handle if we have one in case of Exception, connection will be clean up by this.
        h.close();
      } else if (conn != null) {
        try {
         // clean up a connection if we have one since exception must have been thrown before Handle instance was created
          this.connectionFactory.closeConnection(conn);
        } catch (SQLException ex) {
          throw new ConnectionException(e);
        }
      } if (e instanceof SQLException) { // not sure about this part, whatever Exception type we would like to have here
        throw new ConnectionException(e);
      }
      throw new RuntimeException(e);
    }
  }

[stevenschlansker]

OK, I agree if a customizeHandle call or other point throws, we have to close resources. I updated the PR. Would you mind trying again @kristoffSC ?

[kristoffSC]

The issue has been fixed by #2458

Tested using reproducer from this bug report.
Thanks.

[hgschmie]

I cut a 3.41.0-rc1 release which should hit central soon. As this bug fix is a major part of this release, please test with the 3.41.0-rc1 and lmk if that works for you.

[kristoffSC]

I cut a 3.41.0-rc1 release which should hit central soon. As this bug fix is a major part of this release, please test with the 3.41.0-rc1 and lmk if that works for you.

Hi,
I've check with 3.41.0-rc1, looking good, thx.