qianye1001 opened a new issue, #10906:
URL: https://github.com/apache/rocketmq/issues/10906
### Before Creating the Bug Report
- [x] This is a bug rather than a support question.
- [x] I searched existing GitHub issues and discussions and did not find a
duplicate.
- [x] The bug is in this repository.
### Runtime platform environment
- macOS 15.1.1 (24B91), arm64
- Reproduced by a deterministic JUnit test; no broker is required.
### RocketMQ version
- Branch: `develop`
- Commit: `e3458616d207ee636b1762f0f8dcf788a590d59d`
- Version in the reactor: 5.5.0
- This commit was the tip of `apache/develop` when the reproduction was run.
### JDK Version
- Amazon Corretto 11.0.23
- Also verified with Oracle JDK 8u291.
### Describe the Bug
The first concurrent calls to `MQClientManager.getOrCreateMQClientInstance`
for the same client ID can construct multiple `MQClientInstance` candidates
even though only one is stored and returned.
The current sequence is:
```text
factoryTable.get(clientId)
new MQClientInstance(...)
factoryTable.putIfAbsent(clientId, candidate)
```
All callers that observe the initial miss construct independently. The
losing candidates are discarded without cleanup.
This is a resource leak, not only an allocation race. During construction,
every candidate creates a `ConsumerStatsManager`. It creates five
`StatsItemSet` objects, and every `StatsItemSet` registers six fixed-rate tasks
on the instance's single-thread `MQClientFactoryScheduledThread` executor. The
zero-delay sampling tasks start that non-daemon thread before
`MQClientInstance.start()`.
The surviving reference chain for a losing candidate is:
```text
live Thread
-> ThreadPoolExecutor.Worker
-> ScheduledThreadPoolExecutor
-> DelayedWorkQueue
-> periodic ScheduledFutureTask
-> StatsItemSet task
```
The tasks do not need to retain the outer `MQClientInstance`. Therefore the
losing `MQClientInstance` itself can be garbage-collected while its scheduler
thread and 30 periodic tasks remain live indefinitely.
There is a related failure-path leak. Construction schedules all 30 stats
tasks before creating the optional concurrent-heartbeat pool. Enabling
concurrent heartbeat with pool size 0 makes `Executors.newFixedThreadPool(0,
...)` throw `IllegalArgumentException`; no map entry is installed, but the
already-started stats scheduler remains live.
### Steps to Reproduce
1. Check out `e3458616d207ee636b1762f0f8dcf788a590d59d`.
2. Start 8 callers for one client ID.
3. Use a barrier in a test `ClientConfig.cloneClientConfig()` so every
caller has completed the initial map miss before any candidate can finish
construction.
4. Capture each constructed candidate with a test-only observer.
5. Record returned identities, the candidate schedulers and their queue
sizes.
6. Drop all strong references to losing `MQClientInstance` objects, retain
only their schedulers, and use `WeakReference` plus bounded GC polling.
7. Use bounded polling for thread counts and `awaitTermination`; no fixed
sleep is required.
The test was run with:
```bash
mvn -pl client -am -DskipITs -Dspotbugs.skip=true \
-Dtest=MQClientManagerLeakReproductionTest \
-Dsurefire.failIfNoSpecifiedTests=false test
```
Repeatable evidence from the unmodified code:
```text
candidates=8
returnedIdentity=1
liveSchedulerThreads=8
periodicTasks=240
losersGc=7/7
```
After explicitly calling `shutdownNow()` on all captured schedulers and
waiting for termination, the scheduler-thread count returns to the baseline.
### What Did You Expect to See?
For one client ID, concurrent first access should perform at most one
successful `MQClientInstance` construction and publish that instance to all
callers. Failed construction or failed initialization should not leave a map
entry, live executor thread, periodic task, or partially started client
resource.
Creation for different client IDs should not be guarded by a global lock.
### What Did You See Instead?
Eight concurrent callers constructed eight candidates. They all returned one
winner, but all eight scheduler threads and all 240 periodic tasks remained
live. Seven losing outer instances were garbage-collected, proving that
outer-object reachability and background-resource reachability are different.
A late constructor failure also left the scheduler thread and its 30
periodic tasks live even though `factoryTable` had no entry for the client ID.
### Additional Context
A constructor-path audit found these resource-bearing components:
- `MQClientInstance.scheduledExecutorService`: its thread starts during
`ConsumerStatsManager` construction because stats tasks have zero initial delay.
- `ConsumerStatsManager`: five `StatsItemSet` instances, six periodic tasks
each.
- `MQClientAPIImpl -> NettyRemotingClient`: allocates a timer, event-loop
group, selector, and executors; most threads start lazily, but partial
construction still needs rollback.
- `PullMessageService`: allocates a scheduled executor and retains the
client instance; it starts threads/tasks only on start or submission.
- `RebalanceService`: retains the client instance; its service thread starts
in `MQClientInstance.start()`.
- Inner `DefaultMQProducer` and the optional concurrent-heartbeat executor:
worker threads are lazy, but both require cleanup if initialization fails.
- `MQClientInstance.startScheduledTask()`, pull, rebalance, remoting, and
the inner producer introduce additional resources during `start()`; a mid-start
exception currently leaves the factory in `START_FAILED` without rollback.
A minimal fix should serialize construction per client ID, preserve parallel
construction for independent IDs, remove a failed mapping, roll back resources
on constructor/start failure, and use identity-aware removal so an old instance
cannot delete a newer replacement.
--
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]
