Locking strategy with PropertyChangeListener

Question

I've defined a class with a number of "observable" properties. Internally the class contains a single thread that performs I/O; e.g.

public class Foo {
  private final PropertyChangeSupport support;
  private State state;

  public Foo() { this.support = new PropertyChangeSupport(this); }

  public synchronized State getState() { return state; }

  public synchronized void setState(State state) {
    if (this.state != state) {
      State oldState = this.state;
      this.state = state;

      // Fire property change *whilst still holding the lock*.
      support.firePropertyChange("state", oldState, state);
    }
  }

  public synchronized void start() {
    // Start I/O Thread, which will call setState(State) in some circumstances.
    new Thread(new Runnable() ...
  }
}

My question is: Should I avoid firing property change events whilst holding onto the class's lock? ... or perhaps I should be firing property change events from a single dedicated thread (e.g. "event-multicaster" thread)?

The current design is resulting in a deadlock whereby thread A takes out a lock on an external class: Bar, and then tries to call a method on Foo and take out a second lock. However, at the same time the I/O thread calls setState(State) obtaining the lock on Foo, which propagates a property changed event out to the containing class Bar and attempts to get the lock on this class ... resulting in deadlock. In other words, the property change callback design means I cannot effectively control the order in which my locks are being obtained.

My current workaround is to make state volatile and remove the synchronized keywords but this seems like a kludge; for one thing it means the order in which the property change events are fired is not guaranteed.

Answer 1

If you need to have other threads call back into your class from the notification loop, then you need to reduce the scope of synchronization, using a synchronized block rather than synchronizing the entire message (this is copied from your post, no idea if it compiles):

public void setState(State state) {
    State oldState = null;
    synchronized (this) {
      if (this.state != state) {
        oldState = this.state;
        this.state = state;
      }
    }

    if (oldState != null)
      support.firePropertyChange("state", oldState, state);
  }

Hold locks for the shortest amount of time possible. And think about replacing these callbacks with a synchronized message queue (look in java.util.concurrent).

---

Edit, to generalize on the answer and add some philosophy.

Rant first: multi-threaded programming is hard. Anyone who tells you differently is trying to sell you something. Creating a lot of interdependent connections in a multi-threaded program leads to subtle bugs, if not outright insanity.

The best way that I know of to simplify multi-threaded programming is to write independent modules with well-defined start and stop points -- in other words, the actor model. An individual actor is single-threaded; you can easily comprehend and test it in isolation.

The pure actor model fits well with event notifications: the actor is invoked in response to an event, and performs some action. It doesn't care whether another event has fired since it started. Sometimes, that's not enough: you need to make a decision based on state managed by another thread. That's OK: the actor can look at that state (in a synchronized manner) and make decisions.

The key thing to remember (as Tom Hawtin notes in his comment) is that the state you read now may be different a millisecond from now. You can't ever write code that assumes that you know the exact state of an object. If you feel that you need to do this, you need to rethink your design.

And a final comment: Doug Lea is smarter than you or I. Don't try to reinvent the classes in java.util.concurrent.