title: Possible bug in Collections.synchronizedCollection's forEach and removeIf?
body: <p>As a followup to my last post <a href="https://stackoverflow.com/questions/23450227/is-iteration-via-collections-synchronizedset-foreach-guaranteed-to-be-thr">Is iteration via Collections.synchronizedSet(...).forEach() guaranteed to be thread safe?</a> I will share my thoughts on what I think is a bug in the implementation to verify that it is indeed a bug.</p>
  
  <p>We have a <code>SynchronizedCollection&lt;E&gt;</code> here, which can be obtained from calling <code>Collections.synchronizedCollection(...)</code>, source from JDK:</p>
  
  <pre><code>public static &lt;T&gt; Collection&lt;T&gt; synchronizedCollection(Collection&lt;T&gt; c) {
      return new SynchronizedCollection&lt;&gt;(c);
  }
  </code></pre>
  
  <hr>
  
  <pre><code>static class SynchronizedCollection&lt;E&gt; implements Collection&lt;E&gt;, Serializable {
      private static final long serialVersionUID = 3053995032091335093L;
  
      final Collection&lt;E&gt; c;  // Backing Collection
      final Object mutex;     // Object on which to synchronize
  
      SynchronizedCollection(Collection&lt;E&gt; c) {
          this.c = Objects.requireNonNull(c);
          mutex = this;
      }
  
      SynchronizedCollection(Collection&lt;E&gt; c, Object mutex) {
          this.c = Objects.requireNonNull(c);
          this.mutex = Objects.requireNonNull(mutex);
      }
  
      public int size() {
          synchronized (mutex) {return c.size();}
      }
      public boolean isEmpty() {
          synchronized (mutex) {return c.isEmpty();}
      }
      public boolean contains(Object o) {
          synchronized (mutex) {return c.contains(o);}
      }
      public Object[] toArray() {
          synchronized (mutex) {return c.toArray();}
      }
      public &lt;T&gt; T[] toArray(T[] a) {
          synchronized (mutex) {return c.toArray(a);}
      }
  
      public Iterator&lt;E&gt; iterator() {
          return c.iterator(); // Must be manually synched by user!
      }
  
      public boolean add(E e) {
          synchronized (mutex) {return c.add(e);}
      }
      public boolean remove(Object o) {
          synchronized (mutex) {return c.remove(o);}
      }
  
      public boolean containsAll(Collection&lt;?&gt; coll) {
          synchronized (mutex) {return c.containsAll(coll);}
      }
      public boolean addAll(Collection&lt;? extends E&gt; coll) {
          synchronized (mutex) {return c.addAll(coll);}
      }
      public boolean removeAll(Collection&lt;?&gt; coll) {
          synchronized (mutex) {return c.removeAll(coll);}
      }
      public boolean retainAll(Collection&lt;?&gt; coll) {
          synchronized (mutex) {return c.retainAll(coll);}
      }
      public void clear() {
          synchronized (mutex) {c.clear();}
      }
      public String toString() {
          synchronized (mutex) {return c.toString();}
      }
      // Override default methods in Collection
      @Override
      public void forEach(Consumer&lt;? super E&gt; consumer) {
          synchronized (mutex) {c.forEach(consumer);}
      }
      @Override
      public boolean removeIf(Predicate&lt;? super E&gt; filter) {
          synchronized (mutex) {return c.removeIf(filter);}
      }
      @Override
      public Spliterator&lt;E&gt; spliterator() {
          return c.spliterator(); // Must be manually synched by user!
      }
      @Override
      public Stream&lt;E&gt; stream() {
          return c.stream(); // Must be manually synched by user!
      }
      @Override
      public Stream&lt;E&gt; parallelStream() {
          return c.parallelStream(); // Must be manually synched by user!
      }
      private void writeObject(ObjectOutputStream s) throws IOException {
          synchronized (mutex) {s.defaultWriteObject();}
      }
  }
  </code></pre>
  
  <p>Now let's take a closer look at this code again:</p>
  
  <pre><code>    // Override default methods in Collection
      @Override
      public void forEach(Consumer&lt;? super E&gt; consumer) {
          synchronized (mutex) {c.forEach(consumer);}
      }
      @Override
      public boolean removeIf(Predicate&lt;? super E&gt; filter) {
          synchronized (mutex) {return c.removeIf(filter);}
      }
  </code></pre>
  
  <p>They are the <strong>only</strong> methods in this implementation that allow <strong>any code</strong> to be inserted.</p>
  
  <p>Now I will hand control over to <em>Effective Java: Item 67: Avid excessive synchronization</em></p>
  
  <blockquote>
    <p>...<br>
    <strong>To avoid liveness and safety failures, never cede control to the client within  a synchronized method or block.</strong> ... From the perspective of the class with the synchronized region, such methods are <em>alien</em>. ... Depending on what an alien method does, calling it from a synchronized region can cause exceptions, deadlocks or data corruption.<br>
    ...</p>
  </blockquote>
  
  <p>(Emphasis author)</p>
  
  <p>It looks like the <code>forEach</code> and <code>removeIf</code> methods from <code>Collections.synchronizedCollection</code> violate exactly that rule, which I think is still valid in Java 8.</p>
  
  <p>So we should be able to construct two SSCEE's that deadlock here.</p>
  
  <p>One of the reasons I mark this as a <em>possible bug</em>, is that I am not able to reproduce it with the following code:</p>
  
  <pre><code>public class Java8BugSSCEE1 {
      public static void main(String[] args) {
          Collection&lt;String&gt; collection = Collections.synchronizedCollection(new HashSet&lt;&gt;());
          collection.add("Test");
          collection.forEach(str -&gt; {
              synchronized (collection) {
                  System.out.println("Obtained lock");
                  collection.add(str + Integer.toHexString(ThreadLocalRandom.current().nextInt(16)));
              }
          });
          System.out.println("collection = " + collection);
      }
  }
  </code></pre>
  
  <p>This prints <code>[Test, TestX]</code> as expected, where <code>X in [0, f]</code> in hex.</p>
  
  <p>What I thought is happening is that:</p>
  
  <ol>
  <li><code>SynchronizedCollection.forEach</code> would lock on the <code>mutex</code>, which is exactly the returned <code>collection</code>.</li>
  <li>Then inside my lambda I would try to obtain that same lock and fail.</li>
  </ol>
  
  <p>However the last step doesn't happen, and this program executes normally.</p>
  
  <p>So the final question, is there a bug or not in the Java 8 implementation?</p>
  
  <p><strong>Edit</strong>, I have new SSCEE's that perhaps does show a bug in the implementation.</p>
  
  <pre><code>public class Java8BugSSCEE1 {
      public static void main(String[] args) {
          Collection&lt;String&gt; collection = Collections.synchronizedCollection(new HashSet&lt;&gt;());
          collection.add("Test");
          collection.forEach(str -&gt; {
              new Thread(() -&gt; {
                  collection.add(str + Integer.toHexString(ThreadLocalRandom.current().nextInt(16)));
              }).start();
          });
          System.out.println("collection = " + collection);
      }
  }
  </code></pre>
  
  <p>I would expect this version to add an element, but instead it doesn't add anything.</p>