BewareMyPower opened a new pull request, #24725: URL: https://github.com/apache/pulsar/pull/24725
Fixes #24724 ### Modifications Change `pendingIndividualBatchIndexAcks` to a `ConcurrentSkipListMap` and call `pollFirst()` in the loop to ensure concurrent `flush` call won't copy the reference to the `ConcurrentBitSetRecyclable` twice to `newMultiMessageAckCommon` in different threads, which recycles the object. ### Verifying this change It's hard to write a unit test because this race condition is hard to simulate. ### Documentation <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. --> - [ ] `doc` <!-- Your PR contains doc changes. --> - [ ] `doc-required` <!-- Your PR changes impact docs and you will update later --> - [x] `doc-not-needed` <!-- Your PR changes do not impact docs --> - [ ] `doc-complete` <!-- Docs have been already added --> ### Matching PR in forked repository PR in forked repository: <!-- ENTER URL HERE --> ### Additional information This PR is a simple fix on the issue that avoids refactorings. However, it might not be worth allowing `flush` to be called concurrently. It's hard to test the race condition and could send The advantage is just to avoid coarse grained lock on all methods that access the following fields: - `pendingIndividualAcks` - `pendingIndividualBatchIndexAcks` - `lastCumulativeAck` However, from my perspective, it's a pre-mature optimization that makes code error-prone. Because acquiring a lock for all `acknowledgeAsync` calls should not be a bottle neck of consumer side, whose time consuming tasks are mainly the business logic that handles messages and `receive` calls that have many lock acquirements as well. The use of `ConcurrentBitSetRecyclable` is also a pre-mature optimization that results to the bug described in #24724. As I've shared in https://lists.apache.org/thread/b5r13oz24y935p6o8tfwf578xk35wwpf, sharing a recyclable object among different threads is not a good practice, which introduces more overhead and bypasses the performance benefits of the thread-local stacks. There might be still other potential issues with the use of `ConcurrentBitSetRecyclable`, I might open another PR for the code refactoring. -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
