I have the following task :

Create Class called: ElementsProvider(int n, List elements) that will provide N random elements into given list. Elements Provider will be an thread. Try create 4 instances, each of this instance will add 1000 random elements into the list. start all instances at once and wait until they end. print list size.

And here is what is did ,

Main:

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

public class ElementsProvider implements Runnable{
    private final List<Integer> list;
    private final int n;

    public ElementsProvider(List<Integer> list, int n){
        this.list = list;
        this.n = n;
    }

    @Override
    public void run() {
        Random random = new Random();
        for (int i = 0; i < n; i  ) {
            list.add(random.nextInt());
        }
    }

    public static void main(String[] args) throws InterruptedException {
        List<Integer> list = new ArrayList<>();
        int n = 1000;
        ElementsProvider e1 = new ElementsProvider(list, n);
        ElementsProvider e2 = new ElementsProvider(list, n);
        ElementsProvider e3 = new ElementsProvider(list, n);
        ElementsProvider e4 = new ElementsProvider(list, n);

        Thread t1 = new Thread(e1);
        Thread t2 = new Thread(e2);
        Thread t3 = new Thread(e3);
        Thread t4 = new Thread(e4);

        t1.start();
        t2.start();
        t3.start();
        t4.start();

        t1.join();
        t2.join();
        t3.join();
        t4.join();

        System.out.println(list);
    }
}

Apparently I got that the task is not ok.

Feedback that I got is :

wrong, try to print list size, it will be different each time You run the program.

Can someone point me where I am mistaking please?

CodePudding user response：

You proposed this change in a comment on your original question, above:

@Override
public void run() {
    synchronized (ElementsProvider.class) {
        Random random = new Random();
        for (int i = 0; i < n; i  ) {
            list.add(random.nextInt());
        }
    }
}

O.K., That will ensure that your program always prints the correct answer, but it does so by making your program effectively single-threaded. When you put the entire body of the threads' run() method in a single synchronized block, you prevent them from running concurrently. But, running concurrently is the only reason to use threads.

You need to synchronize a smaller part of the code. The only variable that the threads share is the list. There is no reason for new Random() to be inside the synchronized block, and there is no reason for random.nextInt() to be inside it. The only thing that needs to be inside the synchronized block is the list.add() call.

CodePudding user response：

I'd add a static semaphore to the your ElementsProvider class:

public class ElementsProvider implements Runnable {
    private final List<Integer> list;
    private final int n;
    private static Semaphore semaphore = new Semaphore(1);

    public ElementsProvider(List<Integer> list, int n) {
        this.list = list;
        this.n = n;
    }

    @Override
    public void run() {
        Random random = new Random();
        List<Integer> l = new ArrayList<>(n); 
        for (int i = 0; i < n; i  ) {
            l.add(random.nextInt());
        }

        try {
            semaphore.acquire();
            System.out.println("Adding "   l.size()   " elements to list");
            list.addAll(l);
        } catch (Exception e) {
            e.printStackTrace();
        } finally {
            semaphore.release();
        }
    }
}