[ISSUE #10696] Fix async request future cleanup on synchronous send failure

[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 full CI matrix has been triggered for the refreshed head.

[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 49.33%. Comparing base (1a50c6e) to head (e340eb4).
⚠️ Report is 1 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%   49.33%   -0.07%     
+ Complexity     14237    14222      -15     
=============================================
  Files           1390     1390              
  Lines         103123   103138      +15     
  Branches       13484    13487       +3     
=============================================
- Hits           50940    50878      -62     
- Misses         46031    46087      +56     
- Partials        6152     6173      +21     

☔ 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