Jason918 commented on a change in pull request #12654:
URL: https://github.com/apache/pulsar/pull/12654#discussion_r744246481
##########
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java
##########
@@ -474,7 +475,13 @@ public void registerListener(TopicName topicName,
TopicPolicyListener<TopicPolic
@Override
public void unregisterListener(TopicName topicName,
TopicPolicyListener<TopicPolicies> listener) {
- listeners.computeIfAbsent(topicName, k ->
Lists.newCopyOnWriteArrayList()).remove(listener);
Review comment:
The function passed to computeIfAbsent will only be executed when
topicName is NOT already in this `listeners`.
Consider the actually use case of the listener, we should not add a List
value to the `listeners` map, right?
There are actually two data structures that we should consider thread safe
issue.
One is the ConcurrentHashMap `listeners`, two operations we used here,
`get` and `remove` are both threadSafe.
The other `topicListeners ` is the value of `listeners`, and its type is
CopyOnWriteArrayList, with thread safe `remove` method.
One thing should be noticed is that it's `Collections.emptyList()` instead
of `topicListeners` in line 482. This operation is atomic guaranteed by
ConcurrentHashMap, So the key will be deleted if and only if the value list is
empty.
But it's all theoretical analysis above, any cases that will lead to thread
safe issue?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
