0

The code I've witten doesn't work as I expected.

static Integer sync = 1;

    static void m() throws Exception {
        synchronized (sync) {
            System.err.println("First");
            sync.notify();
            sync.wait(1000L);
            System.err.println("Second");
            System.err.println("Third");
        }
    }

    public static void main(String[] args) throws Exception {
        Runnable r = new Runnable() {
            @Override
            public void run() {
                try {
                    m();
                } catch (Exception ex) {
                    Logger.getLogger(IO.class.getName()).log(Level.SEVERE, null, ex);
                }
            }
        };
        Runnable t = new Runnable() {
            @Override
            public void run() {
                try {
                    m();
                } catch (Exception ex) {
                    Logger.getLogger(IO.class.getName()).log(Level.SEVERE, null, ex);
                }
            }
        };
        Thread th1 = new Thread(r);
        Thread th2 = new Thread(t);
        th1.run();
        th2.run();
   }

We have two threads which execute m()'s syncjronized statement. When the first thread executes one and come across the wait() it'll be added to the wait set. After this, the second thread is starting to execute the synchronized statement, and perform notify(). Since the output must be 

First
First
....

But actually it is

First
Second
Third
First
Second
Third

Why?

St.Antario
  • 26,175
  • 41
  • 130
  • 318
  • 5
    Well, first of all you're calling `Thread.run()` instead of `Thread.start()`, so your code is actually single threaded. – Kayaman Sep 04 '14 at 19:43
  • 1
    and do not expect to see First First if your synchronize on ``sync`` – Jean Logeart Sep 04 '14 at 19:44
  • @JeanLogeart Could you explain why? I really don't understand it (though I've read a corresponding clause from the JLS). – St.Antario Sep 04 '14 at 19:46
  • Don't synchronize on the `Integer` 1 object. They are effectively interned, so an unrelated piece of code that does the same thing would synchronize on the *same* object. Use `new Object()` instead. – Stuart Marks Sep 05 '14 at 03:56

3 Answers3

4

First of all, your program is not creating any threads. You must call th1.start() and th2.start() to create threads.

t.start() is the method that the library provides for your code to call when you want to start a thread. run() is the method that you provide for the library to call in the new thread. Your run() method defines what the thread will do. IMO, run() was a really misleading name.

Second, notify() and wait() don't do what it looks like you think they will do. In particular, sync.notify() will not do anything at all if there are no other threads currently in sync.wait().

The correct way to use notify() and wait() is, one thread does this:

synchronized(lock) {
    while (! someCondition()) {
         lock.wait()
    }
    doSomethingThatRequiresSomeConditionToBeTrue();
}

The other thread does this

synchronized(lock) {
    doSomethingThatMakesSomeConditionTrue();
    lock.notify();
}

When you use this pattern, no thread should ever change the result of someCondition() except from inside a synchronized(lock) block.

Solomon Slow
  • 25,130
  • 5
  • 37
  • 57
1

Firstly, To actually create new threads please use

th1.start() th2.start()

inplace of run() , which is just a regular method call on the thread object.

Secondly, it is possible that the second thread 'th2' did not start running by the time 1000 ms was fninshed , so the first thread finished wait(1000) and executed the remainging lines of code.

if you want the output like so :

first
first
second
third
second
third

then remove the time interval for wait() which will make the threads wait until notified. as in :

static void m() throws Exception {
        synchronized (sync) {
            System.err.println("First");
            sync.notify();
            sync.wait();
            System.err.println("Second");
            System.err.println("Third");
        }
    }
vreddy
  • 173
  • 1
  • 9
1

  1. Use .start() instead of run() to add runables to the queue instead of running them immediately

  2. Documentation says that wait with timeout waits for any notify on this object or the timeout. In your case when runnables are being executed one by one it goes:

    • r: First
    • r: waits 1000ms and try to get lock
    • r: it already have access to lock object (exactly this code got lock) so continue
    • r: Second
    • r: Third
    • t: First, and so on ...

PS. calling run() and not setting timeout will cause deadlock on t's wait, cause it already has the object but will wait never be notified about it.

Hope this helps.

damienix
  • 6,463
  • 1
  • 23
  • 30