sijie commented on a change in pull request #5966: [Issue 5952] [pulsar-broker]
fix ConcurrentModificationException while load-report
serialization/updateLocalBrokerData
URL: https://github.com/apache/pulsar/pull/5966#discussion_r365510408
##########
File path:
pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/TopicStats.java
##########
@@ -131,7 +136,12 @@ public TopicStats add(TopicStats stats) {
}
} else {
for (String repl : stats.replication.keySet()) {
- this.replication.get(repl).add(stats.replication.get(repl));
+ if (this.replication.get(repl) != null) {
Review comment:
I am not convinced about the comment here. If the problem is a concurrent
modification to a HashMap, changing a HashMap to a ConcurrentMap doesn't
address the problem. We also need to change the way how we use a concurrent
hash map.
If there are concurrent accesses, line 143 can be accessed multiple times.
There will be multiple replicator stats added to the map. A newer stats can
replace a stats that might be already accessed by other threads. It results
inconsistent behavior when accessing stats object.
Same comment about at line 139. You check if a replication exists in line
139. but you didn't use the replication stats you got in line 139. But In line
140, you try to get the replication stats again. If there are concurrent
accesses, the replication might be removed at the time executing line 139 and
line 140. This results in NullPointerException.
This change is actually introducing more problems if there are concurrent
accesses. The code here should be rewritten as the followings:
```
ReplicatorStats oldStats = this.replication.get(repl);
if (oldStats == null) {
ReplicatorStats newStats = new ReplicatorStats();
oldStats = this.replication.putIfAbsent(repl, newStats);
if (oldStats == null) {
oldStats = newStats;
}
}
oldStats.add(stats.replication.get(repl));
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
