Synchronize on the same lock

Question

An extract from a book which I am reading-

// Synchronize on the wrong lock, hence @NotThreadSafe

public class ListHelper<E> {

    public List<E> list =  Collections.synchronizedList(new ArrayList<E>());

    public synchronized boolean putIfAbsent(E x){
        boolean absent = !list.contains(x);
        if(absent)
            list.add(x);
        return absent;
    }
}

// Synchromize on the same/correct lock, hence @ThreadSafe
// Implementing Put-if-absent with Client-side Locking.

public class ListHelper<E> {

    public List<E> list =  Collections.synchronizedList(new ArrayList<E>());

    public  boolean putIfAbsent(E x){
        synchronized(list){
            boolean absent = !list.contains(x);
            if(absent)
                list.add(x);
            return absent;
        }
    }
}

Moving swiftly on, the author moves to the 3rd case-
// Implementing put-if-absent using composition.

public class ImprovedList<E> implements List<E> {

    private final List<E> list;

    public ImprovedList(List<E>  list){
        this.list = list;
    }

    public synchronized boolean putIfAbsent(E x){

        boolean contains = list.contains(x);
        if(contains)
            list.add(x);
        return !contains;

    }
}

How is the above class @ThreadSafe, even when the list in public final List<E> list; mightnot be @ThreadSafe

Answer 1

I also see this in one book, see my code as below

public class ListHelper {

 public List<String> list =  Collections.synchronizedList(new ArrayList<String>());

    public synchronized boolean putIfAbsent(String x) throws InterruptedException{// lock is ListHelper
        boolean absent = !list.contains(x);
        Thread.sleep(1000);
        if(absent)
            list.add(x);
        return absent;
    }

   public void addList(String str) throws InterruptedException{
       Thread.sleep(500);
       list.add(str); // lock is SynchronizedCollection mutex 
   }

public static void main(String[] args) throws InterruptedException {
    ListHelper lh = new ListHelper();
    MyThread t1 = new MyThread(lh);
    MyThread2 t2 = new MyThread2(lh);
    t1.start();
    t2.start();
    Thread.sleep(1500);
    System.out.println("size="+lh.list.size());
    for(String str : lh.list){
        System.out.println("item="+str);
    }

}

}

class MyThread extends Thread{
ListHelper lh;
public MyThread(ListHelper lh){
    this.lh=lh;
}

@Override
public void run() {
    try {
        lh.putIfAbsent("maksim");
    } catch (InterruptedException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }
}

}

