apache / apache/pulsar

[Task] Fix thread safety of PendingAckHandleImpl

Open
#21,304 4 comments 0 reactions 1 assignee Claimed by @dao-jun View on GitHub
area/transaction category/reliability
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.