[GitHub] [bookkeeper] dlg99 commented on a diff in pull request #3365: Fix race condition.

[GitBox <gi...@apache.org>]

dlg99 commented on code in PR #3365:
URL: https://github.com/apache/bookkeeper/pull/3365#discussion_r911609146


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java:
##########
@@ -402,6 +402,7 @@ public void registerLedgerMetadataListener(long ledgerId, LedgerMetadataListener
                 }
             }
             synchronized (listenerSet) {
+                listenerSet = listeners.computeIfAbsent(ledgerId, k -> new HashSet<>());

Review Comment:
   with this
   ```
               Set<LedgerMetadataListener> listenerSet = listeners.computeIfAbsent(ledgerId, k -> new HashSet<>());
               synchronized (listenerSet) {
                   listenerSet = listeners.computeIfAbsent(ledgerId, k -> new HashSet<>());
                   listenerSet.add(listener);
               }
   ```
   it looks like the codes assume that between two calls of `listeners.computeIfAbsent` it may need to create a new instance (listeners can be modified/removed concurrently.) 
   
   so you are getting (or creating) one instance to synchronize on, meanwhile, listeners change, now synchronize locks on one instance of listenerSet while modifying another.
   
   so either the second call to computeIfAbsent is not needed or the locking needs to be done differently
   



-- 
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: issues-unsubscribe@bookkeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org