Multithreaded correctness: Using synchronized block

Question

I am using the CMU Sphinx Speech Recognizer library (Link to source) that makes some use of synchronized blocks.

One example block from the RecognizerTask:

Event mailbox;

[...]

public void start() {
    synchronized (this.mailbox) {
        this.mailbox.notifyAll();
        this.mailbox = Event.START;
    }
}

The code works without any problems, however BugFinder gives this warning:

Bug: Synchronization on RecognizerTask.mailbox in futile attempt to guard it

This method synchronizes on a field in what appears to be an attempt to guard against simultaneous updates to that field. But guarding a field gets a lock on the referenced object, not on the field. This may not provide the mutual exclusion you need, and other threads might be obtaining locks on the referenced objects (for other purposes). An example of this pattern would be:

private Long myNtfSeqNbrCounter = new Long(0);
private Long getNotificationSequenceNumber() {
     Long result = null;
     synchronized(myNtfSeqNbrCounter) {
         result = new Long(myNtfSeqNbrCounter.longValue() + 1);
         myNtfSeqNbrCounter = new Long(result.longValue());
     }
     return result;
 }

To be honest, I don't quite understand the bug description and what is supposed to be wrong in this case. Is a global variable not a field? And if not, how can I improve the code?

/edit: This is the only part where Event.wait() is called:

Event todo = Event.NONE;
        synchronized (this.mailbox) {
            todo = this.mailbox;
            /* If we're idle then wait for something to happen. */
            if (state == State.IDLE && todo == Event.NONE) {
                try {
                    //Log.d(getClass().getName(), "waiting");
                    this.mailbox.wait();
                    todo = this.mailbox;
                    //Log.d(getClass().getName(), "got" + todo);
                } catch (InterruptedException e) {
                    /* Quit main loop. */
                    //Log.e(getClass().getName(), "Interrupted waiting for mailbox, shutting down");
                    todo = Event.SHUTDOWN;
                }
            }
            /* Reset the mailbox before releasing, to avoid race condition. */
            this.mailbox = Event.NONE;
        }

This code is actually using a synchronized statement as well. Does it make sense at all to use it?

Answer 1

the synchronized block "captures" the lock for the given object, in your case for the object denoted by mailbox. Once you change the variable mailbox to point to a different object, other threads can "capture" the lock for this object without a problem, since it is not taken.

Note that the lock is for objects, and not for references!

now, concider the following [pseudo code]:

synchronised (myObject) { 
  myObject = new Object();
  i += 5; //assume i is an instance variable
}

practically there is no lock here! every thread is creating a new object in the lock block, and the modification of i is not synchronized!

Answer 2

I don't think it applies in your case. You have a call to notifyAll() which means that somewhere in the code of the other threads there is a matching wait() call:

synchronized (this.mailbox) {
    this.mailbox.wait();        
}

meaning that the other thread will relinquish the lock while waiting to be notified.

Your code inspector is probably confused by the line:

this.mailbox = Event.START;

meaning that you might be concurrently modifying this object, such that if another thread attempts to get the lock on this.mailbox, it will see a different object. However, I do think that since:

1. this.mailbox is globally visible
2. assigns of references are atomic
3. the lock generates a fence

all threads should have an updated view of the synchronization object at all times.