-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Description
Search before reporting
- I searched in the issues and found nothing similar.
Read release policy
- I understand that unsupported versions don't get bug fixes. I will attempt to reproduce the issue on a supported version of Pulsar client and Pulsar broker.
User environment
master
Issue Description
The properties aren't accessed in a thread safe way in org.apache.bookkeeper.mledger.impl.ManagedCursorImpl#removeProperty
pulsar/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
Lines 521 to 534 in 3fb52c5
| @Override | |
| public boolean removeProperty(String key) { | |
| if (lastMarkDeleteEntry != null) { | |
| LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet(this, last -> { | |
| Map<String, Long> properties = last.properties; | |
| if (properties != null && properties.containsKey(key)) { | |
| properties.remove(key); | |
| } | |
| return last; | |
| }); | |
| return true; | |
| } | |
| return false; | |
| } |
The way to modify properties should be implemented in a similar way as it is in the putProperty method.
pulsar/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
Lines 502 to 519 in 81aff30
| @Override | |
| public boolean putProperty(String key, Long value) { | |
| if (lastMarkDeleteEntry != null) { | |
| LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet(this, last -> { | |
| Map<String, Long> properties = last.properties; | |
| Map<String, Long> newProperties = properties == null ? new HashMap<>() : new HashMap<>(properties); | |
| newProperties.put(key, value); | |
| MarkDeleteEntry newLastMarkDeleteEntry = new MarkDeleteEntry(last.newPosition, newProperties, | |
| last.callback, last.ctx); | |
| newLastMarkDeleteEntry.callbackGroup = last.callbackGroup; | |
| return newLastMarkDeleteEntry; | |
| }); | |
| return true; | |
| } | |
| return false; | |
| } |
The Map implementation used for the properties field of org.apache.bookkeeper.mledger.impl.ManagedCursorImpl.MarkDeleteEntry should be immutable to avoid concurrency issues.
Error messages
Reproducing the issue
This issue was found by reading the code. Since it's a concurrency issue, it would require a concurrency test to reproduce.
Additional information
No response
Are you willing to submit a PR?
- I'm willing to submit a PR!