Possible bug in Collections.synchronizedCollection's forEach and removeIf?

Question

As a followup to my last post Is iteration via Collections.synchronizedSet(...).forEach() guaranteed to be thread safe? I will share my thoughts on what I think is a bug in the implementation to verify that it is indeed a bug.

We have a SynchronizedCollection<E> here, which can be obtained from calling Collections.synchronizedCollection(...), source from JDK:

public static <T> Collection<T> synchronizedCollection(Collection<T> c) {
    return new SynchronizedCollection<>(c);
}

---

static class SynchronizedCollection<E> implements Collection<E>, Serializable {
    private static final long serialVersionUID = 3053995032091335093L;

    final Collection<E> c;  // Backing Collection
    final Object mutex;     // Object on which to synchronize

    SynchronizedCollection(Collection<E> c) {
        this.c = Objects.requireNonNull(c);
        mutex = this;
    }

    SynchronizedCollection(Collection<E> 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 <T> T[] toArray(T[] a) {
        synchronized (mutex) {return c.toArray(a);}
    }

    public Iterator<E> 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<?> coll) {
        synchronized (mutex) {return c.containsAll(coll);}
    }
    public boolean addAll(Collection<? extends E> coll) {
        synchronized (mutex) {return c.addAll(coll);}
    }
    public boolean removeAll(Collection<?> coll) {
        synchronized (mutex) {return c.removeAll(coll);}
    }
    public boolean retainAll(Collection<?> 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<? super E> consumer) {
        synchronized (mutex) {c.forEach(consumer);}
    }
    @Override
    public boolean removeIf(Predicate<? super E> filter) {
        synchronized (mutex) {return c.removeIf(filter);}
    }
    @Override
    public Spliterator<E> spliterator() {
        return c.spliterator(); // Must be manually synched by user!
    }
    @Override
    public Stream<E> stream() {
        return c.stream(); // Must be manually synched by user!
    }
    @Override
    public Stream<E> parallelStream() {
        return c.parallelStream(); // Must be manually synched by user!
    }
    private void writeObject(ObjectOutputStream s) throws IOException {
        synchronized (mutex) {s.defaultWriteObject();}
    }
}

Now let's take a closer look at this code again:

    // Override default methods in Collection
    @Override
    public void forEach(Consumer<? super E> consumer) {
        synchronized (mutex) {c.forEach(consumer);}
    }
    @Override
    public boolean removeIf(Predicate<? super E> filter) {
        synchronized (mutex) {return c.removeIf(filter);}
    }

They are the only methods in this implementation that allow any code to be inserted.

Now I will hand control over to Effective Java: Item 67: Avid excessive synchronization

...
To avoid liveness and safety failures, never cede control to the client within a synchronized method or block. ... From the perspective of the class with the synchronized region, such methods are alien. ... Depending on what an alien method does, calling it from a synchronized region can cause exceptions, deadlocks or data corruption.
...

(Emphasis author)

It looks like the forEach and removeIf methods from Collections.synchronizedCollection violate exactly that rule, which I think is still valid in Java 8.

So we should be able to construct two SSCEE's that deadlock here.

One of the reasons I mark this as a possible bug, is that I am not able to reproduce it with the following code:

public class Java8BugSSCEE1 {
    public static void main(String[] args) {
        Collection<String> collection = Collections.synchronizedCollection(new HashSet<>());
        collection.add("Test");
        collection.forEach(str -> {
            synchronized (collection) {
                System.out.println("Obtained lock");
                collection.add(str + Integer.toHexString(ThreadLocalRandom.current().nextInt(16)));
            }
        });
        System.out.println("collection = " + collection);
    }
}

This prints [Test, TestX] as expected, where X in [0, f] in hex.

What I thought is happening is that:

1. SynchronizedCollection.forEach would lock on the mutex, which is exactly the returned collection.
2. Then inside my lambda I would try to obtain that same lock and fail.

However the last step doesn't happen, and this program executes normally.

So the final question, is there a bug or not in the Java 8 implementation?

Edit, I have new SSCEE's that perhaps does show a bug in the implementation.

public class Java8BugSSCEE1 {
    public static void main(String[] args) {
        Collection<String> collection = Collections.synchronizedCollection(new HashSet<>());
        collection.add("Test");
        collection.forEach(str -> {
            new Thread(() -> {
                collection.add(str + Integer.toHexString(ThreadLocalRandom.current().nextInt(16)));
            }).start();
        });
        System.out.println("collection = " + collection);
    }
}

I would expect this version to add an element, but instead it doesn't add anything.

Answer 1

It's not a bug. It's a feature. The collection returned by synchronizedCollection() has all its methods synchronized, that's all. It's the documented behavior.

Does that automatically make your code thread-safe? No.

Does that prevent any possibility of deadlock? No.

Should the caller understand what he's doing to prevent deadlocks and make its code thread-safe? Definitely.

Now why your code doesn't fail? Because locks are reentrant. You can have one thread lock the same object multiple times without problem. And that's what your code does. To have a deadlock, you need more than one thread.

EDIT:

regarding your last SSCCE: you're printing the collection without waiting for the adding thread to have finished its work. You thus have a race condition: the adding thread could have finished, or could have not started yet. The bug is in your code, not in the collection's code.