[ISSUE #10935] Fix shared produce accumulator lifecycle - #10937
Conversation
Signed-off-by: Rui <1685901819@qq.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #10937 +/- ##
=============================================
- Coverage 48.62% 48.50% -0.13%
+ Complexity 13692 13650 -42
=============================================
Files 1381 1381
Lines 101464 101468 +4
Branches 13187 13187
=============================================
- Hits 49337 49215 -122
- Misses 46142 46237 +95
- Partials 5985 6016 +31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Fixes a real bug where MQClientManager returns the same ProduceAccumulator to multiple producers sharing a client ID. Previously, shutting down one producer would stop the shared accumulator, breaking the other producer's batch sends. This PR adds reference counting in ProduceAccumulator and AtomicBoolean idempotency in DefaultMQProducer.
Findings
- [Info]
client/src/main/java/org/apache/rocketmq/client/producer/ProduceAccumulator.java:160–170— The dual protection (AtomicBooleaninDefaultMQProducer+synchronized+producerCountinProduceAccumulator) is correct and serves distinct purposes: theAtomicBooleanprevents a single producer from calling start/shutdown multiple times, whileproducerCounthandles multiple producers sharing the same accumulator. Well-designed. - [Info]
client/src/test/java/org/apache/rocketmq/client/producer/ProduceAccumulatorTest.java:115–168— The regression testtestSharedAccumulatorRemainsRunningUntilLastProducerShutdowncorrectly reproduces the bug on the unmodified baseline (deterministic failure) and passes with the fix. Good use ofCountDownLatchwith timeout for async verification.
Verdict
Clean bug fix with proper synchronization and comprehensive regression tests. The reference counting approach is the right solution for shared accumulator lifecycle management.
Automated review by github-manager-bot
Which Issue(s) This PR Fixes
Brief Description
MQClientManagercan return the sameProduceAccumulatorto multiple producers that share a RocketMQ client ID. Previously, everyDefaultMQProducer.shutdown()stopped that shared accumulator, even if another producer using it was still running. Under low traffic, its small synchronous auto-batched sends could then wait indefinitely, while asynchronous messages remained queued without a callback because timeout flushing had stopped.This change:
ProduceAccumulator;DefaultMQProducerwith anAtomicBoolean, making accumulator release idempotent across repeated shutdown calls;There is no public API or wire-format change.
How Did You Test This Change?
The new lifecycle regression was first applied to the unmodified
developbaseline. It wires the same accumulator into two realDefaultMQProducerinstances, starts both through the public lifecycle methods, shuts down one producer, queues a small asynchronous message, and waits for its callback. Two baseline runs failed deterministically after the callback deadline:With this change, the complete
ProduceAccumulatorTestsuite passes:I also ran all existing
DefaultMQProducerTestcases together with the accumulator tests:Command:
mvn -o -Dmaven.repo.local=/tmp/rocketmq-bug-m2 \ -pl client -am -DskipITs \ -Dcheckstyle.skip -Dspotbugs.skip -Drat.skip \ -Dsurefire.failIfNoSpecifiedTests=false \ -Dtest=ProduceAccumulatorTest,DefaultMQProducerTest testgit diff --checkalso passes.