1

I'm using Spring framework. Need to have a list of objects, which should get all data from database at once. When data is changed, list will be null and next get operation should fill data from database again. Is my code correct for multi-thread environment? 

@Component
@Scope("singleton")
public class MyObjectHolder {
    private volatile List<MyObject> objectList = null;

    public List<MyObject> getObjectList() {
        if (objectList == null) {
            synchronized (objectList) {
                if (objectList == null) {
                    objectList = getFromDB();
                }
            }
        }
        return objectList;
    }

    synchronized
    public void clearObjectList() {
        objectList = null;
    }
}
Jack
  • 435
  • 2
  • 6
  • 16
  • Should a thread that calls `getObjectList()` be guaranteed data or is it okay for it to get null data? Also, is it okay to block a caller or does it need to fail fast? – disrvptor Dec 02 '13 at 12:04

2 Answers2

3

Short answer: no.

public class MyObjectHolder {
  private final List<MyObject> objectList = new List<>();
  public List<MyObject> getObjectList() {          
    return objectList;
  }

This is the preferred singleton pattern.

Now you need to figure out how to get the data into the list in a thread-safe way. For this Java already has some pre-made thread-safe lists in the concurrent package, which should be preferred to any synchronized implementation, as they are much faster under heavy threading.

Your problem could be solved like this:

public class MyObjectHolder {

  private final CopyOnWriteArrayList<MyObject> objectList = new CopyOnWriteArrayList<>();

  public List<MyObject> getObjectList() {
    return objectList;
  }

  public boolean isEmtpy() {
    return objectList.isEmpty();
  }

  public void readDB() {
    final List<MyObject> dbList = getFromDB();
    // ?? objectList.clear();
    objectList.addAll(dbList);
  }
}

Please note the absence of any synchronized, yet the thing is completely thread-safe. Java guarantees that the calls on that list are performed atomically. So I can call isEmpty() while someone else is filling up the list. I will only get a snapshot of a moment in time and can't tell what result I will get, but it will in all cases succeed without error.

The DB call is first written into a temporary list, therefore no threading issues can happen here. Then the addAll() will atomically move the content into the real list, again: all thread-safe.

The worst-case scenario is that Thread A is just about done writing the new data, while at the same time Thread B checks if the list contains any elements. Thread B will receive the information that the list is empty, yet a microsecond later it contains tons of data. You need to deal with this situation by either repeatedly polling or by using an observer pattern to notify the other threads.

TwoThe
  • 13,879
  • 6
  • 30
  • 54
  • Is need getObjectList() to check if list is empty and fill it if it is. And this should be an atomic operation. And other thread can delete items from the list at any moment. – Jack Dec 02 '13 at 11:53
  • When you say the "other thread can delete items from the list at any moment" do you mean another thread can clear the list or delete specific items? Simply clearing the list is a little easier to deal with then removing single items. – disrvptor Dec 02 '13 at 12:02
  • The list can handle multiple removes by different threads, but if you have a situation where you expect many writes other concurrent lists are faster in this case. If you post more of your code I can probably answer that more in detail. – TwoThe Dec 02 '13 at 12:12
  • just two operations on the list: getting data and removing all items. – Jack Dec 02 '13 at 12:27
  • Then the above is ok. Reading is save, and a call to `objectList.clear()` is safe as well (worst-case: you have a reader end too soon, because there was "no more element"). You can as well keep the data lingering in memory until the next `readDB()` call, if you only read small amounts of data, and clear the list right before the `addAll()` as hinted in my example. – TwoThe Dec 02 '13 at 12:32
  • If I change method from your example to `public synchronized List getObjectList() { if (isEmtpy()){ readDB(); } return objectList; }` Should it be correct? – Jack Dec 02 '13 at 12:41
  • It will work fine, but then you are synchronizing again, which will slow things down. For a better solution you however might have to change a lot of code (like using a LinkedBlockingQueue), so if what you asked works for you, you can use it. – TwoThe Dec 02 '13 at 13:04
2

No, your code is not thread safe. For example, you could assign objectList in one thread at time X, but set it to null (via clearObjectList()) at time X+1 because you are synchronizing on 2 different objects. The first synchronization is on objectList itself and the second synchronization is on the instance of MyObjectHolder. You should look into locks when using a shared resource instead of using synchonize, specifically something like a ReadWriteLock.

disrvptor
  • 1,592
  • 12
  • 23