[PR] [ISSUE #11441] fix(broker): wake the held pull requests when an arrival drained one bucket [rocketmq]

["iloveeyjafjalla (via GitHub)" <gi...@apache.org>]

iloveeyjafjalla opened a new pull request, #11442:
URL: https://github.com/apache/rocketmq/pull/11442

   ## What this changes
   
   `PullRequestHoldService#notifyMasterOnline()` checked a bucket for emptiness and then iterated the **result** of the drain:
   
   ```java
   for (PullRequest request : mpr.cloneListAndClear()) {
   ```
   
   `ManyPullRequest#cloneListAndClear()` answers `null`, not an empty list, when the bucket is empty at drain time. The buckets are drained by the message-arrival path on the store's dispatch thread, so an arrival that wins the race between the `isEmpty()` check and the drain makes that call answer `null` and the enhanced `for` throw a `NullPointerException`.
   
   The exception escaped `notifyMasterOnline()` into `BrokerController#onMinBrokerChange` (including the `UPDATE_MIN_BROKER` admin path), abandoning the sweep half way: every bucket not visited yet kept its suspended pull requests until the client's own suspend timeout - the exact stall this wake-up exists to prevent, in the failover window it is meant for.
   
   Fix: drain into a local variable and skip the bucket that lost the race. This mirrors the guard `notifyMessageArriving` already has around the same call in the same class, so the null contract is now handled consistently.
   
   ## Evidence
   
   Red run before the fix (`broker`, JDK 8), forced interleaving where the bucket is drained by a concurrent arrival right after the emptiness check:
   
   ```
   java.lang.AssertionError:
   Expecting code not to raise a throwable but caught
     "java.lang.NullPointerException
   	at org.apache.rocketmq.broker.longpolling.PullRequestHoldService.notifyMasterOnline(PullRequestHoldService.java:191)
   	at org.apache.rocketmq.broker.longpolling.PullRequestHoldServiceTest.notifyMasterOnlineTestWakesTheBucketsLeftAfterAConcurrentArrivalDrainedOneTest(PullRequestHoldServiceTest.java:145)
   ```
   
   Green run after the fix, and the CI-shaped verification (checkstyle + spotbugs) on the module:
   
   ```
   Tests run: 29, Failures: 0, Errors: 0, Skipped: 0   (PullRequestHoldServiceTest, PopLongPollingServiceTest, PopLiteLongPollingServiceTest)
   [INFO] You have 0 Checkstyle violations.
   [INFO] BugInstance size is 0
   [INFO] BUILD SUCCESS
   ```
   
   The new test `notifyMasterOnlineTestWakesTheBucketsLeftAfterAConcurrentArrivalDrainedOneTest` asserts both halves of the contract: the sweep does not throw, and the held requests of the other queues are still woken (their bucket is drained by the sweep) instead of being skipped by an aborted sweep. The broker module's full test suite is not run here - it does not complete on this machine for unrelated reasons - so only the affected package's test classes were executed, as reported above.
   
   Fixes #11441
   


-- 
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: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org