Update a shared variable from all the threads

Question

I am new to multithreading. I have a volatile variable currentPrimeNo and it will print the next prime number as implemented in run method for every new thread. But everytime I am getting currentPrimeNo as 0 for every thread. How should I keep the global variable currentPrimeNo updated?

public class Processor implements Runnable {
    private int id;
    private static volatile int currentPrimeNo = 0;

    public Processor(int id) {
        this.id = id;
    }

    @Override
    public void run() {
        System.out.println("Starting process id: " + id);
        currentPrimeNo = Utils.generateNextPrime(currentPrimeNo);
        System.out.println("Prime Number Associated with this thread is: " + currentPrimeNo);
        System.out.println("Completed process id: " + id);
    }

}

And the main class is:

public class MainClass {

    @SuppressWarnings("resource")
    public static void main(String[] args) {
        System.out.println("****This is where the project starts*****");
        Scanner reader = new Scanner(System.in);
        System.out.print("Enter number of processes you want to create: ");
        int n = reader.nextInt();
        ExecutorService executor = Executors.newFixedThreadPool(n);
        for(int i=1;i<=n; i++) {
            executor.submit(new Processor(i));
        }
        executor.shutdown();
        try {
            executor.awaitTermination(10, TimeUnit.MINUTES);
        } catch (InterruptedException e1) {
            e1.printStackTrace();
        }
        System.out.println("****This is where the project ends*****");
    }
}

Below is the generateNextPrime method from Util class:

public synchronized static int generateNextPrime(int currentPrime) {
        int nextPrime = 2;
        if (currentPrime <= 1) {
            return nextPrime;
        } else {
            for (int i = currentPrime + 1;; i++) {
                boolean notPrime = false;
                for (int j = 2; j < i; j++) {
                    if (i % j == 0) {
                        notPrime = true;
                        break;
                    }
                }
                if (notPrime == false) {
                    return i;
                }
            }
        }
    }

Below is the output I am getting:

****This is where the project starts*****

Enter number of processes you want to create: 4

Starting process id: 2

Starting process id: 3

Starting process id: 1

Starting process id: 4

Prime Number Associated with this thread is: 2

Completed process id: 4

Completed process id: 1

Prime Number Associated with this thread is: 2

Completed process id: 2

Prime Number Associated with this thread is: 2

Completed process id: 3

****This is where the project ends*****

How do you know that `Utils.generateNextPrime` isn't erroneously returning `0`? Have you tested it separately? — rgettman, Mar 15 '18 at 21:53
Yes I have tested it, it runs smoothly. If you pass it 0 it gives 2, if you pass it 2 it gives next prime number, i.e 3 and so on. — Piyush Chandra, Mar 15 '18 at 21:57
Please show the minimal yet complete example (i.e. a [mcve]) so other can reproduce your error. The code you've shown so far is not enough to reproduce the error. — rgettman, Mar 15 '18 at 22:05
Could you please share the internals of `generateNextPrime`? — Alexandre Dupriez, Mar 15 '18 at 22:17
That's correct, you need to believe me. It will make the code big and then hardly people will answer me. I have tested it again. Anyhow I will add it for now, later I will remove. — Piyush Chandra, Mar 15 '18 at 22:20
I am removing the generateNextPrime method to make the question small. — Piyush Chandra, Mar 15 '18 at 22:44

score 0 · Answer 1 · answered Mar 16 '18 at 03:04

After clarifying the problem:

a) getting zero as a result - well in fact it is not the case, I have actually run the code this time :) It returns total randomish result as expected, depending on the number of threads created. (Note that you use threads, not processes.) The reason for randomish result is that each thread instance starts from the value set by other thread instances. As execution order is not deterministic, the output is not deterministic either.

