unbridled-41 opened a new issue, #10988:
URL: https://github.com/apache/rocketmq/issues/10988

   ### Before Creating the Bug Report
   
   - [x] I found a bug, not just asking a question, which should be created in 
[GitHub Discussions](https://github.com/apache/rocketmq/discussions).
   - [x] I have searched the [GitHub 
Issues](https://github.com/apache/rocketmq/issues) and [GitHub 
Discussions](https://github.com/apache/rocketmq/discussions) of this repository 
and believe this is not a duplicate.
   - [x] I have confirmed that this bug belongs to the current repository, not 
other repositories of RocketMQ.
   
   ### Runtime platform environment
   
   - OS: Linux
   - Component: Broker (`ConsumerOffsetManager`)
   
   ### RocketMQ version
   
   - branch: develop
   - Git commit id: e348efa66
   
   ### JDK Version
   
   JDK 8
   
   ### Describe the Bug
   
   `ConsumerOffsetManager#commitOffset(clientHost, key, queueId, offset)` uses 
a non-atomic check-then-act when creating the per `topic@group` map:
   
   ```java
   ConcurrentMap<Integer, Long> map = this.offsetTable.get(key);
   if (null == map) {
       map = new ConcurrentHashMap<>(2);
       map.put(queueId, offset);
       this.offsetTable.put(key, map);      // last put wins, the other 
thread's map is dropped
   } else { ... }
   ```
   
   When two remoting threads commit offsets for two different queues of the 
same *new* `topic@group` concurrently (which is the normal situation right 
after a consumer group starts consuming a multi-queue topic, or while a pop 
consumer acks multiple queues), both threads see `null`, both build their own 
map, and the second `offsetTable.put` overwrites the first. The first queue's 
offset is silently lost until that queue commits again.
   
   Note that the same class already uses the correct idiom for pull offsets 
(`commitPullOffset` → `computeIfAbsent`), which makes the intent clear.
   
   Impact: a `queryOffset` for the lost queue returns `-1` in the window before 
the next commit, so a consumer reconnect/restart re-initializes that queue per 
`consumeFromWhere` (duplicate consumption or skipping to max). The same race 
re-arms after `removeOffset(group)`/`cleanOffsetByTopic` while the group is 
still consuming.
   
   ### Steps to Reproduce
   
   Deterministically with two threads calling `commitOffset(host, group, topic, 
0, i)` and `commitOffset(host, group, topic, 1, i)` concurrently for many fresh 
keys; some keys end up with only one queue's entry (assert `queryOffset(group, 
topic).size() == 2` fails).
   
   ### What Did You Expect to See?
   
   Both queue offsets are retained regardless of thread interleaving.
   
   ### What Did You See Instead?
   
   One thread's `ConcurrentHashMap` replaces the other's, dropping the queue 
offset it already contains.
   
   ### Additional Context
   
   Fix: replace the get/put pair with `offsetTable.computeIfAbsent(key, k -> 
new ConcurrentHashMap<>(2))` and put the queue offset into the returned map. I 
will submit a PR with a concurrency regression test.


-- 
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]

Reply via email to