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

komamitsu · 2025-06-13T07:54:33Z

Description

I found the current com.scalar.db.storage.jdbc.JdbcDatabase#scan doesn't close the connection when the target table doesn't exist. This PR fixes the issue.

Related issues and/or PRs

None

Changes made

Close the connection for Scan operation in JdbcDatabase.scan() whenever an exception is thrown not only when SQLException is thrown, since IllegalArgumentException can be thrown when the target table doesn't exist

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes.
I have considered whether similar issues could occur in other products, components, or modules if this PR is for bug fixes.
Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
Tests (unit, integration, etc.) have been added for the changes.
My changes generate no new warnings.
Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

None

Release notes

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

when the target table is not found

Copilot

Pull Request Overview

This PR fixes a potential connection leak in the JDBC storage layer when a Scan operation fails due to a missing target table. The changes ensure the connection is rolled back and closed regardless of the exception type.

Update JdbcDatabase.scan() to catch Exception instead of only SQLException.
Add a new test case in JdbcDatabaseTest.java to verify that an IllegalArgumentException thrown by the JDBC service results in an ExecutionException and proper cleanup.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
core/src/test/java/com/scalar/db/storage/jdbc/JdbcDatabaseTest.java	Added test case to simulate table-not-found scenario and verify connection rollback and closure.
core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java	Modified exception handling in the scan method to catch a broader range of exceptions.

Comments suppressed due to low confidence (1)

core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java:108

Catching a broad Exception may inadvertently hide unexpected errors. Consider catching only the specific exceptions you intend to handle, or add logging to clearly indicate that an unexpected exception was caught.

} catch (Exception e) {

komamitsu · 2025-06-13T09:12:56Z

~~The current fix changes the bahavior a bit. Let me update this PR.~~

Fixed it.

Torch3333

LGTM, thank you!

brfrn169

Good catch! Left a few comments. Please take a look when you have time!

brfrn169 · 2025-06-16T04:05:46Z

core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java

+      if (connection != null) {
+        close(connection);
+      }


The null handling is already done in the close() method, so we don't need to do that here:

Suggested change

if (connection != null) {

close(connection);

}

close(connection);

brfrn169 · 2025-06-16T04:07:56Z

core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java

@@ -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) {


Don't we need to connection.rollback() here?

Thanks! I focused on the connection leak, but I think the transaction should be rolled-back immediately to avoid potential future lazy-recovery. Addressed it in e96888e.

brfrn169

LGTM! Thank you!

komamitsu · 2025-06-16T07:35:04Z

core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java

@@ -197,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) {


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

feeblefakie

LGTM! Thank you!

…ation fails because the target table doesn't exist (#2766) Co-authored-by: Hiroyuki Yamada <[email protected]>

Fix DB connection leak

4bb2e3a

when the target table is not found

komamitsu self-assigned this Jun 13, 2025

komamitsu requested a review from Copilot June 13, 2025 07:54

Copilot AI reviewed Jun 13, 2025

View reviewed changes

komamitsu requested review from brfrn169, feeblefakie and Torch3333 June 13, 2025 08:00

komamitsu marked this pull request as draft June 13, 2025 09:10

Only close connection when other exception occurs

60d4fba

komamitsu marked this pull request as ready for review June 13, 2025 09:50

Torch3333 approved these changes Jun 16, 2025

View reviewed changes

brfrn169 reviewed Jun 16, 2025

View reviewed changes

Improve error handling based on feedback

e96888e

brfrn169 approved these changes Jun 16, 2025

View reviewed changes

Take care of similar issue

f18d40f

komamitsu commented Jun 16, 2025

View reviewed changes

feeblefakie approved these changes Jun 17, 2025

View reviewed changes

Merge branch 'master' into close-scanner-conn-if-table-does-not-exist

c96db72

komamitsu merged commit 644578b into master Jun 17, 2025
106 of 107 checks passed

komamitsu deleted the close-scanner-conn-if-table-does-not-exist branch June 17, 2025 04:19

feeblefakie added a commit that referenced this pull request Jun 17, 2025

Fix potential connection leak when using jdbc storage and Scan oper…

b4d1939

…ation fails because the target table doesn't exist (#2766) Co-authored-by: Hiroyuki Yamada <[email protected]>

feeblefakie mentioned this pull request Jun 17, 2025

Backport to branch(3) : Fix potential connection leak when using jdbc storage and Scan operation fails because the target table doesn't exist #2778

Merged

komamitsu added a commit that referenced this pull request Jun 17, 2025

Fix potential connection leak when using jdbc storage and Scan oper…

e46d112

…ation fails because the target table doesn't exist (#2766) Co-authored-by: Hiroyuki Yamada <[email protected]>

komamitsu added a commit that referenced this pull request Jun 17, 2025

Fix potential connection leak when using jdbc storage and Scan oper…

47d8fba

…ation fails because the target table doesn't exist (#2766) Co-authored-by: Hiroyuki Yamada <[email protected]>

komamitsu added a commit that referenced this pull request Jun 17, 2025

Fix potential connection leak when using jdbc storage and Scan oper…

b64cb6a

…ation fails because the target table doesn't exist (#2766) Co-authored-by: Hiroyuki Yamada <[email protected]>

komamitsu added a commit that referenced this pull request Jun 17, 2025

Fix potential connection leak when using jdbc storage and Scan oper…

0694d6a

…ation fails because the target table doesn't exist (#2766) Co-authored-by: Hiroyuki Yamada <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

komamitsu commented Jun 13, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

komamitsu commented Jun 13, 2025 •

edited

Loading

Uh oh!

Torch3333 left a comment

Uh oh!

brfrn169 left a comment

Uh oh!

brfrn169 Jun 16, 2025

Uh oh!

brfrn169 Jun 16, 2025

Uh oh!

komamitsu Jun 16, 2025

Uh oh!

brfrn169 left a comment

Uh oh!

komamitsu Jun 16, 2025

Uh oh!

brfrn169 Jun 16, 2025

Uh oh!

feeblefakie left a comment

Uh oh!

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

komamitsu commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

komamitsu commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Torch3333 left a comment

Choose a reason for hiding this comment

Uh oh!

brfrn169 left a comment

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

komamitsu Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 left a comment

Choose a reason for hiding this comment

Uh oh!

komamitsu Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

feeblefakie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

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

komamitsu commented Jun 13, 2025 •

edited

Loading

komamitsu commented Jun 13, 2025 •

edited

Loading