[ai-yang]

Which Issue(s) This PR Fixes

• Fixes [Bug] Async request future remains registered after synchronous send failure #10696

Brief Description

The asynchronous request-reply overloads register a RequestResponseFuture before invoking the underlying send method. When send initiation throws synchronously, the exception reaches the caller but the future remains in RequestFutureHolder until timeout processing, which retains request state and can deliver a callback after the synchronous failure.

This change:

• removes the exact registered future when sendDefaultImpl, sendSelectImpl, or sendKernelImpl does not return normally;
• uses remove(correlationId, requestResponseFuture) so a concurrent replacement under the same key is not removed;
• preserves the future after a normal asynchronous handoff so reply, send-failure, and timeout paths continue to own completion;
• updates the existing synchronous-exception test semantics and adds regression coverage for the default, selector, and explicit-queue callback overloads.

How Did You Test This Change?

Verified on 2026-09-11 at head e340eb4d4b1372d04c9cdf3c57799a6c9724662e, rebased onto develop at 1a50c6e4e35524ac22055b76fc0ff2a6d3cff99f, using JDK 8 and Maven 3.8.1:

mvn -pl client -am -DskipITs \
  -Dtest=DefaultMQProducerTest,DefaultMQProducerImplTest \
  -Dsurefire.failIfNoSpecifiedTests=false clean test

• DefaultMQProducerTest: 41 tests passed.
• DefaultMQProducerImplTest: 37 tests passed.
• Total: 78 tests, 0 failures, 0 errors, 0 skips; all 4 reactor modules succeeded.
• Checkstyle: 0 violations; SpotBugs: 0 bug instances and 0 errors.
• git diff --check: passed.

The new regression assertion was previously verified to fail before the production fix because the correlation ID remained in requestFutureTable. Earlier full-client reactor evidence (995 tests, 0 failures/errors, 1 skip) predates this rebase; the latest local run above is targeted, and the fresh hosted CI results are recorded separately below.

Hosted CI follow-up (2026-11-02)

The original head remains e340eb4d4b1372d04c9cdf3c57799a6c9724662e. Closing and immediately reopening this PR created fresh runs after the original runs had passed GitHub's rerun window; no empty commit, source change, test skip, or retry logic was added.

All seven freshly triggered workflows succeeded:

• Maven build and tests, including Ubuntu, macOS, and the previously failing Windows JDK 8 job.
• Integration tests.
• Bazel, Coverage, CodeQL, License checker, and Misspell Check also passed.

These runs validate the current PR merge ref. They are not a controlled reproduction of the original base/head pair and do not establish that the earlier failure was unrelated or flaky.

[RockteMQ-AI]

Review by github-manager-bot

Summary

This PR fixes a resource leak in the async request-reply path of DefaultMQProducerImpl. When sendDefaultImpl, sendSelectImpl, or sendKernelImpl throws synchronously (before async handoff), the registered RequestResponseFuture remains in RequestFutureHolder.requestFutureTable until timeout. The fix wraps each send invocation in a try/finally block with a sendInvocationCompleted flag, removing the future only when the send call itself fails.

Findings

• [Info] DefaultMQProducerImpl.java:1662-1689 — The try/finally + boolean flag pattern is clean and idiomatic. Using ConcurrentHashMap.remove(key, value) (conditional remove) correctly avoids clobbering a concurrent replacement under the same correlation ID. Good defensive design.

• [Info] DefaultMQProducerImpl.java — All three request() overloads (default, selector, kernel/queue) apply the same pattern consistently. This uniformity makes the fix easy to verify and maintain.

• [Info] DefaultMQProducerTest.java:483-508 — The test rename from testAsyncRequest_OnException to testAsyncRequest_SynchronousExceptionRemovesFuture better reflects the actual behavior being tested. The assertion change from checking callback invocation to checking future removal is semantically correct for this scenario.

• [Info] selector/DefaultMQProducerImplTest.java:134-175 — Good coverage of all three send paths (default, selector, queue) with dedicated regression tests. The @Before/@After cleanup of requestFutureTable prevents test pollution.

• [Info] The timeout - cost computation is a pre-existing concern (could go negative if cost > timeout), but is out of scope for this PR.

Verdict

Well-structured, focused fix for a real resource leak. The code is clean, tests are thorough (995 tests passing), and the conditional remove(key, value) shows attention to concurrent correctness. No issues found.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Summary

Defensive fix with proper validation and test coverage. LGTM.

---

Automated review by github-manager-bot

[ai-yang]

Updated and ready for another review round:

