Concurrent access: volatility and synchronization

Question

I have been reading through quite a few posts about synchronized and volatile keywords/idoms and I think I understood correctly how they work and when should they be used. However, I still have some doubts on something I'm trying to do. Consider the following:

public class X {
  private volatile int x;

  public X(int x) {
    this.x = x;
  }

  public void setX(int x) {
    this.x = x;
  }  

  public int getX() {
    return x;
  }
} 

The one above is pretty much straightfoward and thread-safe. Now consider the same class X with the following changes:

public class X {
  private volatile int x;
  private volatile Y yObj;
  private volatile boolean active;

  public X(Y yObj) {
    this.yObj = yObj;
    active = false;
    x = yObj.getY();
  }

  public void setX(int x) {
    if (active) throw new IllegalStateException()
    if (!yObj.isValid(x)) throw new IllegalArgumentException();
    this.x = x;  
  }

  public void setY(Y yObj) {
    if (active) throw new IllegalStateException();
    this.yObj = yObj;
    x = yObj.getY();
  }

  public int getX() {
    return x;
  }

  public Y getY() {
    return yObj;
  } 

  public synchronized void start() {
     if (active) throw new IllegalStateException();
     /*
      * code that performs some initializations and condition checking runs here
      * does not depend on x and yObj
      * might throw an exception
      */
      active = true;
  }

  public synchronized void stop() {
      if (!active) throw new IllegalStateException();
      /* some code in the same conditions of the comments in the start()
       * method runs here
       */
      active = false;
  }

  public boolean isActive() {
    return active;
  }
} 

Now, I declared yObj as volatile to ensure that every single thread sees the same object reference when changed by calling setY(Y) method. The idea of the Y class is to provide the X class a set (in this example just one) of reference values when calling the setters of a X object. The questions are:

1. Can x still be declared as volatile and ensure common visibility for all threads or further synchronization is required?
2. The idea is to make all objects of class Y immutable. So, I assume that all its fields must be immutable as well. What would be the best way to make Y user implementable but as the same time be thread-safe? An abstract class that implements the thread-safe mechanisms and then it can be extendable? Currently, Y is an interface with getter methods that can be implemented, of course, not thread-safe.
3. Is the start/stop mechanism implemented correctly from the perspective of concurrent access?

Answer 1

The crux of your problem is that private volatile Y yObj; only makes the yObj reference volatile, not its contents.

When you later do x = yObj.getY(); you may be requesting access to a non-volatile variable which could, theoretically be thread-unsafe as a result.

Making yObj immutable may help but enforcing that would be difficult.

Your start/stop mechanism looks fine but I would use an AtomicBoolean, drop the synchronisation and use if(active.compareAndSet(false, true) { ... or similar.