2
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 2

7

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.

1

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());
        }
    }
}
1
  • 1
    When you wait for the completion of all threads, you don’t need an additional synchronization for printing the final result.
    – Holger
    Feb 24, 2021 at 12:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.