Doris-Breakwater commented on issue #66037: URL: https://github.com/apache/doris/issues/66037#issuecomment-5077417482
Breakwater-GitHub-Analysis-Slot: slot_fc1d2497e5a7 This content is generated by AI for reference only. Initial assessment: **partially valid**. The Java-side cleanup gap is real, but the reported “sticky JNI exception survives into a later pooled-thread call” mechanism is contradicted by the current native exception wrapper. I checked the live issue metadata and the relevant code on current `master` (`27cb451229c7a42dd089c5248847828eb0536d02`) and `upstream/branch-4.0` (`823b3fa039c921047aaccd8cb097d5c3a205a918`). At triage time the issue is open with no labels, assignee, milestone, or linked PR. Confirmed Java-side issue: - `fe/be-java-extensions/java-common/.../JniScanner.java:getNextBatchMeta()` allocates and retains `vectorTable`, but its catch block covers only `IOException`. - A `RuntimeException` or `NullPointerException` from `getNext()` therefore bypasses the immediate `releaseTable()` call. The `VectorTable` remains referenced, including its native allocations, until a later scanner cleanup succeeds. - This is a valid deterministic resource-lifetime bug and should be fixed in the Java wrapper so cleanup does not depend on a later BE close path. However, the current evidence proves retention until close, not yet a permanent process-lifetime leak: both legacy and v2 BE close paths normally attempt `releaseTable()`. The cross-query pending-exception claim is not reproduced by the current code: - On `master`, `Jni::FunctionCall::call()` invokes `RETURN_ERROR_IF_EXC` immediately after every wrapped Java method call (`be/src/util/jni-util.h`). `Jni::Env::GetJniExceptionMsg()` obtains the throwable and calls `ExceptionClear()` before returning the error `Status` (`be/src/util/jni-util.cpp`). - Both `JniReader::_do_get_next_block()` and `JniTableReader::_get_next_jni_block()` use that wrapper for `getNextBatchMeta()`. Thus an unchecked Java exception is converted to an error and cleared on the same JNI call; it is not left pending for a later `getStatistics()` or another query. - `upstream/branch-4.0` has the same effective behavior in the older path: `JniConnector::get_next_block()` calls `RETURN_ERROR_IF_EXC` immediately after `CallLongMethod`, and `JniUtil::GetJniExceptionMsg()` clears the exception. - The v2 close path is already best-effort and retryable after #65503: `_close_jni_scanner()` attempts both `releaseTable()` and Java `close()` even if the first fails, and it retains state when cleanup fails. Its metric calls also use wrappers that consume/clear Java exceptions. There is a separate legacy cleanup weakness worth fixing: `JniReader::close()` marks `_closed` before cleanup and uses early-return error handling for the metric getters, `releaseTable()`, and Java `close()`. An independent close-time JNI failure can therefore skip later cleanup and prevent retry. That is a real leak risk, but it is not caused by a pending exception left by `getNextBatchMeta()` under the current wrapper. Recommended fix/review direction: 1. Accept a Java-side change that releases the table for unchecked failures from `getNext()`, preserves the original exception as the cause, and handles cleanup failures without silently replacing the primary failure. 2. Be careful with `catch (Throwable)` plus unconditional normalization: `Throwable` includes JVM `Error`s such as `OutOfMemoryError`. Cleanup should still be attempted, but fatal `Error` semantics should normally be preserved rather than downgraded to `IOException`; normalization is appropriate for `RuntimeException`/NPE. 3. Do not add blind BE “discard any residual exception” logic as the root-cause fix without identifying an actual unwrapped JNI call that can leave the exception pending. Clearing an unexplained exception at a later entry point can hide the real violating call. The current `getNextBatchMeta()` path already clears it. 4. Independently harden legacy `JniReader::close()` along the v2 lifecycle pattern: make profile collection best-effort, always attempt both table release and scanner close, and only commit the closed state after cleanup has completed or preserve retryable state. Required tests: - A Java unit test with a synthetic `JniScanner` whose `getNext()` throws `RuntimeException` after the table has been created; assert that `getTable()` is null afterward and that the reported `IOException` retains the original cause. - Equivalent coverage for an existing connector-style NPE/unchecked failure and idempotent cleanup. - A JNI integration test showing that after the failed `getNextBatchMeta()` call, `env->ExceptionCheck()` is false and a benign Java call on the same native thread succeeds. This directly validates or disproves the issue’s pooled-thread claim. - Legacy close-path tests where metric collection and resource release fail independently, verifying that Java `close()` is still attempted and cleanup can be retried. Missing evidence needed to establish the broader production failure described in the issue: - The exact Doris build SHA and concrete connector/scanner used; “master / 4.0” is not enough for backport scope. - A complete first-failure Java stack trace plus the allegedly misattributed later JNI error, including query IDs and BE thread IDs. - A minimal deterministic reproducer that runs the failure and the later healthy call on the same scan thread. - A BE memory-tracker snapshot/delta after scanner close, demonstrating that the native allocation remains outstanding after cleanup rather than only between `getNext()` failure and close. No Doris Profile is required to validate the Java exception-cleanup gap itself. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
