RockteMQ-AI commented on PR #2948:
URL:
https://github.com/apache/rocketmq-dashboard/pull/2948#issuecomment-5502618450
Verification complete — I've confirmed the repo state, the three
pre-existing unguarded walks, the test helpers the new tests rely on, and found
the same latent bug in six other files. Here is my review.
---
# Review: fix(broker): bound root-cause traversal to survive cyclic
exception chains
## Summary
The PR bounds three `Throwable.getCause()` walks (`MqAdminExtFactory`,
`MqClientPool`, `RegistryProbeRunner`) with an identity-based visited set, and
adds `newPullConsumer`/`newProducer` protected seams so `MqClientPool` can be
unit-tested with stub clients. The core fix is correct — I traced the new loop
against acyclic chains, direct self-cycles, two-exception cycles, and cycles
that don't include the head; all terminate, and behavior for non-cyclic chains
is unchanged (same terminal exception, same message). `IdentityHashMap` is the
right choice (identity semantics, no `hashCode`/`equals` dispatch on
exceptions). Thread safety is fine: the set is method-local, only allocated on
error paths; the new seams don't alter the Spring lifecycle. No backward-compat
concerns.
## ⚠️ Diff integrity — please confirm before merging
In the diff as provided, all three new test methods show
`@server/src/main/java/org/apache/rocketmq/studio/settings/DataSourceTestDTO.java`
where `@Test` should be (e.g. before
`probeAllShouldSurviveCyclicCauseChainWithoutHangingTest`). Taken literally
that's a compile error; if the annotations are merely missing, JUnit 5 will
silently skip the tests and the "Tests run: 21" claim (6 + 14 + 1, which does
match the expected counts) wouldn't hold. The referenced
`DataSourceTestDTO.java` content is unrelated to this PR and looks like
stray/injected content in the diff/description — I excluded it from review
scope. Please re-verify the actual patch and PR description for tampering.
## Main issue: the bug class is only partially fixed
The PR's own rationale ("nothing guarantees cause chains are acyclic …
unbounded walk is a latent infinite loop on the hot path") applies verbatim to
at least six other sites that this PR leaves untouched:
| Location | Guard today |
|---|---|
| `RocketMQClientProvider.java:426` (`rootMessage`) | none |
| `RocketMQClusterProvider.java:238` (`rootMessage`) | none |
| `RocketMQConsumerDiagnosticsProvider.java:133` (`rootMessage`) | none |
| `ApacheAclReadService.java:85` (`rootMessage`) | direct self-cycle only |
| `RocketMQMessageProvider.java:501` (`isTraceTopicAbsent`) | direct
self-cycle only |
| `SettingsService.java:456` (`hasCause`) | none |
| `AbstractPrometheusCompatibleMetricsSource.java:407` | none |
The provider-side `rootMessage` methods are the same user-facing 502-message
hot paths as the three fixed ones. Fixing three copies and leaving five+
creates two coexisting idioms for the same problem. I'd suggest either fixing
all sites in this PR, or (better) extracting one shared helper, e.g.
`org.apache.rocketmq.studio.common.util.RootCauses.rootMessage(Throwable)` plus
a bounded-iteration variant for the `hasCause`/`isTraceTopicAbsent` walks, and
migrating every call site. That also removes the now-quadruplicated `message ==
null ? getClass().getSimpleName() : message` tail.
## Minor observations
- `MqAdminExtFactory.java` — the added `import java.util.Set;` lands between
`Map` and `Objects`, breaking the existing alphabetical order. Trivial.
- When the loop exits on a repeat, the reported message comes from the first
repeated exception — arbitrary in a cycle, but acceptable since a cycle has no
true root.
- Optional hardening: an identity set doesn't bound a pathological
`getCause()` override returning fresh instances per call; a small max-depth cap
(e.g. 64) would cover that too. Low priority.
- The `MqClientPool` seams are fine and consistent with the existing
`newAdmin` seam in `MqAdminExtFactory.java:173`.
## Tests
Good approach overall: two-exception cycle + `assertTimeoutPreemptively` is
the right tool, and the unfixed-code failure claims are consistent with how
`CompletableFuture.join()` ignores interrupts. Gaps:
- No cyclic-chain test for the client **start-failure** paths —
`createPullConsumer`/`createProducer` catch blocks (and
`MqAdminExtFactory.createAndStart`) also call `rootMessage`; a
`doThrow(cyclic).when(producer).start()` case would cover them.
- `withPullConsumer` cyclic path is untested (only `withProducer`); both
route through the same `execute`, so low risk.
- A direct self-cycle case would pin the old-behavior equivalence.
**Verdict:** the three fixes are correct and well-tested, but please (1)
confirm the `@Test` annotations are intact in the real patch, and (2) either
extend the guard to the remaining unguarded sites or land a shared utility so
the class of bug is closed out, not three instances of it.
--
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]
