title: Listener / Observable implementation, synchronized vs concurrent collections
body: <p>When implementing thread-safe listeners I usually wonder which type of <code>Collection</code> is the best to hold the listeners. I've found three options so far.</p>
  
  <p>The standard <code>Observable</code> uses <code>synchronized</code> access to a simple <code>ArrayList</code>. Using a copy of the listeners is optional but as far as I can tell a good idea since it prevents problems like</p>
  
  <ul>
  <li>Listener removes itself in callback (<code>ConcurrentModificationException</code> - could iterate via indexed <code>for</code> loop in inverse order to prevent that)</li>
  <li>Executing external code inside synchronized block can block the entire thing.</li>
  </ul>
  
  <p>Implementation is unfortunately more than a few lines. <code>Collections.synchronizedList()</code> would not require <code>synchronized</code> in <code>removeListener</code> but that's not really worth it.</p>
  
  <pre><code>class ObservableList {
      private final List&lt;Listener&gt; listeners = new ArrayList&lt;Listener&gt;();
      public void addListener(Listener listener) {
          synchronized (listeners) {
              if (!listeners.contains(listener)) {
                  listeners.add(listener);
              }
          }
      }
      public void removeListener(Listener listener) {
          synchronized (listeners) {
              listeners.remove(listener);
          }
      }
      protected void notifyChange() {
          Listener[] copyOfListeners;
          synchronized (listeners) {
              copyOfListeners = listeners.toArray(new Listener[listeners.size()]);
          }
          // notify w/o synchronization
          for (Listener listener : copyOfListeners) {
              listener.onChange();
          }
      }
  }
  </code></pre>
  
  <p>But there are <code>Collection</code>s in <code>java.util.concurrent</code> that are inherently thread-safe and potentially more efficient since I would assume that their internal locking mechanism is optimized better than a simple <code>synchronized</code> block. Creating a copy for every notification is also quite expensive.</p>
  
  <p>Based on <code>CopyOnWriteArrayList</code></p>
  
  <pre><code>class ObservableCopyOnWrite {
      private final CopyOnWriteArrayList&lt;Listener&gt; listeners = new CopyOnWriteArrayList&lt;&gt;();
      public void addListener(Listener listener) {
          listeners.addIfAbsent(listener);
      }
  
      public void removeListener(Listener listener) {
          listeners.remove(listener);
      }
  
      protected void notifyChange() {
          for (Listener listener : listeners) {
              listener.onChange();
          }
      }
  }
  </code></pre>
  
  <p>Should do roughly what the first version does but with less copies. Adding / removing listeners is not a very frequent action which means that copies should not be very frequent as well.</p>
  
  <p>The version I usually use is based on <code>ConcurrentHashMap</code> which states for <code>.keySet()</code> which is used as <code>Set</code> here:</p>
  
  <blockquote>
    <p>The view's iterator is a "weakly consistent" iterator that will never throw ConcurrentModificationException, and guarantees to traverse elements as they existed upon construction of the iterator, and may (but is not guaranteed to) reflect any modifications subsequent to construction.</p>
  </blockquote>
  
  <p>Which means that iteration includes at least every listener that was registered when starting the iteration and may even include new ones that were added during iteration. Not sure about listeners that were removed. I do like this version because it's not copying and as simple to implement as <code>CopyOnWriteArrayList</code>.</p>
  
  <pre><code>class ObservableConcurrentSet {
      private final Set&lt;Listener&gt; listeners = Collections.newSetFromMap(new ConcurrentHashMap&lt;Listener, Boolean&gt;());
  
      public void addListener(Listener listener) {
          listeners.add(listener);
      }
  
      public void removeListener(Listener listener) {
          listeners.remove(listener);
      }
  
      protected void notifyChange() {
          for (Listener listener : listeners) {
              listener.onChange();
          }
      }
  }
  </code></pre>
  
  <p>Is a <code>ConcurrentHashMap</code> based implementation a good idea or did I overlook something here? Or is there an even better <code>Collection</code> to use?</p>