LuciferYang opened a new issue, #7640:
URL: https://github.com/apache/kyuubi/issues/7640
### Description
`DynMethods.UnboundMethod.invoke` unwraps exceptions twice. `invokeChecked`
already unwraps the `InvocationTargetException` and throws the real exception
produced by the invoked method, but `invoke` then catches that exception and
re-throws `e.getCause()` as if `e` were still the wrapper:
```java
public <R> R invoke(Object target, Object... args) {
try {
return this.invokeChecked(target, args);
} catch (Exception e) {
if (e.getCause() instanceof RuntimeException) {
throw (RuntimeException) e.getCause();
}
throw new RuntimeException(e.getCause());
}
}
```
Two consequences for callers:
- An exception without a cause degrades to `new RuntimeException(null)` —
the type, message and stack trace of the real failure are all lost. For
example, a target method throwing `new IOException("connection refused")`
surfaces as a blank `RuntimeException` with a null message.
- A `RuntimeException` carrying a cause is silently replaced by its cause:
`IllegalStateException("...", inner)` surfaces as `inner`, so `catch
(IllegalStateException)` at the call site misses it.
Both shapes are reachable wherever `invoke` is used across the codebase
(e.g. `KyuubiBeeLine` error reporting, `EngineTab` install-error reporting,
`ApplicationMaster`'s exit message), which today can print `null` or a
misattributed failure instead of the real error.
### Provenance
The vendored copy diverges from both upstreams: parquet-common
(`org.apache.parquet.util.DynMethods`, unchanged since PARQUET-777 in 2017) and
Iceberg (`org.apache.iceberg.common.DynMethods`) both re-throw the
already-unwrapped `e` itself (`throwIfInstance(e, RuntimeException.class);
throw new RuntimeException(e);`). The double-unwrap was introduced by the
vendoring commit db0047d51 ([KYUUBI #3230]) when the upstream helper calls were
hand-translated into instanceof checks on `e.getCause()`. parquet's own
`TestDynMethods#testExceptionThrown` pins the correct semantics, which the
current code fails.
The sibling `DynConstructors.Ctor.newInstance` in the same package already
implements the correct pattern.
### Fix direction
Replace the second unwrap with: re-throw `RuntimeException` as-is and wrap
checked exceptions with the real exception as cause, matching
`DynConstructors.Ctor.newInstance` and upstream. Add unit tests for
`org.apache.kyuubi.util.reflect`, which currently has none.
--
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]
