What is the correct way to synchronize a shared, static object in Java?

Question

This is a question concerning what is the proper way to synchronize a shared object in java. One caveat is that the object that I want to share must be accessed from static methods. My question is, If I synchronize on a static field, does that lock the class the field belongs to similar to the way a synchronized static method would? Or, will this only lock the field itself?

In my specific example I am asking: Will calling PayloadService.getPayload() or PayloadService.setPayload() lock PayloadService.payload? Or will it lock the entire PayloadService class?

public class PayloadService extends Service {   


private static PayloadDTO payload = new PayloadDTO();


public static  void setPayload(PayloadDTO payload){
    synchronized(PayloadService.payload){
        PayloadService.payload = payload;
    }
}

public static  PayloadDTO getPayload() {
    synchronized(PayloadService.payload){
        return PayloadService.payload ;
    }
}

...

Is this a correct/acceptable approach ?

In my example the PayloadService is a separate thread, updating the payload object at regular intervals - other threads need to call PayloadService.getPayload() at random intervals to get the latest data and I need to make sure that they don't lock the PayloadService from carrying out its timer task

Based on the responses, I refactored to the following:

public class PayloadHolder {

private static PayloadHolder holder;    
private static PayloadDTO payload;

private PayloadHolder(){        
}

public static synchronized PayloadHolder getInstance(){
    if(holder == null){
        holder = new PayloadHolder();
    }
    return holder;
}

public static synchronized void initPayload(){      
    PayloadHolder.payload = new PayloadDTO();       
}
public static synchronized PayloadDTO getPayload() {
    return payload;
}
public static synchronized void setPayload(PayloadDTO p) {
    PayloadHolder.payload = p;
}

}

public class PayloadService extends Service {   

  private static PayloadHolder payloadHolder = PayloadHolder.getInstance();

  public static  void initPayload(){        
            PayloadHolder.initPayload();        
  }

  public static  void setPayload(PayloadDTO payload){       
        PayloadHolder.setPayload(payload);      
  }

  public static  PayloadDTO getPayload() {      
    return PayloadHolder.getPayload();      
  }

     ...

Is this approach legitimate? I am also curious if it is better to do it this way or using the AtomicReference approach mentioned by Hardcoded ...? - I am keeping an instance of PayloadHolder on PayloadService simply to keep a reference to the PayloadHolder class active in the jvm for as long as the PayloadService is running.

Answer 1

Your code should look like this:

public static  void setPayload(PayloadDTO payload){
    synchronized(PayloadService.class){
        PayloadService.payload = payload;
    }
}

public static  PayloadDTO getPayload() {
    synchronized(PayloadService.class){
        return PayloadService.payload ;
    }
}

Your original code wouldn't have worked even if the methods weren't static. The reason being is you were synchronizing on the payload instance that you were changing.

Update, a response to johnrock comment: Locking the whole class is only a problem if you have other synchronized static blocks that you want to run currently. If you want to have multiple independent locked section then I suggest you do something like this:

public static final Object myLock = new Object();

public static  void setPayload(PayloadDTO payload){
    synchronized(myLock){
        PayloadService.payload = payload;
    }
}

public static  PayloadDTO getPayload() {
    synchronized(myLock){
        return PayloadService.payload ;
    }
}

Or, if you require a more complex concurrency pattern look at java.util.concurrent which has many pre-built classes to aid you.

Answer 2

My question is, If I synchronize on a static field, does that lock the class the field belongs to similar to the way a synchronized static method would? Or, will this only lock the field itself?

No, it just lock in the object itself ( the class attribute not the whole class )

Is this a correct/acceptable approach ?

You could probably take a look at the java.util.concurrent.lock package.

I don't really like synchronizing on a class attribute, but I guess that's just a matter of teste.

Answer 3

There is a major part of functionality in this class which isn't posted which contributes to whether or not this approach is thread-safe: how do you access the PayloadDTO instance from the other parts of this class where it is used?

If you are providing methods which could swap in another instance for payload while another thread is running code which uses the payload object, then this is not thread safe.

For example if you have a method execute() which does the main work of this class and invokes methods on payload, you need to make sure that one thread cannot change the payload instance with the setter method while another thread is busy running execute().

In short, when you have shared state, you need to synchronize on all read and write operations on the state.

Personally I don't understand this approach and would never take it - providing static methods to allow other threads to re-configure a class smells like a violation of separation of concerns.

Answer 4

Is this a correct/acceptable approach ?

No, the reason for this is that you should never synchronize on an variable/field that can change its value. That is, when you synchronize on PayloadService.payload and set a new PayloadService.payload, then you are violating a golden rule of synchronization.

You should either synchronize on the class instance or create some arbitrary private static final Object lock = new Object() and synchronize on that. You will have the same effect as synchronizing on the class.

Answer 5

Synchronize on another static object that does not change:

public class PayloadService extends Service {   


private static PayloadDTO payload = new PayloadDTO();

private static final Object lock = new Object();


public static  void setPayload(PayloadDTO payload){
    synchronized(lock){
        PayloadService.payload = payload;
    }
}

public static  PayloadDTO getPayload() {
    synchronized(lock){
        return PayloadService.payload ;
    }
}

Answer 6

You could, as mentioned in other posts, synchronize on the class or on an explicit monitor.

There are 2 other ways, if we assume that your are using the sychnronize only for thread-safe getting and setting of the property: volatile and AtomicReference.

volatile

The volatile keyword will make access to the variable atomic, meaning that reading and assigning the variable won't be optimized by the CPUs local registers and are done atomically.

AtomicReference

The AtomicReference is a special class at the java.util.concurrent.atomic package, which allows atomic access to a variable-like reference. It is very similiar to volatile, but gives you some additional atomic operations, like compareAndSet.

Example:

public class PayloadService extends Service {   

private static final AtomicReference<PayloadDTO> payload 
          = new AtomicReference<PayloadDTO>(new PayloadDTO());

public static void setPayload(PayloadDTO payload){
    PayloadService.payload.set(payload);
}

public static PayloadDTO getPayload() {
    return PayloadService.payload.get ;
}

Edit:

Your Holder seems quite confused, since you are instantiating classes only to call static Methods. A try to get it fixed with AtomicReference:

public class PayloadHolder {

  private static AtomicReference<PayloadHolder> holder = new AtomicReference<PayloadHolder();    

  //This should be fetched through the holder instance, so no static
  private AtomicReference<PayloadDTO> payload = new AtomicReference<PayloadDTO>();

  private PayloadHolder(){        
  }

  public static PayloadHolder getInstance(){
    PayloadHolder instance = holder.get();

    //Check if there's already an instance
    if(instance == null){

      //Try to set a new PayloadHolder - if no one set it already.
      holder.compareAndSet(null, new PayloadHolder());
      instance = holder.get();

    }
    return instance;
  }

  public void initPayload(){      
    payload.set(new PayloadDTO());

    //Alternative to prevent a second init:
    //payload.compareAndSet(null, new PayloadDTO());
  }

  public PayloadDTO getPayload() {
    return payload.get;
  }

  public void setPayload(PayloadDTO p) {
    payload.set(p);
  }

}

public class PayloadService extends Service {   

  private final PayloadHolder payloadHolder = PayloadHolder.getInstance();

  public void initPayload(){        
    payloadHolder.initPayload();        
  }

  public void setPayload(PayloadDTO payload){       
    payloadHolder.setPayload(payload);      
  }

  public PayloadDTO getPayload() {      
    return payloadHolder.getPayload();      
  }
}