From 4bb2e3af45aeb7e279bf719a3db5fad8ec4579a7 Mon Sep 17 00:00:00 2001 From: Mitsunori Komatsu Date: Fri, 13 Jun 2025 16:47:29 +0900 Subject: [PATCH 1/4] Fix DB connection leak when the target table is not found --- .../scalar/db/storage/jdbc/JdbcDatabase.java | 2 +- .../db/storage/jdbc/JdbcDatabaseTest.java | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java index 64a20cc725..26d7386aa7 100644 --- a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java +++ b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java @@ -105,7 +105,7 @@ public Scanner scan(Scan scan) throws ExecutionException { connection.setAutoCommit(false); rdbEngine.setConnectionToReadOnly(connection, true); return jdbcService.getScanner(scan, connection); - } catch (SQLException e) { + } catch (Exception e) { try { if (connection != null) { connection.rollback(); diff --git a/core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java b/core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java index 3bcf119d2b..e877489087 100644 --- a/core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java +++ b/core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java @@ -138,6 +138,29 @@ public void whenScanOperationExecutedAndScannerClosed_shouldCallJdbcService() th verify(connection).close(); } + @Test + public void + whenScanOperationExecutedAndJdbcServiceThrowsIllegalArgumentException_shouldThrowExecutionException() + throws Exception { + // Arrange + Exception cause = new IllegalArgumentException("Table not found"); + // Simulate the table not found scenario. + when(jdbcService.getScanner(any(), any())).thenThrow(cause); + + // Act Assert + assertThatThrownBy( + () -> { + Scan scan = new Scan(new Key("p1", "val")).forNamespace(NAMESPACE).forTable(TABLE); + jdbcDatabase.scan(scan); + }) + .isInstanceOf(ExecutionException.class) + .hasCause(cause); + verify(connection).setAutoCommit(false); + verify(connection).setReadOnly(true); + verify(connection).rollback(); + verify(connection).close(); + } + @Test public void whenScanOperationExecutedAndScannerClosed_SQLExceptionThrownByConnectionCommit_shouldThrowIOException() From 60d4fbaa933e7e6329586b09701a8d9ce20e0332 Mon Sep 17 00:00:00 2001 From: Mitsunori Komatsu Date: Fri, 13 Jun 2025 18:22:54 +0900 Subject: [PATCH 2/4] Only close connection when other exception occurs --- .../java/com/scalar/db/storage/jdbc/JdbcDatabase.java | 7 ++++++- .../java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java | 8 ++------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java index 26d7386aa7..c74ccc47a0 100644 --- a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java +++ b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java @@ -105,7 +105,7 @@ public Scanner scan(Scan scan) throws ExecutionException { connection.setAutoCommit(false); rdbEngine.setConnectionToReadOnly(connection, true); return jdbcService.getScanner(scan, connection); - } catch (Exception e) { + } catch (SQLException e) { try { if (connection != null) { connection.rollback(); @@ -117,6 +117,11 @@ public Scanner scan(Scan scan) throws ExecutionException { close(connection); throw new ExecutionException( CoreError.JDBC_ERROR_OCCURRED_IN_SELECTION.buildMessage(e.getMessage()), e); + } catch (Exception e) { + if (connection != null) { + close(connection); + } + throw e; } } diff --git a/core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java b/core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java index e877489087..dfe784ad7d 100644 --- a/core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java +++ b/core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java @@ -140,7 +140,7 @@ public void whenScanOperationExecutedAndScannerClosed_shouldCallJdbcService() th @Test public void - whenScanOperationExecutedAndJdbcServiceThrowsIllegalArgumentException_shouldThrowExecutionException() + whenScanOperationExecutedAndJdbcServiceThrowsIllegalArgumentException_shouldCloseConnectionAndThrowIllegalArgumentException() throws Exception { // Arrange Exception cause = new IllegalArgumentException("Table not found"); @@ -153,11 +153,7 @@ public void whenScanOperationExecutedAndScannerClosed_shouldCallJdbcService() th Scan scan = new Scan(new Key("p1", "val")).forNamespace(NAMESPACE).forTable(TABLE); jdbcDatabase.scan(scan); }) - .isInstanceOf(ExecutionException.class) - .hasCause(cause); - verify(connection).setAutoCommit(false); - verify(connection).setReadOnly(true); - verify(connection).rollback(); + .isInstanceOf(IllegalArgumentException.class); verify(connection).close(); } From e96888e673ea7f9aea35c6622e4937a2b448bf61 Mon Sep 17 00:00:00 2001 From: Mitsunori Komatsu Date: Mon, 16 Jun 2025 13:51:21 +0900 Subject: [PATCH 3/4] Improve error handling based on feedback --- .../java/com/scalar/db/storage/jdbc/JdbcDatabase.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java index c74ccc47a0..d8a04da366 100644 --- a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java +++ b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java @@ -118,9 +118,15 @@ public Scanner scan(Scan scan) throws ExecutionException { throw new ExecutionException( CoreError.JDBC_ERROR_OCCURRED_IN_SELECTION.buildMessage(e.getMessage()), e); } catch (Exception e) { - if (connection != null) { - close(connection); + try { + if (connection != null) { + connection.rollback(); + } + } catch (SQLException ex) { + e.addSuppressed(ex); } + + close(connection); throw e; } } From f18d40f6765a0a61b6e73c521d6967d0551ca830 Mon Sep 17 00:00:00 2001 From: Mitsunori Komatsu Date: Mon, 16 Jun 2025 16:31:49 +0900 Subject: [PATCH 4/4] Take care of similar issue --- .../scalar/db/storage/jdbc/JdbcDatabase.java | 3 ++ .../db/storage/jdbc/JdbcDatabaseTest.java | 28 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java index d8a04da366..e238a1aa28 100644 --- a/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java +++ b/core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java @@ -197,6 +197,9 @@ public void mutate(List mutations) throws ExecutionException close(connection); throw new ExecutionException( CoreError.JDBC_ERROR_OCCURRED_IN_MUTATION.buildMessage(e.getMessage()), e); + } catch (Exception e) { + close(connection); + throw e; } try { diff --git a/core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java b/core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java index dfe784ad7d..62d217d132 100644 --- a/core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java +++ b/core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java @@ -2,7 +2,9 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -401,4 +403,30 @@ public void mutate_withConflictError_shouldThrowRetriableExecutionException() verify(connection).rollback(); verify(connection).close(); } + + @Test + public void mutate_WhenSettingAutoCommitFails_ShouldThrowExceptionAndCloseConnection() + throws SQLException, ExecutionException { + // Arrange + Exception exception = new RuntimeException("Failed to set auto-commit"); + doThrow(exception).when(connection).setAutoCommit(anyBoolean()); + + // Act Assert + assertThatThrownBy( + () -> { + Put put = + new Put(new Key("p1", "val1")) + .withValue("v1", "val2") + .forNamespace(NAMESPACE) + .forTable(TABLE); + Delete delete = + new Delete(new Key("p1", "val1")).forNamespace(NAMESPACE).forTable(TABLE); + jdbcDatabase.mutate(Arrays.asList(put, delete)); + }) + .isEqualTo(exception); + verify(connection).setAutoCommit(false); + verify(jdbcService, never()).mutate(any(), any()); + verify(connection, never()).rollback(); + verify(connection).close(); + } }