From b4d19397beec60a192b549e620952bd041879b1f Mon Sep 17 00:00:00 2001 From: Mitsunori Komatsu Date: Tue, 17 Jun 2025 13:19:31 +0900 Subject: [PATCH] Fix potential connection leak when using `jdbc` storage and Scan operation fails because the target table doesn't exist (#2766) Co-authored-by: Hiroyuki Yamada --- .../scalar/db/storage/jdbc/JdbcDatabase.java | 14 ++++++ .../db/storage/jdbc/JdbcDatabaseTest.java | 47 +++++++++++++++++++ 2 files changed, 61 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 64a20cc725..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 @@ -117,6 +117,17 @@ 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) { + try { + if (connection != null) { + connection.rollback(); + } + } catch (SQLException ex) { + e.addSuppressed(ex); + } + + close(connection); + throw e; } } @@ -186,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 3bcf119d2b..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; @@ -138,6 +140,25 @@ public void whenScanOperationExecutedAndScannerClosed_shouldCallJdbcService() th verify(connection).close(); } + @Test + public void + whenScanOperationExecutedAndJdbcServiceThrowsIllegalArgumentException_shouldCloseConnectionAndThrowIllegalArgumentException() + 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(IllegalArgumentException.class); + verify(connection).close(); + } + @Test public void whenScanOperationExecutedAndScannerClosed_SQLExceptionThrownByConnectionCommit_shouldThrowIOException() @@ -382,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(); + } }