qianye1001 opened a new pull request, #10907:
URL: https://github.com/apache/rocketmq/pull/10907
### Which Issue(s) This PR Fixes
- Fixes #10906
### Brief Description
Concurrent first access to `MQClientManager` previously used `get ->
construct -> putIfAbsent`. Callers racing on one client ID could therefore
construct multiple `MQClientInstance` candidates. Every candidate starts
`MQClientFactoryScheduledThread` during `ConsumerStatsManager` construction and
registers 30 periodic `StatsItemSet` tasks, so discarded candidates leaked live
scheduler threads and tasks.
The unmodified-code reproduction used 8 synchronized callers and observed:
```text
candidates=8
returnedIdentity=1
liveSchedulerThreads=8
periodicTasks=240
losersGc=7/7
```
The losing outer instances were garbage-collected while the scheduler
threads and periodic tasks remained live, confirming the resource reference
chain independently of `MQClientInstance` reachability.
This change:
- uses `ConcurrentHashMap.computeIfAbsent` so one client ID has at most one
successful construction and all callers receive the same instance;
- clones `ClientConfig` before entering the mapping function, keeping
overridable configuration code outside the map's per-bin reservation;
- does not add a global lock, so independent client IDs in different map
bins can construct concurrently;
- rolls back the stats scheduler, optional heartbeat executor, and remoting
client if construction fails after resources have been allocated;
- rolls back remoting, scheduled tasks, pull, rebalance, inner-producer, and
heartbeat resources if `start()` fails, removes the failed mapping, and
preserves the existing `START_FAILED` state;
- adds identity-aware map removal so a stale instance's delayed shutdown
cannot remove a newer replacement.
#### Design trade-offs
- A global synchronized section would unnecessarily serialize every client
ID.
- A removable per-key lock map has waiter/lifecycle and ABA hazards;
retaining locks forever would introduce a different leak.
- Moving all constructor resources to `start()` would be a much larger
lifecycle and compatibility change, and by itself would not prevent duplicate
candidates.
- `computeIfAbsent` provides the required atomic publication and exception
behavior with the existing Java 8 `ConcurrentHashMap`: an exception installs no
mapping, and a later call may retry.
The fast-path `get` and existing public removal method remain intact. Normal
repeated `start()`/`shutdown()` behavior and the `START_FAILED` retry rejection
on the same object are preserved.
#### Resource audit
The constructor/start path was audited for thread, scheduled-task, and
retained-resource creation:
- `ConsumerStatsManager`: five `StatsItemSet` objects with six periodic
tasks each; zero-delay sampling starts `MQClientFactoryScheduledThread` during
construction.
- `MQClientAPIImpl -> NettyRemotingClient`: timer, event-loop/selector,
public and scan executors; most threads are lazy, but the remoting client is
shut down on partial construction/start failure.
- `PullMessageService`: service thread plus its scheduled executor; both are
stopped on start failure.
- `RebalanceService`: service thread stopped on start failure.
- Inner `DefaultMQProducer`: registration/executors and
request-future/detector lifecycle are shut down when started.
- Optional concurrent-heartbeat executor: shut down on construction or start
failure.
- `MQClientInstance.startScheduledTask()`: all client periodic tasks use the
scheduler that is shut down on failure.
### How Did You Test This Change?
All commands used the repository-configured Maven source/target 1.8; no
compiler target was changed.
1. Full client reactor regression on Amazon Corretto 11.0.23:
```bash
mvn -pl client -am -DskipITs -Dspotbugs.skip=true test
```
Results:
- `rocketmq-common`: 243 tests, 0 failures/errors
- `rocketmq-remoting`: 174 tests, 0 failures/errors
- `rocketmq-client`: 585 tests, 0 failures/errors, 1 skipped
- Reactor: BUILD SUCCESS (3:45)
2. Focused regression plus SpotBugs/checkstyle on Amazon Corretto 11.0.23:
```bash
mvn -pl client -am -DskipITs \
-Dtest=MQClientManagerTest \
-Dsurefire.failIfNoSpecifiedTests=false test
```
Results: 7 tests passed; SpotBugs reported 0 findings and checkstyle
reported 0 violations.
3. Java 8 compatibility on Oracle JDK 8u291:
```bash
mvn -pl client -am -DskipITs -Dspotbugs.skip=true \
-Dtest=MQClientManagerTest \
-Dsurefire.failIfNoSpecifiedTests=false test
```
Results: 7 tests passed. `javap -verbose` reports class-file major
version 52.
The new tests use barriers, bounded futures, Awaitility assertions, executor
termination waits, and isolated thread groups rather than fixed sleeps. They
cover:
- high-concurrency same-client-ID creation: one instance, one scheduler, no
orphan candidates;
- concurrent construction for different client IDs;
- late constructor failure: no map entry or scheduler thread, followed by
successful retry;
- removal and recreation;
- repeated start/shutdown and recreation;
- mid-start failure rollback and replacement creation;
- stale shutdown preserving 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]
