Skip to content

[ISSUE #10737] Propagate admin future failures - #10738

Open
ai-yang wants to merge 1 commit into
apache:developfrom
ai-yang:agent/fix-admin-future-propagation
Open

[ISSUE #10737] Propagate admin future failures#10738
ai-yang wants to merge 1 commit into
apache:developfrom
ai-yang:agent/fix-admin-future-propagation

Conversation

@ai-yang

@ai-yang ai-yang commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Which Issue(s) This PR Fixes

Brief Description

MqClientAdminImpl used thenAccept to complete a separate result future and ignored the dependent stage. An exceptional remoting completion skipped the handler, while a decoder/handler exception completed only that ignored stage; in both cases the future returned to the caller remained pending.

Add one generic response bridge and use it for all 19 asynchronous admin operations. The bridge propagates the upstream remoting throwable and completes the public future exceptionally if existing response handling throws. Normal SUCCESS and non-SUCCESS response-code behavior is unchanged, and there is no public API or protocol change.

How Did You Test This Change?

  • Mocked one manually controlled remoting future across all 19 operations: the previous implementation left all public futures pending; the regression failed 5/5.
  • Added a SUCCESS/null-body decoder failure path: the previous implementation left the public future pending; the regression failed 5/5.
  • With the fix, the focused tests passed 20/20.
  • Complete MqClientAdminImplTest: 41/41 passed.
  • Complete JDK 8 client reactor: 995 tests, 0 failures, 0 errors, 1 skipped.
  • Maven validate / Checkstyle: 0 violations across common, remoting, and client.
  • SpotBugs: BugInstance=0, Error=0 across common, remoting, and client.
  • git diff --check: passed.

Signed-off-by: Rui <1685901819@qq.com>
@ai-yang
ai-yang marked this pull request as ready for review August 1, 2026 04:11

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by github-manager-bot

Summary

Fixes CompletableFuture propagation in all 19 async admin operations of MqClientAdminImpl. The previous thenAccept pattern left the caller's future permanently pending when the remoting call failed or the response handler threw.

Findings

  • [Info] MqClientAdminImpl.java:406-420 — The new handleResponse helper correctly uses whenComplete to bridge both remoting failures and handler exceptions to the public future. Catching Throwable in the handler path is appropriate since decoder/handler code may throw unchecked exceptions.
  • [Info] MqClientAdminImpl.java — All 19 call sites are updated consistently with the same pattern. No call site was missed.
  • [Info] MqClientAdminImplTest.java:529-596 — The allOperationsShouldPropagateRemotingFailure test efficiently covers all 19 operations in a single test by collecting futures and asserting after a single completeExceptionally. The operationShouldPropagateResponseHandlerFailure test covers the decoder-failure path with a null body triggering NPE.

Suggestions

No issues found. The fix is minimal, correct, and well-tested.

  • Correctness: The root cause (pending futures on error) is properly addressed. whenComplete is the right choice over handle or exceptionally since it preserves the void-like bridge pattern.
  • Compatibility: No public API or protocol change. The handleResponse method is private.
  • Tests: 41/41 tests pass, including the two new regression tests.

LGTM.


Automated review by github-manager-bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] MqClientAdmin futures never complete when remoting invocation fails

2 participants