is the locked object of synchronized wrong?

Viewed 98
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?

2 Answers

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.

You have two errors here. First of all, you don't wait until thread have finished their work. Second, you still need to synchronize the access to the list:

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

        List<Thread> threads = new ArrayList<>();


        for (int i = 0; i < 100; i++) {
            Thread t = new Thread(r);
            t.start();
            threads.add(t);
        }

        for (Thread t: threads) {
            t.join();
        }

        synchronized(r.list) {
            System.out.println(r.list.size());
        }
    }
}
Related