iloveeyjafjalla opened a new pull request, #11428:
URL: https://github.com/apache/rocketmq/pull/11428
### Which Issue(s) This PR Fixes
- Fixes #11427
### Brief Description
`DefaultMQPushConsumerImpl.start()` creates and starts two consume services,
but neither shutdown path stops the pop one:
```java
this.consumeMessageService.start();
// POPTODO
this.consumeMessagePopService.start(); // :984-986
...
case RUNNING:
this.consumeMessageService.shutdown(awaitTerminateMillis); // :908 —
the pop service was left running
```
The pop service is not idle: `ConsumeMessagePopOrderlyService.start()`
schedules `lockMQPeriodically()` at a fixed rate on a single-thread executor
created with `new ThreadFactoryImpl("ConsumeMessageScheduledThread_")`
(non-daemon by default), and `ConsumeMessagePopConcurrentlyService.start()`
starts its consume executor. Both services implement `shutdown(long)`, and no
code in the client module ever calls it — the field is only assigned, started
and used for `submitPopConsumeRequest`. The same omission existed in the
failed-start branch (consumer group already registered).
This change stops the pop service in both places, next to the non-pop one:
```java
this.consumeMessageService.shutdown(awaitTerminateMillis);
this.consumeMessagePopService.shutdown(awaitTerminateMillis);
```
so a shutdown consumer really releases its resources
(`DefaultMQPushConsumer.shutdown()` documents exactly that) and no non-daemon
scheduler keeps firing on an empty process-queue table.
### How Did You Test This Change?
New test
`DefaultMQPushConsumerImplTest#testShutdownStopsThePopConsumeService`: it
injects a mocked `consumeMessagePopService` (the harness already does this for
other tests), puts the consumer in `RUNNING` and calls `shutdown()`, then
asserts the service received `shutdown(...)`.
Results on Temurin JDK 8 (the JDK CI uses), base `a7edeecfb` plus this patch:
- before the fix: `Wanted but not invoked:
consumeMessageService.shutdown(...)` — the pop service was never stopped
- after the fix: `DefaultMQPushConsumerImplTest` — `Tests run: 44, Failures:
0, Errors: 0, Skipped: 1`
- the consumer-facing client classes together
(`DefaultMQPushConsumerImplTest`, `DefaultLitePullConsumerImplTest`,
`DefaultMQPushConsumerTest`, `RebalanceImplTest`, `RebalancePushImplTest`,
`RebalanceLitePullImplTest`): `Tests run: 67, Failures: 0, Errors: 0, Skipped:
1`
- `mvn -B -ntp -pl client clean package` (module tests + checkstyle +
spotbugs enabled): `BUILD SUCCESS`, 603 tests / 0 failures / 0 errors / 8
skipped, `You have 0 Checkstyle violations.`, `BugInstance size is 0`.
This change was prepared with AI assistance (analysis, patch and test
drafting); the diff and every result above were reproduced locally by running
the module's tests against the base commit and the patched tree.
--
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]
