Skip to content

Fix potential connection leak when using jdbc storage and Scan operation fails because the target table doesn't exist #2766

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 17, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -186,6 +197,9 @@ public void mutate(List<? extends Mutation> mutations) throws ExecutionException
close(connection);
throw new ExecutionException(
CoreError.JDBC_ERROR_OCCURRED_IN_MUTATION.buildMessage(e.getMessage()), e);
} catch (Exception e) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brfrn169 I took care of this similar potential leak in f18d40f.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

close(connection);
throw e;
}

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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();
}
}