class MyThread2 extends Thread{
ListHelper lh;
public MyThread2(ListHelper lh){
    this.lh=lh;
}
@Override
public void run() {
    try {
        lh.addList("maksim");
    } catch (InterruptedException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

}

}

Answer 2

Short answer. You need to read another book. This one is confusing and incorrect.

// Synchronize on the wrong lock, hence @NotThreadSafe

I assume the reason why the book says that this is synchronizing on the wrong object (you don't synchronize on a "lock") is because the list is a public field. That's a very bad practice for sure. public fields are always dangerous -- especially so with multithreaded applications.

If it is the "wrong lock" because it is locking the helper instead of the list then the book is incorrect. You can say that it is a better practice to lock on the list but if so, it should be final.

// Synchronize on the same/correct lock, hence @ThreadSafe

// Implementing Put-if-absent with Client-side Locking.

But the field is still public which means that some other caller could be making operations on the list at the same time the putIfAbsent(...) call was being made. Synchronizing on the list instead of the helper makes little difference unless you can somehow guarantee that everyone else is doing this as well. That's a very dangerous assumption. If true then there is no need for the Collections.synchronizedList(...) wrapper.

For example, another call could call listHelper.list.add(x) after the list.contains(x) call finishes inside of the putIfAbsent(...) method, which would mean that x would be in the list twice. Not good.

// Implementing put-if-absent using composition.

This isn't thread-safe either because the List is passed in and there are no guarantees that the caller isn't continuing to operate on the list improperly. You could make this thread-safe by copying all of entries out of the passed in list into an internal list. The list must be owned by the helper for it to be fully thread-safe.

How is the above class @ThreadSafe, even when the list in public final List list; mightnot be @ThreadSafe

It's not. None of the 3 examples are thread-safe.

public class ListHelper<E> {
    private final List<E> list = new ArrayList<E>();
    public boolean putIfAbsent(E x) {
        synchronized (list) {
            boolean absent = !list.contains(x);
            if (absent) {
                list.add(x);
            }
            return absent;
        }
    }
}

This class, is thread-safe because it owns the list, the list field is not public, and the two operations on the list are synchronized. It would also be thread-safe if the synchronized keyword was on the putIfAbsent(...) method and not the list.

Answer 3

Can you tell me why in the second case, there is synchronized(list){...} even when list itself is synchronised.

If you make some object, O, entirely out of other thread-safe objects, that does not necessarily mean that O will be thread-safe.

What if you took out the synchronized statement, and then you let two threads call your listHelper.putIfAbsent(x) method with the same x at the same time? Both threads could call list.contains(x) at the same time, and both could set absent to true. Once they get to that point, they will both call list.add(x), and the thread-safe list will contain two references to the same object.

That appears not to be what you want.

Thread-safety for the list means that the two threads can do no harm to the internal structure of the list when they both call list.add(x) at the same time. It means that the list will live up to its promises, even when it is used by several threads at the same time.

The list.contains(x) method lived up to its promise: It returned false both times because x was not in the list. The list.add(x) lived up to its promise, it put an x reference into the list both times, just like it was told to do.

The problem (if you take out the synchronization) is not anywhere in the list object because the requirement that the list only contain one x reference is your requirement.

If you don't want the list to contain more than one x reference, then you have to write the code that prevents it from happening. That's what the synchronized statement does. It makes an atomic operation out of testing whether x is in the list and then doing something about it. "Atomic" basically means, two threads can't do it at the same time.

---

The class in the second case is thread safe as given in JCIP

The phrase "thread safe" does not have an exact meaning. I didn't say that ListHelper is not thread safe; I said it would be wrong to call it "thread safe."

I'm just taking a more conservative stance than Mr. Bloch, that's all.

The class is thread safe if you use it the way the author meant for you to use it. But by making the list member public, he also made it very easy for you to use his class in non-thread-safe ways.

I write software for a living, and in my job, it's neither necessary, nor sufficient for me to make programs work. My job is to make programs that make customers happy. If there's a wrong way to use my programs, and if I make the wrong way more attractive looking than the right way, then the customers are not going to be happy, my bosses are not going to be happy, and the excuse, "Yeah, but it works" isn't going to get me off the hook.

Answer 4

I believe this example is from JCIP. Given that,

In case 3 - It is still thread-safe because you are acquiring a lock on ImprovedList object by virtue of public synchronized boolean putIfAbsent(E x). The key thing to note here is even all other methods of this class that are working with list are using synchronized keyword. So only one thread would ever be able to use this list object at any given time.

It would be more clear if we try to add other list related methods and see why it works.

public class ImprovedList<E> implements List<E> {

    public final List<E> list;

    public ImprovedList(List<E>  list){
        this.list = list;
    }

    public synchronized boolean putIfAbsent(E x){

        boolean contains = list.contains(x);
        if(contains)
            list.add(x);
        return !contains;
    }

    public synchronized void add(E x){
        list.add(x);
    }
    //...Other list methods, but using synchronized keyword as above method.
}

In case 1 - Although it is public synchronized boolean putIfAbsent(E x) it is not thread safe because the key thing to note here other methods without any synchronized keyword in the ListHelper class can still update the list. ** Those methods are doing it because they think it is safe because of Collections.synchronizedList(new ArrayList<E>());**.

It would be more clear if we try to add other list related methods and see why it fails.

public class ListHelper<E> {

    public List<E> list =  Collections.synchronizedList(new ArrayList<E>());

    public synchronized boolean putIfAbsent(E x){
        boolean absent = !list.contains(x);
        if(absent)
            list.add(x);
        return absent;
    }

    //Not synchronized as this method assumes it is not necessary because the list itself is synchronized but doesn't help as above method and this method lock on different objects.
    public void add(E x){
        list.add(x);
    }
    //...Other list methods but no synchronized keyword as above
}

So basically the summary is to use either same lock as that of list (case-2) or uses an explicit lock (case -3).

Regarding @Markspace question :

That list, being public, is not thread safe. What book is this?

That is true and a caution has been added in the book to that extent. It says,

Like Collections.synchronizedList and other collections wrappers, ImprovedList assumes that once a list is passed to its constructor, the client will not use the underlying list directly again, accessing it only through the ImprovedList.