Java concurrency issue - Locks and Synchronize method

Question

I am working on Re entrant locks and trying to correlate it with Synchronize. However both these classes are giving me unexpected results. I am expecting the arrayList to have 0 to 9. but that value never comes in both these programs. Please suggest. With lock:

package Threads;

import java.util.ArrayList;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

public class Locking {

    Lock lock = new ReentrantLock(true);
    ArrayList<Integer> al = new ArrayList<Integer>();
    static int count = 0;

    public void producer() {
        lock.lock();
        count++;
        al.add(count);
        System.out.println(al);
        try {
            Thread.sleep(5000);
        } catch (InterruptedException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        } finally {
            lock.unlock();
        }
        // System.out.println("I came out of this block:"+Thread.currentThread().getName());
    }

    public void consumer() {
    }

    public static void main(String[] args) {
        // ExecutorService ex= Executors.newCachedThreadPool();
        ExecutorService ex = Executors.newFixedThreadPool(10);

        for (int i = 0; i < 10; i++) {
            ex.submit(new Runnable() {

                @Override
                public void run() {
                    try {
                        Thread.sleep(2000);
                    } catch (InterruptedException e) {
                        // TODO Auto-generated catch block
                        e.printStackTrace();
                    }
                    new Locking().producer();
                }

            });
        }
        ex.shutdown();
    }
}

With synchronize:

package Threads;

import java.util.ArrayList;
import java.util.Collections;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

public class LockwithSynchronize {

    ArrayList<Integer> al = new ArrayList<Integer>();
    static int count = 0;

    public synchronized void producer() {
        count++;
        al.add(count);
        System.out.println(al);
        try {
            Thread.sleep(5000);
            // System.out.println("I came out of this block:"+Thread.currentThread().getName());
        } catch (InterruptedException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    }

    public static void main(String[] args) {
        // ExecutorService ex= Executors.newCachedThreadPool();
        ExecutorService ex = Executors.newFixedThreadPool(10);

        for (int i = 0; i < 10; i++) {
            ex.submit(new Runnable() {
                @Override
                public void run() {
                    try {
                        Thread.sleep(2000);
                    } catch (InterruptedException e) {
                        // TODO Auto-generated catch block
                        e.printStackTrace();
                    }
                    new LockwithSynchronize().producer();
                }
            });
        }
        ex.shutdown();
    }

}

What output are you getting?

Krease
– Krease

2015-05-07 19:23:05 +00:00
Commented May 7, 2015 at 19:23 — Krease
– Krease, Commented May 7, 2015 at 19:23

Dathan · Accepted Answer · 2015-05-07 20:01:14Z

There are so many things wrong with this.

Firstly, you expect it to contain 0..9, but you call count++ before al.add(count), which means you should actually expect 1..10.

Since you're using a new LockWithSynchronize or Locking each time, there isn't actually any shared lock -- each instance gets its own lock, which never conflicts with any other lock, which means your count variable is completely unprotected. And you're likely to periodically get either a corrupted ArrayList due to add being called on multiple threads without synchronization, or ConcurrentModificationException or similar.

Try this:

public static void main(String [] args){

    ExecutorService ex= Executors.newFixedThreadPool(10);

    final Locking producer = new Locking();

    for (int i=0; i<10;i++)
    {
        ex.submit(new Runnable(){

            @Override
            public void run() {
                try {
                    Thread.sleep(2000);
                } catch (InterruptedException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }
                producer.producer();
            }

        });
    }

    ex.shutdown();
}

Note that instead of a new Locking every time, we're reusing the same instance, and what you expect now happens: you spawn ten threads very quickly, each of which wait for two seconds before trying to call producer(). One thread gets the lock, while the other nine block. The one that gets the lock then waits with the lock for five seconds before exiting, at which point the next thread gets the lock, waits for five seconds, and exits, etc. So this'll take close to a minute to run.

Similar modification fixes the other one, too.

BadZen · Accepted Answer · 2015-05-07 19:18:48Z

0

Your synchronize does not protect the static variable - synchonize only locks the monitor of the current this object when use like that. No produce() invocation will ever wait for any other in the code you posted. Lock on something shared - say LockWithProducer.class, for example.

answered May 7, 2015 at 19:18

BadZen

4,3953 gold badges30 silver badges55 bronze badges

1 Comment

BadZen Over a year ago

Also, what output are you expecting exactly? Do you really want one list for each LockWithSynchronize? Or are you trying to have all threads output to a single list?

Collectives™ on Stack Overflow

Java concurrency issue - Locks and Synchronize method

2 Answers 2

Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related