Copilot commented on code in PR #2112:
URL: https://github.com/apache/fluss/pull/2112#discussion_r2642102718
##########
fluss-common/src/main/java/org/apache/fluss/utils/ExceptionUtils.java:
##########
@@ -365,10 +369,65 @@ public static <T extends Throwable> T firstOrSuppressed(T
newException, @Nullabl
if (previous == null || previous == newException) {
return newException;
- } else {
- previous.addSuppressed(newException);
+ }
+
+ // If the exceptions already reference each other through the
suppression or cause chains,
+ // return the previous exception to avoid introducing cycles.
+ if (existsInExceptionChain(newException, previous)
+ || existsInExceptionChain(previous, newException)) {
return previous;
}
+
+ previous.addSuppressed(newException);
+ return previous;
+ }
+
+ /**
+ * Checks whether the given {@code exception} throwable exception exists
anywhere within the
Review Comment:
The javadoc contains redundant wording: "the given {@code exception}
throwable exception". This should be simplified to either "the given {@code
exception}" or "the given throwable exception" to avoid redundancy.
```suggestion
* Checks whether the given {@code exception} exists anywhere within the
```
##########
fluss-common/src/test/java/org/apache/fluss/utils/ExceptionUtilsTest.java:
##########
@@ -121,6 +122,35 @@ void testFirstOrSuppressed() {
assertThat(suppressedException.getSuppressed()).isEqualTo(new
Throwable[] {newException});
}
+ @Test
+ void testFirstOrSuppressedCyclePrevention() {
+ // create two test exceptions (assuming thrown during shutdown, etc.)
+ Exception exceptionA = new Exception("Exception A");
+ Exception exceptionB = new Exception("Exception B");
+
+ // associate the suppressions (creating a suppression chain)
+ ExceptionUtils.firstOrSuppressed(exceptionB, exceptionA);
+ assertThat(exceptionA.getSuppressed()).contains(exceptionB);
+
+ // attempt to create a suppression cycle (A -> B; B -> A)
+ Exception result = ExceptionUtils.firstOrSuppressed(exceptionA,
exceptionB);
+ assertThat(result).isEqualTo(exceptionB);
+
+ // verify the exception cycle was prevented (no bidirectional
reference)
+ assertThat(exceptionA.getSuppressed()).contains(exceptionB);
+ assertThat(exceptionB.getSuppressed()).doesNotContain(exceptionA);
+ assertThat(exceptionB.getSuppressed()).isEmpty();
+
+ // verify that processing suppressed exceptions no longer causes
StackOverflowError
+ Assertions.assertDoesNotThrow(() ->
recursivelyProcessSuppressedExceptions(exceptionA));
+ }
Review Comment:
The test `testFirstOrSuppressedCyclePrevention` only validates cycle
prevention through the suppressed exception chain. However, the
`existsInExceptionChain` method also traverses the cause chain (line 424-427).
Consider adding a test case that validates cycle prevention when exceptions are
linked through cause chains, to ensure comprehensive test coverage of the
implemented behavior.
##########
fluss-common/src/main/java/org/apache/fluss/utils/ExceptionUtils.java:
##########
@@ -365,10 +369,65 @@ public static <T extends Throwable> T firstOrSuppressed(T
newException, @Nullabl
if (previous == null || previous == newException) {
return newException;
- } else {
- previous.addSuppressed(newException);
+ }
+
+ // If the exceptions already reference each other through the
suppression or cause chains,
+ // return the previous exception to avoid introducing cycles.
+ if (existsInExceptionChain(newException, previous)
+ || existsInExceptionChain(previous, newException)) {
return previous;
}
+
+ previous.addSuppressed(newException);
+ return previous;
+ }
+
+ /**
+ * Checks whether the given {@code exception} throwable exception exists
anywhere within the
+ * exception chain of {@code previous}. This includes both the cause chain
and all suppressed
+ * exceptions. A visited set is used to avoid cycles and redundant
traversal.
+ *
+ * @param exception The throwable exception to search for.
+ * @param previous The previous throwable exception chain to search in.
+ * @return True, if the exception is found within the suppressed chain,
false otherwise.
+ */
+ private static boolean existsInExceptionChain(Throwable exception,
Throwable previous) {
+ if (exception == null || previous == null) {
+ return false;
+ }
+ if (exception == previous) {
+ return true;
+ }
+
+ // Apply cycle prevention through a graph-like traversal of existing
+ // suppressed or cause chain exceptions
+ Set<Throwable> previousExceptions = new HashSet<>();
+ Deque<Throwable> exceptionStack = new ArrayDeque<>();
+ exceptionStack.push(previous);
+
+ while (!exceptionStack.isEmpty()) {
+ Throwable currentException = exceptionStack.pop();
+ if (!previousExceptions.add(currentException)) {
Review Comment:
The variable name `previousExceptions` in the context of a graph traversal
is misleading. This set actually contains all visited exceptions during the
traversal, not just "previous" exceptions. Consider renaming to
`visitedExceptions` or `seenExceptions` to better convey its purpose as a
cycle-prevention mechanism in the graph traversal.
```suggestion
Set<Throwable> visitedExceptions = new HashSet<>();
Deque<Throwable> exceptionStack = new ArrayDeque<>();
exceptionStack.push(previous);
while (!exceptionStack.isEmpty()) {
Throwable currentException = exceptionStack.pop();
if (!visitedExceptions.add(currentException)) {
```
##########
fluss-common/src/main/java/org/apache/fluss/utils/ExceptionUtils.java:
##########
@@ -365,10 +369,65 @@ public static <T extends Throwable> T firstOrSuppressed(T
newException, @Nullabl
if (previous == null || previous == newException) {
return newException;
- } else {
- previous.addSuppressed(newException);
+ }
+
+ // If the exceptions already reference each other through the
suppression or cause chains,
+ // return the previous exception to avoid introducing cycles.
+ if (existsInExceptionChain(newException, previous)
+ || existsInExceptionChain(previous, newException)) {
return previous;
}
+
+ previous.addSuppressed(newException);
+ return previous;
+ }
+
+ /**
+ * Checks whether the given {@code exception} throwable exception exists
anywhere within the
+ * exception chain of {@code previous}. This includes both the cause chain
and all suppressed
+ * exceptions. A visited set is used to avoid cycles and redundant
traversal.
+ *
+ * @param exception The throwable exception to search for.
+ * @param previous The previous throwable exception chain to search in.
Review Comment:
The parameter names in the javadoc don't accurately describe their
semantics. The parameter named `exception` is described as "The throwable
exception to search for" and `previous` is described as "The previous throwable
exception chain to search in". However, these names suggest temporal ordering
rather than the search relationship. Consider renaming them to something like
`searchTarget` and `chainToSearch` or `needle` and `haystack` to better convey
that this is a search operation, or update the description to clarify the
purpose more explicitly.
##########
fluss-common/src/test/java/org/apache/fluss/utils/ExceptionUtilsTest.java:
##########
@@ -121,6 +122,35 @@ void testFirstOrSuppressed() {
assertThat(suppressedException.getSuppressed()).isEqualTo(new
Throwable[] {newException});
}
+ @Test
+ void testFirstOrSuppressedCyclePrevention() {
+ // create two test exceptions (assuming thrown during shutdown, etc.)
+ Exception exceptionA = new Exception("Exception A");
+ Exception exceptionB = new Exception("Exception B");
+
+ // associate the suppressions (creating a suppression chain)
+ ExceptionUtils.firstOrSuppressed(exceptionB, exceptionA);
+ assertThat(exceptionA.getSuppressed()).contains(exceptionB);
+
+ // attempt to create a suppression cycle (A -> B; B -> A)
+ Exception result = ExceptionUtils.firstOrSuppressed(exceptionA,
exceptionB);
+ assertThat(result).isEqualTo(exceptionB);
+
+ // verify the exception cycle was prevented (no bidirectional
reference)
+ assertThat(exceptionA.getSuppressed()).contains(exceptionB);
+ assertThat(exceptionB.getSuppressed()).doesNotContain(exceptionA);
+ assertThat(exceptionB.getSuppressed()).isEmpty();
+
+ // verify that processing suppressed exceptions no longer causes
StackOverflowError
+ Assertions.assertDoesNotThrow(() ->
recursivelyProcessSuppressedExceptions(exceptionA));
Review Comment:
The test uses `Assertions.assertDoesNotThrow` from JUnit Jupiter while the
rest of the test class consistently uses AssertJ's assertion methods (like
`assertThat`). For consistency, consider using AssertJ's equivalent approach or
stick with the existing assertion pattern used throughout the file.
##########
fluss-common/src/main/java/org/apache/fluss/utils/ExceptionUtils.java:
##########
@@ -365,10 +369,65 @@ public static <T extends Throwable> T firstOrSuppressed(T
newException, @Nullabl
if (previous == null || previous == newException) {
return newException;
- } else {
- previous.addSuppressed(newException);
+ }
+
+ // If the exceptions already reference each other through the
suppression or cause chains,
+ // return the previous exception to avoid introducing cycles.
+ if (existsInExceptionChain(newException, previous)
+ || existsInExceptionChain(previous, newException)) {
return previous;
}
+
+ previous.addSuppressed(newException);
+ return previous;
+ }
+
+ /**
+ * Checks whether the given {@code exception} throwable exception exists
anywhere within the
+ * exception chain of {@code previous}. This includes both the cause chain
and all suppressed
+ * exceptions. A visited set is used to avoid cycles and redundant
traversal.
+ *
+ * @param exception The throwable exception to search for.
+ * @param previous The previous throwable exception chain to search in.
+ * @return True, if the exception is found within the suppressed chain,
false otherwise.
Review Comment:
The documentation of the return value for `existsInExceptionChain` is
inaccurate. The javadoc says "if the exception is found within the suppressed
chain" but the method actually searches through both the suppressed chain AND
the cause chain, which should be reflected in the documentation.
```suggestion
* @return {@code true} if the exception is found within the exception
chain (suppressed or
* cause), {@code false} otherwise.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
