clebertsuconic commented on code in PR #4972:
URL: https://github.com/apache/activemq-artemis/pull/4972#discussion_r1644812334


##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/postoffice/impl/SimpleAddressManager.java:
##########
@@ -100,6 +104,11 @@ public boolean addBinding(final Binding binding) throws 
Exception {
       if (nameMap.putIfAbsent(binding.getUniqueName(), bindingAddressPair) != 
null) {
          throw ActiveMQMessageBundle.BUNDLE.bindingAlreadyExists(binding);
       }
+      directBindingMap.compute(binding.getAddress(), (key, value) -> {
+         Collection<Binding> bindingList = value == null ? new ArrayList<>() : 
value;
+         bindingList.add(binding);

Review Comment:
   Maybe I'm being naive... but as far as I remember.. the ArrayList itself is 
not supposed to be used from multiple threads.
   
   The compute will do a replay, but you could still call the compute from 
multiple threads.
   
   I feel like you should have:
   
   ```
       synchronized(bindingList) {
           bindingsList.add(binding);
       }
   ```
   
   
   Or another data structure to be used here.
   
   
   similarly on the remove the queue, it should use a synchronized or use a 
concurrent equivalent.



-- 
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: gitbox-unsubscr...@activemq.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org
For additional commands, e-mail: gitbox-h...@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact


Reply via email to