External call to synchronized function held/locked

The Following class DoStuff starts a thread and syncs to protect the listener object from being accessed when null.

Now when accessing the DoStuff class function setOnProgressListener() externally I'm having issues because the call is getting held for a long time before it exits the function call. I'm not sure why this happens? I seems as if the synchronization has queued up a lot of calls? Any input on this would help!

I'm essentially passing null to the listener because I no longer wish to get updated for this status. I do this as part of my process to kill the DoStuff Thread.

Thanks!

public c开发者_如何学JAVAlass DoStuff extends Runnable
{
    Object MUTEX = new Object();
    private OnProgressListener mOnProgressListener  = null;

    public DoStuff()
    {
        new Thread(this).start();
    }

    public void setOnProgressListener( OnProgressListener onProgressListener )
    {
        synchronized (MUTEX) 
        {
            mOnProgressListener = onProgressListener;
        }
    }

    private void reportStatus( int statusId )
    {
        synchronized (MUTEX) 
        {
            if (null != mOnStatusListener)
            {
                mOnStatusListener.setStatusMessage(new OnStatusEvent(this, statusId));
            }
        }
    }

    // this is the run of a thread
    public void run()
    {
        int status = 0;
        do
        {
            // do some work and report the current work status
            status = doWork();
            reportStatus( status );
        } while(true);
    }
}

You should use wait/notify. here is sample;

public class DoStuff {
    Object MUTEX = new Object();
    String data = null;

    public void setData(String data) {
        synchronized (MUTEX) {
            this.data = data;
            System.out.println(Thread.currentThread());
            MUTEX.notifyAll();
        }
    }

    public void run() {
        do {
            synchronized (MUTEX) {
                if (null == data) {
                    return;
                } else {
                    System.out.println(data);
                }
                try {
                    MUTEX.wait();
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
        } while (true);
    }
}

The trouble with this code is that your while() loop is constantly trying to grab the monitor for MUTEX immediately after releasing it or even yield()-ing to help the scheduler put another thread in. So there's a very good chance that anyone else trying to obtain that monitor will be starved, because your while() loop will consume most of your CPU time and even when other threads could run, they might not get the monitor they're waiting for.

Ideally a wait()/notify() pair should be used or failing that, you should at least call a Thread.yield() in your while loop, outside the synchronized block. (But I this second "solution" really isn't a very good one, you should consider using the first one instead.)

UPDATE: I read the code again and I think I believe to see what you wanted to achieve: printing the value of data every time you set a new value. If that's true, you should definitely go for the wait/notify solution, although if you want to absolutely guarantee that every single value is printed, you need to do even more work, possibly using a queue.

I'm a little confused about your code, can you provide the full listing?

First, where does DoStuff start a thread? Why are you quitting if your data is still null? (you might actually be out of the thread before setData even executes).

But the main thing here is that you're doing essentially a busy-waiting loop, in which you synchronize on the mutex. This is pretty wasteful and will generally block cores of your CPU.

Depending on what you are trying to do, you might want to use a wait-notify scheme, in which the thread goes to sleep until something happens.

Thanks all for your help. I was able to determine why the indefinite lock. Something important and obvious is that once I run the reportStatus() function call it will hold the lock MUTEX until it is completely done executing the callback. My fault was that at the registered callback I was calling setOnProgressListener(null) by mistake. Yes, I admit didn't post enough code, and most likely all of you would have catched the bug... So calling setOnProgressListener(null) would wait until the MUTEX object has been released, and the reportStatus() was held waiting to call setOnProgressListener(null), therefore I was in a deadlock!

Again the main point I learned is to remember that triggering a callback message will hold until the registered callback function is done processing it's call.

Thanks all!