This is an automated email from the ASF dual-hosted git repository.
yuqi4733 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new ad54a7a2a7 [#10177] improvement(core): ensure SessionUtils rolls back
on Throwable (#10335)
ad54a7a2a7 is described below
commit ad54a7a2a7b45b70cd2eccec7c200a3ccaae631d
Author: ChangJun Rho <[email protected]>
AuthorDate: Thu Mar 12 10:23:43 2026 +0900
[#10177] improvement(core): ensure SessionUtils rolls back on Throwable
(#10335)
### What changes were proposed in this pull request?
This PR updates `SessionUtils` to ensure transaction cleanup also runs
when callbacks throw non-`Exception` throwables.
The changes include:
- broaden the failure handling in `doWithCommit` from `Exception` to
`Throwable`
- apply the same pattern to `doWithCommitAndFetchResult`
- apply the same pattern to `doMultipleWithCommit`
- add unit tests to verify rollback happens when callbacks throw
`AssertionError`
With this change, `SqlSessions.rollbackAndCloseSqlSession()` is invoked
consistently for callback failures before the throwable is rethrown.
### Why are the changes needed?
`SessionUtils` previously caught only `Exception` in commit-managed
helper methods.
If a mapper callback threw a non-`Exception` throwable such as
`AssertionError`, the rollback path was skipped, which could leave the
SQL session or transaction state unclosed.
This PR fixes that bug by making rollback/close run for all throwables
in the callback path.
Fix: #10177
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added unit tests in `TestSessionUtils` to verify:
- `doWithCommit` rolls back and does not commit on `AssertionError`
- `doWithCommitAndFetchResult` rolls back and does not commit on
`AssertionError`
- `doMultipleWithCommit` rolls back and does not commit on
`AssertionError`
Verified with:
```bash
env
JAVA_HOME=/opt/homebrew/Cellar/openjdk@17/17.0.16/libexec/openjdk.jdk/Contents/Home
\
./gradlew :core:test --tests
org.apache.gravitino.storage.relational.utils.TestSessionUtils -PskipITs
---
.../storage/relational/utils/SessionUtils.java | 12 +--
.../storage/relational/utils/TestSessionUtils.java | 90 ++++++++++++++++++++++
2 files changed, 96 insertions(+), 6 deletions(-)
diff --git
a/core/src/main/java/org/apache/gravitino/storage/relational/utils/SessionUtils.java
b/core/src/main/java/org/apache/gravitino/storage/relational/utils/SessionUtils.java
index 0482bfecfd..8a36bc1add 100644
---
a/core/src/main/java/org/apache/gravitino/storage/relational/utils/SessionUtils.java
+++
b/core/src/main/java/org/apache/gravitino/storage/relational/utils/SessionUtils.java
@@ -40,9 +40,9 @@ public class SessionUtils {
T mapper = SqlSessions.getMapper(mapperClazz);
consumer.accept(mapper);
SqlSessions.commitAndCloseSqlSession();
- } catch (Exception e) {
+ } catch (Throwable t) {
SqlSessions.rollbackAndCloseSqlSession();
- throw e;
+ throw t;
}
}
@@ -56,9 +56,9 @@ public class SessionUtils {
R result = func.apply(mapper);
SqlSessions.commitAndCloseSqlSession();
return result;
- } catch (Exception e) {
+ } catch (Throwable t) {
SqlSessions.rollbackAndCloseSqlSession();
- throw e;
+ throw t;
}
}
@@ -101,9 +101,9 @@ public class SessionUtils {
try {
Arrays.stream(operations).forEach(Runnable::run);
SqlSessions.commitAndCloseSqlSession();
- } catch (Exception e) {
+ } catch (Throwable t) {
SqlSessions.rollbackAndCloseSqlSession();
- throw e;
+ throw t;
}
}
diff --git
a/core/src/test/java/org/apache/gravitino/storage/relational/utils/TestSessionUtils.java
b/core/src/test/java/org/apache/gravitino/storage/relational/utils/TestSessionUtils.java
new file mode 100644
index 0000000000..f3e7fc7d27
--- /dev/null
+++
b/core/src/test/java/org/apache/gravitino/storage/relational/utils/TestSessionUtils.java
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.storage.relational.utils;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.Mockito.mockStatic;
+import static org.mockito.Mockito.never;
+
+import org.apache.gravitino.storage.relational.session.SqlSessions;
+import org.junit.jupiter.api.Test;
+import org.mockito.MockedStatic;
+
+public class TestSessionUtils {
+
+ @Test
+ public void testDoWithCommitShouldRollbackOnAssertionError() {
+ Object mapper = new Object();
+
+ try (MockedStatic<SqlSessions> mockedSqlSessions =
mockStatic(SqlSessions.class)) {
+ mockedSqlSessions.when(() ->
SqlSessions.getMapper(Object.class)).thenReturn(mapper);
+
+ assertThrows(
+ AssertionError.class,
+ () ->
+ SessionUtils.doWithCommit(
+ Object.class,
+ ignored -> {
+ throw new AssertionError("boom");
+ }));
+
+ mockedSqlSessions.verify(SqlSessions::rollbackAndCloseSqlSession);
+ mockedSqlSessions.verify(() -> SqlSessions.commitAndCloseSqlSession(),
never());
+ }
+ }
+
+ @Test
+ public void testDoWithCommitAndFetchResultShouldRollbackOnAssertionError() {
+ Object mapper = new Object();
+
+ try (MockedStatic<SqlSessions> mockedSqlSessions =
mockStatic(SqlSessions.class)) {
+ mockedSqlSessions.when(() ->
SqlSessions.getMapper(Object.class)).thenReturn(mapper);
+
+ assertThrows(
+ AssertionError.class,
+ () ->
+ SessionUtils.doWithCommitAndFetchResult(
+ Object.class,
+ ignored -> {
+ throw new AssertionError("boom");
+ }));
+
+ mockedSqlSessions.verify(SqlSessions::rollbackAndCloseSqlSession);
+ mockedSqlSessions.verify(() -> SqlSessions.commitAndCloseSqlSession(),
never());
+ }
+ }
+
+ @Test
+ public void testDoMultipleWithCommitShouldRollbackOnAssertionError() {
+ try (MockedStatic<SqlSessions> mockedSqlSessions =
mockStatic(SqlSessions.class)) {
+ assertThrows(
+ AssertionError.class,
+ () ->
+ SessionUtils.doMultipleWithCommit(
+ () -> {
+ throw new AssertionError("boom");
+ }));
+
+ mockedSqlSessions.verify(SqlSessions::getSqlSession);
+ mockedSqlSessions.verify(SqlSessions::rollbackAndCloseSqlSession);
+ mockedSqlSessions.verify(() -> SqlSessions.commitAndCloseSqlSession(),
never());
+ }
+ }
+}
