Synchronization on static and instance method

Question

I am confused about synchronizing an instance method and a static method. I want to write a thread safe class as follow :

public class safe {

  private final static ConcurrentLinkedQueue<Object> objectList=
      new ConcurrentLinkedQueue<Object>();

  /**
   * retrieves the head of the object and prints it
   */
    public synchronized static  void getHeadObject() {
      System.out.println(objectList.peek().toString());

    }

    /**
     * creates a new object and stores in the list.
     */
    public synchronized void addObject() {
      Object obj=new Object();
      objectList.add(obj);

    }
}

Synchronizing on a static method will lock on safe.class lock and synchronizing on a instance method will lock on this .and hence an inconsistent state will be reached.

If I want to achieve a consistent state for a below code snippet how can that be achieved?

Answer 1

First, ConcurrentLinkedQueue does not require explicit synchronization. See this answer.

Second, you always can synchronize object you are accessing:

public class safe {

      private final static ConcurrentLinkedQueue<Object> objectList=
          new ConcurrentLinkedQueue<Object>();

      /**
       * retrieves the head of the object and prints it
       */
     public static  void getHeadObject() {
         synchronized(objectList){
          System.out.println(objectList.peek().toString());
         }

     }

        /**
         * creates a new object and stores in the list.
         */
     public void addObject() {
          Object obj=new Object();
       synchronized(objectList){
          objectList.add(obj);
       }

     }
}

Answer 2

EDIT: I'm assuming you meant Queue<Object> objectList instead of ConcurrentLinkedQueue<Object> objectList. ConcurrentLinkedQueue<Object> already does all of your thread safety for you, meaning you can call objectList.peek() all you want without worrying about race conditions. This is great if you're developing multi-threaded programs but not so great for learning about thread safety.

Your methods need not be synchronized, assuming you have one thread operating on one instance of the object at a time, but however if you need to have multiple instances of the class that all refer to the same static class variable, you need to synchronized over the class variable like so:

public static void getHeadObject() {
    synchronized(safe.objectList) {
        System.out.println(objectList.peek().toString());
    }
}

This locks the objectList and does not allow it to be read or written to in any other thread as soon as the program is inside the synchronization block. Do the same for all other methods to be synchronized.

NOTE:

However, since you are doing only one simple get operation List.peek(), you really don't need to synchronize over the objectList since in a race condition, it'll get either one value of the List or another. The problem with race conditions is when multiple complex read/write operations are performed, with the value changing in between them.

For example, if you had a class PairInt with a PairInt.x and PairInt.y fields, with the constraint that x = 2y, and you wanted to do

System.out.println(myIntPair.x.toString() + ", " + myIntPair.y.toString());

and another thread was updating the value of x and y at the same time,

myIntPair.y = y + 3;
myIntPair.x = 2 * y;

And the write thread modified myIntPair in between your read thread's myIntPair.x.toString() and myIntPair.y.toString() you might get an output that looks like (10, 8), which means if you are operating on the assumption that x == 2 * y could crash your program.

In that case, your read needs to use a synchronized, but for more simpler things like peek() on a simple object that is being added or deleted, not modified while in the queue, the synchronized can, in most cases be dropped. In fact, for string, int, bool, and the like, the synchronized condition for a simple read should be dropped.

However, writes should always be synchronized on operations that are not explicitly thread safe, i.e. already handled by java. And as soon as you acquire more than one resource, or require that your resource stay the same throughout the operation as you do multiple lines of logic to it, then you MUST USE synchronized

Answer 3

A few comments:

• Java conventions:
  • class names should be in CamelCase (i.e. call your class Safe, not safe)
  • static comes before synchronized in methods declaration
  • static comes before final in fields declaration
• as others have already said, ConcurrentLinkedQueue is already thread safe, so there is no need for synchronization in the example you give.
• mixing static and non static methods the way you do looks weird.
• assuming that your actual use case is more complicated and you need a method to run atomic operations, then your code does not work, as you pointed out, because the 2 synchronized methods don't synchronize on the same monitor:

public static synchronized getHeadObject(){} //monitor = Safe.class
public static synchronized addObject(){} //monitor = this

So to answer your specific question, you could use a separate static object as a lock:

public class Safe {

    private static final ConcurrentLinkedQueue<Object> objectList =
            new ConcurrentLinkedQueue<Object>();
    // lock must be used to synchronize all the operations on objectList
    private static final Object lock = new Object();

    /**
     * retrieves the head of the object and prints it
     */
    public static void getHeadObject() {
        synchronized (lock) {
            System.out.println(objectList.peek().toString());
        }
    }

    /**
     * creates a new object and stores in the list.
     */
    public void addObject() {
        synchronized (lock) {
            Object obj = new Object();
            objectList.add(obj);
        }
    }
}