Skip to content

Commit

Permalink
Handle: unconditionally close Connection
Browse files Browse the repository at this point in the history
Right now, we check if a connection is closed before closing it.

However, this isn't good enough: even if the server closed the connection,
we might have borrowed this from a pool, which needs to know we've returned it.

Connection.close is specified to no-op if called more than once, so it's safe to just do it regardless.

Fixes #2446
  • Loading branch information
stevenschlansker committed Aug 3, 2023
1 parent 299268b commit 338738b
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 40 deletions.
1 change: 1 addition & 0 deletions core/src/main/java/org/jdbi/v3/core/ConnectionFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ default void closeConnection(Connection conn) throws SQLException {

/**
* Returns a {@link Cleanable} that will close the connection and release all necessary resources.
* Like {@link Connection#close()}, the Cleanable should no-op if called more than once.
*
* @param conn A {@link Connection} object.
* @return A {@link Cleanable} instance. Calling the {@link Cleanable#close()} method will close the connection and release all resources.
Expand Down
67 changes: 27 additions & 40 deletions core/src/main/java/org/jdbi/v3/core/Handle.java
Original file line number Diff line number Diff line change
Expand Up @@ -115,12 +115,8 @@ private Handle(Jdbi jdbi,
this.transactionHandler = transactionHandler.specialize(this);
this.forceEndTransactions = !transactionHandler.isInTransaction(this);

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

/**
Expand Down Expand Up @@ -897,52 +893,43 @@ private void notifyHandleClosed() {
private void cleanConnection(boolean doForceEndTransactions) {

final ThrowableSuppressor throwableSuppressor = new ThrowableSuppressor();
final boolean connectionIsLive = checkConnectionIsLive();

boolean wasInTransaction = false;

if (connectionIsLive && doForceEndTransactions) {
wasInTransaction = throwableSuppressor.suppressAppend(this::isInTransaction, false);
if (doForceEndTransactions) {
wasInTransaction = throwableSuppressor.suppressAppend(() -> {
if (connection.isClosed()) {
return false;
}
return isInTransaction();
}, false);
}

if (wasInTransaction) {
throwableSuppressor.suppressAppend(this::rollback);
}

if (connectionIsLive) {
try {
connectionCleaner.close();
} catch (SQLException e) {
CloseException ce = new CloseException("Unable to close Connection", e);
throwableSuppressor.attachToThrowable(ce);
throw ce;
}

throwableSuppressor.throwIfNecessary(t -> new CloseException("Failed to clear transaction status on close", t));

if (wasInTransaction) {
TransactionException te = new TransactionException("Improper transaction handling detected: A Handle with an open "
+ "transaction was closed. Transactions must be explicitly committed or rolled back "
+ "before closing the Handle. "
+ "Jdbi has rolled back this transaction automatically. "
+ "This check may be disabled by calling getConfig(Handles.class).setForceEndTransactions(false).");

throwableSuppressor.attachToThrowable(te); // any exception present is not the cause but just collateral.
throw te;
}
} else {
throwableSuppressor.throwIfNecessary(t -> new CloseException("Failed to clear transaction status on close", t));
}
}

private boolean checkConnectionIsLive() {
try {
return !connection.isClosed();
connectionCleaner.close();
} catch (SQLException e) {
// if the connection state can not be determined, assume that the
// connection is closed and ignore the exception
return false;
CloseException ce = new CloseException("Unable to close Connection", e);
throwableSuppressor.attachToThrowable(ce);
throw ce;
}

throwableSuppressor.throwIfNecessary(t -> new CloseException("Failed to clear transaction status on close", t));

if (wasInTransaction) {
TransactionException te = new TransactionException("Improper transaction handling detected: A Handle with an open "
+ "transaction was closed. Transactions must be explicitly committed or rolled back "
+ "before closing the Handle. "
+ "Jdbi has rolled back this transaction automatically. "
+ "This check may be disabled by calling getConfig(Handles.class).setForceEndTransactions(false).");

throwableSuppressor.attachToThrowable(te); // any exception present is not the cause but just collateral.
throw te;
}
throwableSuppressor.throwIfNecessary(t -> new CloseException("Failed to clear transaction status on close", t));
}

@Override
Expand Down

0 comments on commit 338738b

Please sign in to comment.