[Task] Fix thread safety of PendingAckHandleImpl
- Dominant language
- Java
- Stars
- 15.3k
- Forks
- 3.8k
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 160
Description
PendingAckHandleImpl seems to contain some thread safety issues:
individualAckOfTransaction uses org.apache.commons.collections4.map.LinkedMap which is not thread safe.
Another thread safety issue is the mutation of MutablePair instance. MutablePair is not thread safe.
HashMaps are also used as mutable state.
This state is mutated without a guarantee that the same thread is reading and writing the state or there's some other way to ensure consistency according to the Java Memory Model.
It seems that the intention would be use a single thread execution model to address this. There's a internalPinnedExecutor in
[PendingAckHandleImpl](https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/pendingack/impl/PendingAckHandleImpl.java) which is used to run some operations on the single executor thread.
However, the thread could possibly get switched multiple times in the call chain because of asynchronous method calls.
There's also some code where synchronized blocks are used as a way to ensure consistency such as
`synchronized (org.apache.pulsar.broker.transaction.pendingack.impl.PendingAckHandleImpl.this) {` and `synchronized (PendingAckHandleImpl.this) {`.
This is bad since the thread that gets synchronized is whatever thread is completing the asynchronous call. This might for example block threads from the bookkeeper client.
Contributor guide
Assessment
This issue has not been assessed yet.