CODEWITHSUNDEEP
javamultithreadingsynchronization
Wesley.C
Wesley.C

Reputation: 75

Synchronized keyword doesn't work

package threadShareResource1;

public class NonSynchro1 {
    private int sum = 0;

    public static void main(String[] args) {
        NonSynchro1 n = new NonSynchro1();
        n.task();
        System.out.println(n.getSum());
    }

    public synchronized void sumAddOne(){
        sum++;
    }

    public void task(){
        for (int i = 0; i < 100; i++) {
            new Thread(new Runnable(){
                @Override
                public void run() {
                    sumAddOne();
                }
            }).start();

        /*  try {
                Thread.sleep(10);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }   */
        }
    }
    public int getSum() {
        return sum;
    }
}

Without the commented part of code, the program has data corruption, which is not 100 every time I run it. But I thought the synchronized keyword should acquires a lock on the sumAddOne method, which is the critical region of my program, allowing one thread accessing this method every time.

I've try to use ExecutorService as well, but it doesn't give 100 all the runs.

public void task(){
    ExecutorService s = Executors.newCachedThreadPool();

    for (int i = 0; i < 100; i++) {
        s.execute(new Thread(new Runnable(){
            @Override
            public void run() {
                sumAddOne();
            }
        }));
    }
    s.shutdown();

    while(!s.isTerminated()){}
}

Upvotes: 2

Views: 451

Answers (3)

Kenney
Kenney

Reputation: 9093

Just for completeness sake: Here's a solution showing how the original program can be made to wait for all threads to finish by joining them:

        for (Thread t : n.task())
            try {
                t.join();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }

which requires task to return the threads it creates. In this case we don't need to complicate things with caching managers or collections: a simple array will do. Here's the complete class:

public class TestSynchro1 {
    private int sum = 0;

    public synchronized void sumAddOne() {
        sum++;
    }

    public Thread[] task(int n) {
        Thread[] threads = new Thread[n];
        for (int i = 0; i < n; i++) {
            (threads[i] = new Thread(new Runnable() {
                @Override
                public void run() {
                    sumAddOne();
                }
            })).start();
        }
        return threads;
    }

    public static void main(String[] args) {
        TestSynchro1 n = new TestSynchro1();
        for (Thread t : n.task(100))
            try {
                t.join();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        System.out.println(n.sum);
    }
}

Upvotes: 0

Jaroslaw Pawlak
Jaroslaw Pawlak

Reputation: 5578

Synchronised keyword is used correctly, the problem is that you are not waiting for the threads to finish. Here is a possible solution:

public class NonSynchro1 {

    private static final ExecutorService executorService = Executors.newCachedThreadPool();

    private int sum = 0;

    public static void main(String[] args) {
        NonSynchro1 n = new NonSynchro1();
        n.task();
        System.out.println(n.getSum());
        executorService.shutdown();
    }

    public synchronized void sumAddOne() {
        sum++;
    }

    public void task() {
        List<Callable<Object>> callables = new ArrayList<>();
        for (int i = 0; i < 100; i++) {
            callables.add(() -> {
                sumAddOne();
                return null;
            });
        }

        List<Future<Object>> futures;
        try {
            futures = executorService.invokeAll(callables);
        } catch (InterruptedException e) {
            throw new RuntimeException(e);
        }

        futures.forEach(future -> {
            try {
                future.get();
            } catch (ExecutionException | InterruptedException e) {
                throw new RuntimeException(e);
            }
        });
    }

    public int getSum() {
        return sum;
    }

}

First we create a list of callables - a list of functions that will be executed in parallel.

Then we invoke them on the executor service. newCachedThreadPool I have used here, by default has 0 threads, it will create as many as necessary to execute all passed callables, the threads will be killed after being idle for a minute.

Finally, in the for-each loop we resolve all futures. get() call will block until the function was executed by the executor service. It will also throw exception if it was thrown inside the function (without calling get() you would not see such exception at all).

Also, it is a good idea to shutdown the executor service when you want to terminate the program gracefully. In this case, it is just executorService.shutdown() at the end of main method. If you don't do this, the program will terminate after a minute when idle threads are killed. However, if different executor service, threads might not be killed when idle, in which case the program would never terminate.

Upvotes: 3

modal_dialog
modal_dialog

Reputation: 743

In Task(), you start 100 threads (which is a lot) and each one is to add 1 to sum.

But when Task is done all you know is that 100 threads are in some process of having started. You don't block before calling println(), so how do you know all the threads have completed?

The sleep probably "prevents the corruption" just because it gives the system time to finish launching all the threads.

Beyond that you are using Synchronized correctly. Any place multiple threads may write to the same variable you need it and, in general (simplifying), you don't need it if you are only reading.

Upvotes: 4

Related Questions