From a99ec5ed517b2f2374a73f0c6e9298c8d8f17802 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Fri, 8 Apr 2022 15:24:43 +0100 Subject: [PATCH 1/3] Fix a race condition when starting a transaction during shutdown. --- .../main/java/org/briarproject/bramble/db/JdbcDatabase.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java index 3ae8f7255..539304474 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java @@ -582,7 +582,13 @@ abstract class JdbcDatabase implements Database { txn.setAutoCommit(false); connectionsLock.lock(); try { + // The DB may have been closed since the check above + if (closed) { + tryToClose(txn, LOG, WARNING); + throw new DbClosedException(); + } openConnections++; + connectionsChanged.signalAll(); } finally { connectionsLock.unlock(); } From 9304a6b2666ced14900e7aa942b114dfa7cabe0a Mon Sep 17 00:00:00 2001 From: akwizgran Date: Fri, 8 Apr 2022 15:37:02 +0100 Subject: [PATCH 2/3] Continue with closing connections if an exception is thrown. --- .../main/java/org/briarproject/bramble/db/JdbcDatabase.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java index 539304474..94b0c13d8 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java @@ -641,12 +641,12 @@ abstract class JdbcDatabase implements Database { } } - void closeAllConnections() throws SQLException { + void closeAllConnections() { boolean interrupted = false; connectionsLock.lock(); try { closed = true; - for (Connection c : connections) c.close(); + for (Connection c : connections) tryToClose(c, LOG, WARNING); openConnections -= connections.size(); connections.clear(); while (openConnections > 0) { @@ -656,7 +656,7 @@ abstract class JdbcDatabase implements Database { LOG.warning("Interrupted while closing connections"); interrupted = true; } - for (Connection c : connections) c.close(); + for (Connection c : connections) tryToClose(c, LOG, WARNING); openConnections -= connections.size(); connections.clear(); } From 5d952ff68e4f2f6dc534339bd62c9eb6bfcff8b8 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Fri, 8 Apr 2022 15:49:43 +0100 Subject: [PATCH 3/3] Don't return connections to the pool if they've thrown exceptions. --- .../briarproject/bramble/db/JdbcDatabase.java | 57 +++++++++++++------ 1 file changed, 40 insertions(+), 17 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java index 94b0c13d8..0dfe41bd9 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java @@ -70,6 +70,7 @@ import static java.sql.Types.BOOLEAN; import static java.sql.Types.INTEGER; import static java.sql.Types.VARCHAR; import static java.util.Arrays.asList; +import static java.util.logging.Level.FINE; import static java.util.logging.Level.INFO; import static java.util.logging.Level.WARNING; import static java.util.logging.Logger.getLogger; @@ -572,6 +573,7 @@ abstract class JdbcDatabase implements Database { try { if (closed) throw new DbClosedException(); txn = connections.poll(); + logConnectionCounts(); } finally { connectionsLock.unlock(); } @@ -588,6 +590,7 @@ abstract class JdbcDatabase implements Database { throw new DbClosedException(); } openConnections++; + logConnectionCounts(); connectionsChanged.signalAll(); } finally { connectionsLock.unlock(); @@ -599,42 +602,57 @@ abstract class JdbcDatabase implements Database { return txn; } + @GuardedBy("connectionsLock") + private void logConnectionCounts() { + if (LOG.isLoggable(FINE)) { + LOG.fine(openConnections + " connections open, " + + connections.size() + " in pool"); + } + } + @Override public void abortTransaction(Connection txn) { + // The transaction may have been aborted due to an earlier exception, + // so close the connection rather than returning it to the pool try { txn.rollback(); - connectionsLock.lock(); - try { - connections.add(txn); - connectionsChanged.signalAll(); - } finally { - connectionsLock.unlock(); - } } catch (SQLException e) { - // Try to close the connection logException(LOG, WARNING, e); - tryToClose(txn, LOG, WARNING); - // Whatever happens, allow the database to close - connectionsLock.lock(); - try { - openConnections--; - connectionsChanged.signalAll(); - } finally { - connectionsLock.unlock(); - } + } + closeConnection(txn); + } + + private void closeConnection(Connection txn) { + tryToClose(txn, LOG, WARNING); + connectionsLock.lock(); + try { + openConnections--; + logConnectionCounts(); + connectionsChanged.signalAll(); + } finally { + connectionsLock.unlock(); } } @Override public void commitTransaction(Connection txn) throws DbException { + // If the transaction commits successfully then return the connection + // to the pool, otherwise close it try { txn.commit(); + returnConnectionToPool(txn); } catch (SQLException e) { + logException(LOG, WARNING, e); + closeConnection(txn); throw new DbException(e); } + } + + private void returnConnectionToPool(Connection txn) { connectionsLock.lock(); try { connections.add(txn); + logConnectionCounts(); connectionsChanged.signalAll(); } finally { connectionsLock.unlock(); @@ -650,6 +668,10 @@ abstract class JdbcDatabase implements Database { openConnections -= connections.size(); connections.clear(); while (openConnections > 0) { + if (LOG.isLoggable(INFO)) { + LOG.info("Waiting for " + openConnections + + " connections to be closed"); + } try { connectionsChanged.await(); } catch (InterruptedException e) { @@ -660,6 +682,7 @@ abstract class JdbcDatabase implements Database { openConnections -= connections.size(); connections.clear(); } + LOG.info("All connections closed"); } finally { connectionsLock.unlock(); }