[server] Add meta-test to prevent exception cause loss in catch blocks#3776
Open
XuQianJin-Stars wants to merge 1 commit into
Open
[server] Add meta-test to prevent exception cause loss in catch blocks#3776XuQianJin-Stars wants to merge 1 commit into
XuQianJin-Stars wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
An earlier PR —
Preserve exception cause when rethrowing catalog exceptions— already fixed the most visible
catch (Exception e) { throw new XxxException(...); }sites in
fluss-serverso that the original cause (e) is preserved in theexception chain. That change is great for debugging because it lets us trace
the root cause from server logs without having to grep through stdout.
However, the same pitfall — re-throwing a new exception without passing
the caught one as
cause— is easy to reintroduce in any new code path,because the Java compiler happily accepts a single-argument constructor
call. Without a guard, future catalog / coordinator / RpcServiceBase
contributions could quietly regress the diagnostic chain again.
This PR adds a regression-proof static check for exactly that pattern
inside the
fluss-servermodule: aRpcServiceBaseTest#metaTestNoCatchRethrowWithoutCausetest that walks every
.javafile undersrc/main/java/org/apache/fluss/server,parses
catch (X var)/throw new YyyException(...)sites, and fails thebuild if any
throw newinside acatchblock does not reference thecaught variable in its constructor arguments.
In addition to the guard, this PR performs a one-time, full scan over
fluss-server/src/main/java/org/apache/fluss/server/to confirm that noremaining
throw new XxxException(...)inside a catch block is missing thecause — and the scan passes against the current
upstream/main, so nobusiness code is changed in this PR.
Linked issue: N/A (build-time regression guard for catalog / RPC exception
handling)
Brief change log
fluss-server/src/test/java/org/apache/fluss/server/RpcServiceBaseTest.javawith a single but powerful meta-test:
RpcServiceBaseTest#metaTestNoCatchRethrowWithoutCause.src/main/java/org/apache/fluss/server(constantSERVER_SOURCE_DIR) viajava.nio.file.Files.walkFileTreeand aSimpleFileVisitor<Path>, processing every.javasource file.catchblocks and theirbrace depth / caught variable name, populated by a
CATCH_PATTERNregex that captures the variable name from anycatch (Type varName)clause.catchblock ends andmust be popped from the active set.
THROW_NEW_PATTERN(
throw new XxxException(), collects the full multi-line statementvia
collectThrowStatement(...)(balanced parens scanner).appears somewhere in the throw statement's text. If not, the
statement is appended to a
violationslist along with the filepath, line number, and the offending statement.
assertThat(violations).isEmpty(), failing thebuild with a clear, actionable message that lists every offending file
and line.
upstream/mainreturns zero violations, which is itself the proofpoint that the earlier cause-preservation work has held.
Tests
RpcServiceBaseTest#metaTestNoCatchRethrowWithoutCause— the onlytest method in the new file. It:
actionable error message if run from the wrong CWD).
.javafile.violationslist is empty.file:line: catch(var) -> statemententries in the assertion message so a reviewer can immediately
navigate to the offender.
~0.3son a developer laptop (it is purefile-IO + regex; no real cluster needed), so the cost of running it
in CI is negligible.
upstream/main:mvn test -pl fluss-server -Dtest=RpcServiceBaseTest→
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0.API and Format
@Testfromorg.junit.jupiter.api).assertThat(...).isTrue(),.isEmpty()).@Timeoutannotation (relies on the global per-test timeout).metaTestNoCatchRethrowWithoutCause).Test(unit, not ITCase).mvn spotless:applyclean — imports and Javadoc followAGENTS.md §3 Code Organization.
java.nio.file,java.util.regex,java.util.Listand lambdas, all available since Java 8.[server] Add meta-test to prevent exception cause loss in catch blocksfollows the<component>prefix convention../mvnw clean install -DskipTests -T 32passes locally on JDK11.0.23-kona.Documentation
catchblocks that throwa new exception without preserving the cause).
chains, which hurts RPC-side debugging).
argument to the new exception constructor").
this pattern.
fluss-client) can simply copy the test class and pointSERVER_SOURCE_DIRat the new module — the scanner is intentionallymodule-agnostic.