Why Synchronizing Every Method Did Not Make the Class Thread-Safe

A collection where every single method is synchronised, used by two threads, and two threads both take the last item.

java

List<Job> queue = Collections.synchronizedList(new ArrayList<>());

if (!queue.isEmpty()) {
    Job next = queue.remove(0);
    process(next);
}

isEmpty() is synchronised and remove(0) is synchronised, and the gap between them is not. Two threads can both pass line 3 when one item is left, and the second remove throws IndexOutOfBoundsException.

Each method is atomic; the sequence is not. Synchronising every method makes each call safe on its own and says nothing about a caller that needs two of them to happen together.

Either hold the lock across the whole sequence, or use a method that does the whole thing in one step.

java

synchronized (queue) {
    if (!queue.isEmpty()) { process(queue.remove(0)); }
}

// or, better
Job next = concurrentQueue.poll();
if (next != null) { process(next); }