CODEWITHSUNDEEP
javamultithreadingwaitsynchronized
Elad
Elad

Reputation: 51

synchronized method while using wait()

I ran the following code:

class Counter extends Thread {

 static int i=0;
//method where the thread execution will start
public void run(){
    //logic to execute in a thread

    while (true) {
        increment();
    }
}

public synchronized void increment()  {
    try {
        System.out.println(this.getName() + " " +  i++);
        wait(1000);
        notify();
    } catch (InterruptedException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

}
//let’s see how to start the threads
public static void main(String[] args){
    Counter c1 = new Counter();
    Counter c2 = new Counter();
    c1.setName("Thread1");
    c2.setName("Thread2");
    c1.start();
    c2.start();
}
}

The result of this code was (added line numbers):

1: Thread1 0
2: Thread2 1
3: Thread2 2
4: Thread1 3
5: Thread2 4
6: Thread1 4
7: Thread1 5
8: Thread2 6
stopping...

Since increment method is synchronized and since it contains wait(1000) I didnt expect: 1. Thread2 to print 2 consecutive prints: lines 2,3 I expected the threads to interleave their prints 2. on lines 5,6 the i remains 4.

could anyone give me an explanation for this?

Upvotes: 5

Views: 6172

Answers (3)

raphaëλ
raphaëλ

Reputation: 6523

This is probably the code you are looking for

class Counter implements Runnable {

    static int i = 0;
    private Lock lock;
    private Condition condition;

    public Counter(Lock lock, Condition condition) {

        this.lock = lock;
        this.condition = condition;
    }


    public void run() {
        while (true) {
            lock.lock();
            try {
                condition.await(1, TimeUnit.SECONDS);
                System.out.append(Thread.currentThread().getName()).append(" ").println(i++);
                condition.signalAll();
            } catch (InterruptedException e) {
                throw new RuntimeException(e);
            }
        }
    }

    public static void main(String[] args) {
        Lock lock = new ReentrantLock(true);
        Condition condition = lock.newCondition();
        Executor e = Executors.newFixedThreadPool(2);
        e.execute(new Counter(lock, condition));
        e.execute(new Counter(lock, condition));

    }
}

Upvotes: 0

Tudor
Tudor

Reputation: 62439

Synchronized instance methods like this:

public synchronized void foo() { 
    ... 
}

are roughly equivalent to:

public void foo() {
   synchronized(this) {
       ...
   }
}

Do you see the problem here? The synchronization is done on the current instance.

Since you are creating two separate Thread objects each increment method will synchronize on a different object, thus rendering the lock useless.

You should either make your increment method static (thus the locking is done on the class itself) or use a static lock object:

private static final Object locker = new Object();

public void foo() {
   synchronized(locker) {
       ...
   }
}

And one final advice: the preferred way to create a thread in java is by implementing Runnable, not extending Thread.

Upvotes: 9

Steve Townsend
Steve Townsend

Reputation: 54138

You are only synchronizing at the instance level. To synchronize across all Counter instances you need the increment method to be static as well as synchronized.

As it stands all your threads run freely, concurrent with each other, because they share no synchronization mechanism.

Upvotes: 1

Related Questions