4

I'm using multiple threads in my application. Basically I have a combo box and upon selecting Inbox, p1 resumes and p2 is suspended and upon selecting Send, p2 starts and p1 stops. Below is the code (I'm sure it's not perfect) 

public void modifyText(ModifyEvent e) {
                if (combo.getText().equals("Inbox"))
                {
                    synchronized(p2) 
                    {
                        p2.cont = false;
                    }
                    table.removeAll();
                    synchronized(p1)
                    {
                        p1.cont = true;
                        p1.notify();
                    }
                }


                else if (combo.getText().equals("Sent"))
                {
                    synchronized(p2) 
                    {
                        p1.cont = false;
                    }
                    table.removeAll();
                    synchronized(p1)
                    {
                        p2.cont = true;
                        p2.notify();
                    }
                }
            }
        });

and for P1 and P2 I have this inside their while loops:

synchronized (this) {
            while (cont == false)
                try {
                    wait();
                } catch (Exception e) {
                }
        }

... As it is it's now working (I'm a beginner to threads). On pressing Sent in the combo box, I get an IllegalStateMonitorException. Could anyone help me solve the problem plz?

Thanks and regards, Krt_Malta

4

3 回答 3

6

the problem is here:

synchronized(p1)
{
    p2.cont = true;
    p2.notify();
}

You are doing p2.notify() when you haven't got a lock on p2 (you must hold the monitor to call notify on it). Change synchronized(p1) to synchronized(p2). Additionally, you need to reverse the other synchronized clause as well which is also faulty. So, as an example:

synchronized(p1) 
{
    p1.cont = false;
    // p1.notify(); <- do you need this here?
}
table.removeAll();
synchronized(p2)
{
    p2.cont = true;
    p2.notify();
}

Additionally, your other code is a bit wrong too, it's very bad practice to lock inside an entire loop, make it a bit more atomic.

while (!cont) {
    synchronized (this) {
       try {
           wait();
       } catch (Exception e) {
       }
    }
}

Additional optimisation, avoid synchronised if possible:

if (p1.cont) {
   synchronized(p1) 
   {
       p1.cont = false;
       // p1.notify(); <- do you need this here?
   }
}
table.removeAll();

if (!p2.cont) {
   synchronized(p2)
   {
       p2.cont = true;
       p2.notify();
   }
}

Make the cont field volatile here, and mirror for the other part of the if statement as appropriate.

Edit: looking back on this and battling with a concurrency bug I was recently facing, anyone implementing this pattern might face a problem with an infinite wait, if an object being locked & half-locked on is being looked at by the conditional of the while loop (this is because there is a gap where the state can change between evaluating the conditional and the imposition of the wait statement). In this case, place the synchronized block on the outside of the loop.

于 2010-03-16T18:01:59.823 回答
0

在这段代码中

                synchronized(p1)
                {
                    p2.cont = true;
                    p2.notify();
                }

您正在同步p1但调用notify()on p2，这会导致异常。

于 2010-03-16T18:02:43.257 回答
-1

您不能在 awt 事件调度线程中等待，否则会阻碍您的整个应用程序。阅读http://en.wikipedia.org/wiki/Event_dispatching_thread

此外，除非您真的知道自己在做什么，否则您不应该使用原始线程。查看http://java.sun.com/javase/6/docs/api/java/util/concurrent/package-summary.html并阅读Executors

于 2010-03-16T18:04:59.733 回答