b) not getting prime numbers generated one after the another - this is because the calculation starts with multiple threads at the same time, and those threads work in parallel (that's what the pooled executor does).

To force all your tasks running sequentially, use a newSingleThreadExecutor.

// final ExecutorService executor = Executors.newFixedThreadPool(n); // this uses a pool
final ExecutorService executor = Executors.newSingleThreadExecutor(); // this is sequential 


public static void main(String[] args) throws InterruptedException {
    System.out.println("****This is where the project starts*****");
    final Scanner reader = new Scanner(System.in);
    System.out.print("Enter number of processes you want to create: ");
    final int n = reader.nextInt();
    // final ExecutorService executor = Executors.newFixedThreadPool(n); // this uses a pool
    final ExecutorService executor = Executors.newSingleThreadExecutor(); // this uses a single thread

    for(int i=1;i<=n; i++) {
        executor.submit(new Processor(i));
    }
    executor.awaitTermination(10, TimeUnit.MINUTES);
    System.out.println("****This is where the project ends*****");
}

The expected output is produced as:

****This is where the project starts***** Enter number of processes you want to create: 10 Starting process id: 1 Prime Number Associated with this thread is: 2 Completed process id: 1 Starting process id: 2 Prime Number Associated with this thread is: 3 Completed process id: 2 Starting process id: 3 Prime Number Associated with this thread is: 5 Completed process id: 3 Starting process id: 4 Prime Number Associated with this thread is: 7 Completed process id: 4 Starting process id: 5 Prime Number Associated with this thread is: 11 Completed process id: 5 Starting process id: 6 Prime Number Associated with this thread is: 13 Completed process id: 6 Starting process id: 7 Prime Number Associated with this thread is: 17 Completed process id: 7 Starting process id: 8 Prime Number Associated with this thread is: 19 Completed process id: 8 Starting process id: 9 Prime Number Associated with this thread is: 23 Completed process id: 9 Starting process id: 10 Prime Number Associated with this thread is: 29 Completed process id: 10

Note that as the execution is actually serialized, you will have no performance gain by using an executor (or separate execution threads) here.

The best problems which can benefit from parallel execution are problems where the inputs can be splitted, then processed by multiple threads in parallel, and finally composed back again. For example, converting a bitmap picture to black-and-white is a good problem for parallel execution, as the bitmap can be sliced into 8 pieces, and these pieces fed to 8 threads running in parallel. Finally once all threads are completed, the code can assembly the output to one picture, and benefit from the 8x faster execution.

Thanks. But the only problem with your code is the threads are not running in parallel. I want the threads to run in parallel. As you see in my output, it is fine, just the problem is I want them to be assigned different prime-numbers, I don't care which order the threads run. — Piyush Chandra, Mar 16 '18 at 03:19
Well, this comes from the problem you are trying to solve. You can generate arbitrary prime numbers using parallel threads. You cannot generate prime numbers depending on each other with parallel threads, since one thread needs the result of the other thread before it can start any calculation. Hence the problem of "calculating *all* prime numbers in *parallel*" is not possible. — Gee Bee, Mar 16 '18 at 12:07

Kavitha Karunakaran · Accepted Answer · 2018-03-17T12:41:54.393

Since you have not shared the code for generateNextPrime here, it would be a bit difficult to point out where exactly the code is failing.

There is an inherent problem associated with this.

Edit after Util.generateNextPrime() was added.

When we use volatile keyword, all threads would see the current value and not the cached value of the variable. But in your code, volatile variable is defined inside Runnable implementation. Thus, it doesn't server for the purpose. It is true that run method calls generateNextPrime and passes the volatile variable but what the called method actually sees and works on is a copy of the variable and not the exact variable (reading more on pass by value vs pass by reference will be beneficial to understand this better). The aim here is to have a single variable whose value should be altered by the generateNextPrime call which will be done by each thread while running.

I moved the currentPrimeNo definition to Util class so that all threads see only one variable (and not a copy of it) and that too the real-time value of the volatile variable. The method generateNextPrime() was also altered a bit for sake of compactness. The output necessarily need not be in same order as you will not know the order of invocation of the worker threads.

Here is the code:

public class Processor implements Runnable {
    private int id;

    public Processor(int id) {
        this.id = id;
    }

    @Override
    public void run() {
        System.out.println("Starting process id: " + id);
        int currentPrimeNo = Utils.generateNextPrime();
        System.out.println("Prime Number Associated with this thread " + id +" is: " + currentPrimeNo);
        System.out.println("Completed process id: " + id);
    }

}

public class Utils {

    private static volatile int currentPrime = 0;
    public static synchronized int generateNextPrime(){
        currentPrime++;
        if(currentPrime < 2){
            currentPrime = 2;
            return currentPrime;
        }
        for (int i = 2; i <currentPrime; i++) {
            if(currentPrime%i == 0) {
                currentPrime++;
                i=2;
            } else{
                continue;
            }
        }
        return currentPrime;
    }
}

Output seen while benchtesting

Sample 1:

****This is where the project starts*****
Enter number of processes you want to create: 4
Starting process id: 3
Starting process id: 1
Starting process id: 2
Starting process id: 4
Prime Number Associated with this thread 3 is: 2
Prime Number Associated with this thread 1 is: 7
Completed process id: 1
Prime Number Associated with this thread 2 is: 3
Completed process id: 2
Prime Number Associated with this thread 4 is: 5
Completed process id: 3
Completed process id: 4
****This is where the project ends*****

Sample 2:

****This is where the project starts*****
Enter number of processes you want to create: 6
Starting process id: 5
Starting process id: 1
Starting process id: 3
Starting process id: 2
Starting process id: 4
Prime Number Associated with this thread 2 is: 7
Prime Number Associated with this thread 4 is: 11
Completed process id: 4
Prime Number Associated with this thread 1 is: 3
Completed process id: 1
Prime Number Associated with this thread 5 is: 5
Completed process id: 5
Prime Number Associated with this thread 3 is: 2
Starting process id: 6
Completed process id: 2
Prime Number Associated with this thread 6 is: 13
Completed process id: 6
Completed process id: 3
****This is where the project ends*****

Thank you. This works and I understood what was the mistake. — Piyush Chandra, Mar 17 '18 at 17:20
In your fixed code, there is no need for `currentPrime` to be `volatile` anymore, as the entire access is guarded by `synchronized`. — Holger, Mar 20 '18 at 14:28

Thai · Answer 3 · 2018-03-16T18:15:41.697

When looking at the output, you can see that all 4 threads start before any computation. The prime number is calculated right after a thread starts, so it's very likely that all threads start with the same starting prime number (0), so finishes with the same ending number (2). As a result, the output you got makes sense.

The biggest problem with your code is that you execute computations in parallel, but expect the results in sequence. To achieve an output as you want, you can wrap the Utils.generateNextPrime(currentPrimeNo) method invocation in a synchronized block. This will make sure only one thread can take action on the prime value at a time.

Update 1: This is the output I got when running your code:

****This is where the project starts*****
Enter number of processes you want to create: 4
Starting process id: 2
Prime Number Associated with this thread is: 2
Completed process id: 2
Starting process id: 1
Prime Number Associated with this thread is: 3
Completed process id: 1
Starting process id: 4
Prime Number Associated with this thread is: 5
Completed process id: 4
Starting process id: 3
Prime Number Associated with this thread is: 7
Completed process id: 3
****This is where the project ends*****

Update 2: You can also change your Processor class like the following, without synchronizing the generateNextPrime method:

public class Processor implements Runnable {
    private static Object lock = new Object();
    private int id;
    private static volatile int currentPrimeNo = 0;

    public Processor(int id) {
        this.id = id;
    }

    @Override
    public void run() {
        System.out.println("Starting process id: " + id);
        synchronized (lock) {
            currentPrimeNo = generateNextPrime(currentPrimeNo);
        }
        System.out.println("Prime Number Associated with this thread is: " + currentPrimeNo);
        System.out.println("Completed process id: " + id);
    }
}

I have now made the method synchronized, still it is not giving the desired output. — Piyush Chandra, Mar 16 '18 at 17:21
Synchronizing the method doesn't help because each thread will be locked using a different Processor object. You can create an object right in MainClass and lock the method invocation using that object: synchronized(object) {/* your method invocation */}. — Thai, Mar 16 '18 at 17:48
Actually it helps. The generateNextPrime method is not declared in the Processor class. I have added the output I got in my answer. — Thai, Mar 16 '18 at 17:59
Thanks, it works now. But one question I have. When I put the method generateNextPrime in the Util class it doesn't work, but when I put it in the Processor class it works. Is it because Processor class is implementing Runnable? — Piyush Chandra, Mar 16 '18 at 18:46
Does that mean all thread safe methods have to be in the class implementing runnable or extending thread? — Piyush Chandra, Mar 16 '18 at 18:47
That has nothing to do with Runnable. The problem is when you synchronize the method, the argument is passed BEFORE threads compete for the lock. Thus, it's possible that more than one thread starts with the same original value. The behavior difference when the generateNextPrime is declared in the Utils and the Processor class may lie in the way the JVM caches variables. The best solution is what I showed in Update 2: Synchronizing the method before the argument is passed in. — Thai, Mar 17 '18 at 05:09

Update a shared variable from all the threads

3 Answers3