java - is the locked object of synchronized wrong?

Question

public class UnsafeCol implements Runnable{
    List<String> list = new ArrayList<>();
    @Override
    public  void run() {
        synchronized (list) {
            list.add(Thread.currentThread().getName());
        }

    }

    public static void main(String[] args) {
        UnsafeCol r = new UnsafeCol();
        for (int i = 0; i < 100; i++) {
            new Thread(r).start();
        }
        System.out.println(r.list.size());
    }
}

output: 84
description: I always get the incorrect size, but I have used synchronized to lock the list,why?

question from:https://stackoverflow.com/questions/65842557/is-the-locked-object-of-synchronized-wrong

Answer 1

You're starting all the threads... but you're not waiting for them all to finish before you print out the size.

To do that, you'd want to keep a List<Thread> for the threads you've started, then call join on each of them (in a separate loop).

I'd also recommend keeping all access to the list synchronized - including your final printout of the size. It may be unnecessary here, but I tend to think that "obviously correct but possibly marginally inefficient" is better than "requires spec checking to validate for correctness" when it comes to most threading code.

So you'd have something like this:

import java.util.ArrayList;
import java.util.List;

class UnsafeCol implements Runnable {
    private final List<String> list = new ArrayList<>();
    @Override
    public void run() {
        synchronized (list) {
            list.add(Thread.currentThread().getName());
        }
    }

    public int size() {
        synchronized (list) {
            return list.size();
        }
    }
}

public class Test {
    public static void main(String[] args) throws InterruptedException {
        UnsafeCol col = new UnsafeCol();
        List<Thread> threads = new ArrayList<>();

        // Start 100 threads
        for (int i = 0; i < 100; i++) {
            Thread thread = new Thread(col);
            thread.start();
            threads.add(thread);
        }

        // Wait for all the threads to start
        for (Thread thread : threads) {
            thread.join();
        }

        // Print out the size
        System.out.println(col.size());
    }
}

(I've separated this into two classes for clarity to show how you'd prevent unsynchronized access to the list.)

Personally I prefer to synchronize on an object whose sole purpose is synchronization, but that's a slightly separate matter.