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:
SynchronizedCollection.forEach
would lock on the mutex
, which is exactly the returned collection
.
- 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.