• Rebased onto the current apache/rocketmq:develop (e348efa66b08eb645ee123706ea6492fa9a3ad35).
• Refreshed the branch to signed-off commit 56a47e8292251a2858b5fa9baab0a8e08f7261bf.
• Clean targeted verification passed:
  • mvn -Dmaven.repo.local=/developer/wangrui/.m2/repository -pl client -am -DskipITs -Dtest=DefaultMQProducerTest,DefaultMQProducerImplTest -Dsurefire.failIfNoSpecifiedTests=false clean test
  • 77 tests, 0 failures/errors/skips.
  • All 4 reactor modules succeeded; Checkstyle and SpotBugs reported no violations.

The background name-server lookup warnings in the test log are expected in the isolated test environment and did not affect the result.

@drpmma @RongtongJin, could you please take a look when convenient?

[codecov-commenter]

Codecov Report

❌ Patch coverage is 54.54545% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.26%. Comparing base (1a50c6e) to head (e340eb4).
⚠️ Report is 38 commits behind head on develop.

Files with missing lines	Patch %	Lines
...mq/client/impl/producer/DefaultMQProducerImpl.java	54.54%	15 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10697      +/-   ##
=============================================
+ Coverage      49.39%   50.26%   +0.86%     
- Complexity     14237    15007     +770     
=============================================
  Files           1390     1391       +1     
  Lines         103123   106890    +3767     
  Branches       13484    14574    +1090     
=============================================
+ Hits           50940    53730    +2790     
- Misses         46031    46801     +770     
- Partials        6152     6359     +207     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ai-yang]

Refreshed onto develop at 1a50c6e4e and pushed signed-off commit e340eb4d4.

Resolved the import conflict in DefaultMQProducerImplTest by retaining both the upstream EndTransactionRequestHeader and regression-test After imports. The request-future cleanup implementation is unchanged by the rebase.

Clean JDK 8 verification: mvn -pl client -am -DskipITs -Dtest=DefaultMQProducerTest,DefaultMQProducerImplTest -Dsurefire.failIfNoSpecifiedTests=false clean test passed 78/78 tests across the two classes. All 4 reactor modules passed with Checkstyle and SpotBugs enabled. The PR description now distinguishes this latest run from earlier full-reactor evidence.

@drpmma @RongtongJin, could you take a human review of the synchronous-send-failure cleanup when convenient? The updated commit also triggers a new full CI run.

[RockteMQ-AI]

Summary

Re-reviewed after new commits. The try-finally pattern correctly cleans up RequestFutureHolder entries when sendDefaultImpl/sendSelectImpl/sendKernelImpl throws synchronously (e.g., serialization failure, no available broker).

Key points:

• sendInvocationCompleted flag is a clean way to detect synchronous failure before the async callback takes over
• remove(correlationId, requestResponseFuture) with value check prevents removing a different future if the ID was somehow reused
• Applied consistently across all three request() overloads
• Tests verify cleanup on sync failure and normal async path

LGTM — re-approved with the latest changes.

---

Automated review by github-manager-bot

[ai-yang]

The remaining Windows CI failure is in ConsumerOrderInfoManagerLockFreeNotifyTest.testRecover: Awaitility reports the condition satisfied after 1.611 seconds, earlier than the specified minimum of 2 seconds. This identifies the timing assertion but does not establish whether this PR causes it. Could a maintainer rerun that job on the same head and, if it repeats, compare the case with the base? I do not have upstream Actions write permission.

[ai-yang]

@RongtongJin Incremental CI update for #10697, #10937 and #10759: their failed runs remain at attempt 1 with no newer run on the current heads. They were created on September 11/13, beyond GitHub's documented 30-day rerun window, so repeating the old-run requests is no longer the right next step. The original Bazel artifacts for #10759 are also no longer listed.

The three workflows already support pull_request.reopened and do not define workflow_dispatch. Could we coordinate a fresh CI trigger without changing the PR heads? I have not closed/reopened the PRs or added empty commits. The existing failure diagnoses remain unchanged; in particular, #10759's fixture enables ConsumeQueueExt, so an interaction has not been excluded. My current repository permissions are read-only for upstream Actions.

Fresh CI results

With the author's go-ahead, the three original PRs were closed and immediately reopened. All remain open, and all three head SHAs are unchanged. This created seven new workflows per PR, without empty commits or production changes:

• [ISSUE #10696] Fix async request future cleanup on synchronous send failure #10697: new Maven run
• [ISSUE #10935] Fix shared produce accumulator lifecycle #10937: new integration run
• [ISSUE #10756] Fix duplicate dispatch ConsumeQueueExt leak #10759: new Bazel run

These are fresh runs against the current PR merge refs, not strict reproductions of the original base/head experiments. All 21 freshly triggered workflows have now completed successfully (seven per PR), including all three original failing workflow types. Maven passed on Ubuntu, macOS, and Windows with JDK 8 for each PR. The individual PR descriptions now link the new validation evidence. This does not establish that the original failures were flaky or unrelated.