Synchronised Methods Do Not Make a Sequence of Calls Atomic

Every method of the list is synchronised, and two threads still both take the last item.

Making every method of a class synchronized makes each call safe on its own. It does nothing for a caller that needs two calls to happen together, such as checking that a list is not empty and then removing from it: another thread can run between the two.

A collection where every 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. It happens rarely and only under load, which is why it survives testing.

Each method is atomic; the sequence is not. This is the same check-then-act shape as checking a balance and then withdrawing from it, where another thread can act between the check and the withdrawal: the problem is where the operation's boundary really is, and here the boundary is in the caller, not in the list.

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); }

← → move through sections and cards