6
\$\begingroup\$

I would like to find out if following usage of an AtomicBoolean as a flag variable is correct and good for a multi-threaded application. It is expected that data is going to be loaded only once and when the application is at very low load. It is also expected that the list of SomeObjects is going to be read quite frequently by multiple threads at peak load, probably thousands of time in an hour so that is why I want to avoid hitting the database. The list is not going to be very large so keeping it in memory is not going to be a problem.

public class HibernateDao extends SomeOtherClass implements Dao {   

    //Need suggestions for correct usage of following variable
    private AtomicBoolean dataLoaded = new AtomicBoolean();
    private List<SomeObject> someObjects;

    @Override
    public List<SomeObject> getAllSomeObjects() {       
        if (dataLoaded.getAndSet(false)) {
            //aim is to atomically load the list only once and avoid calling loadAll 
            //every time list is used           
            someObjects= (List<SomeObject>) getHibernateTemplate().loadAll(SomeObject.class);           
        }
        return someObjects;
    }

    @Override
    public void updateAllSomeObjects(Collection<SomeObject> someObjects) {
            //replaceAll is part of parent class and it removed and reloads
            //data for the entity i.e. SomeObject
        replaceAll(SomeObject.class, someObjects);
            //set this flag atomically so that data is fetched from DB next time
        dataLoaded.getAndSet(true);
    }

}
\$\endgroup\$
  • \$\begingroup\$ It's a quite interesting question you have here, could you provide some description or code about what the replaceAll method does? I imagine that the answer can be a bit different depending on what that does. Or how is the two methods of this class used in your application? \$\endgroup\$ – Simon Forsberg May 12 '14 at 12:06
  • \$\begingroup\$ it's a hibernate method and this just cleans the table mapped to SomeObject and then inserts data again. This operation is not expected to take very long time as data reloaded will be very small. Also, it will only run once daily when application load is low. \$\endgroup\$ – sttaq May 12 '14 at 12:08
  • \$\begingroup\$ Why do you want to use a atomic boolean instead of a synchronized block? \$\endgroup\$ – mheinzerling May 12 '14 at 12:43
  • \$\begingroup\$ can you please suggest where should I synchronize then? the whole method or just around loadAll? tbh I thought using Atomic would be more efficient? \$\endgroup\$ – sttaq May 12 '14 at 12:51
6
\$\begingroup\$

You are attempting to use the AtomicBoolean to satisfy two requirements:

  • The first thread to access the data loads it while all others wait until it's been loaded the first time.

  • Another thread can update the data for future callers.

Neither of these is working perfectly.

  • While only one thread will load the data, all other threads will receive null without waiting until that first thread completes the loading process.

  • Because the list of objects is updated after the memory barrier, other threads are not guaranteed to see the new reference immediately.

Solving the first problem is simple enough: use a CountDownLatch to cause all incoming threads to block until the first list of objects is set. Ideally the system would start another thread to specifically load the data rather than letting the first random thread to come along and do it. This isn't necessary (see below), but it's much cleaner.

For the second problem, wrap the list of objects in an AtomicReference to supply a correct memory barrier. You might be thinking, "Oh no! Not two synchronizers for each access!" but Java's synchronization primitives have improved greatly over the years, and the atomic value holders are even cheaper than full synchronization.

public class HibernateDao extends SomeOtherClass implements Dao {
    private CountDownLatch dataLoaded = new CountDownLatch();
    private AtomicReference<List<SomeObject>> data = new AtomicReference<>();

    @Override
    public List<SomeObject> getAllSomeObjects() {
        dataLoaded.await();          // wait until first countDown call
        return data.get();
    }

    @Override
    public void updateAllSomeObjects(Collection<SomeObject> someObjects) {
        replaceAll(SomeObject.class, someObjects);
        data.set(someObjects);
        dataLoaded.countDown();      // release any waiting threads
    }
}

If you really must have the first thread perform the initial loading rather than a separate thread that calls updateAllSomeObjects on its own, you can add back your beloved AtomicBoolean. This would be my last choice, however.

Note that while you can use if (!ab.getAndSet(true)) it's clearer to use compareAndSet as it indicates that you're performing a tested set rather than allowing any thread to set the new value.

    private AtomicBoolean firstCaller = new AtomicBoolean();

    @Override
    public List<SomeObject> getAllSomeObjects() {
        if (firstCaller.compareAndSet(false, true)) {
            updateAllSomeObjects((List<SomeObject>) getHibernateTemplate().loadAll(SomeObject.class));
        }
        else {
            dataLoaded.await();      // wait until first countDown call
        }
        return data.get();
    }
\$\endgroup\$
  • \$\begingroup\$ hmm I think after the first countDown, await will never cause any thread to wait? The updateAllSomeObjects method is going to be called once daily. So for the first time the threads will wait for it but after that they will not wait? \$\endgroup\$ – sttaq May 13 '14 at 8:40
  • \$\begingroup\$ @sttaq That is correct. The latch is there solely to cause threads to wait for the first batch of data to be loaded. When the data is loaded again each night, threads don't need to wait; they simply start seeing the new data as soon as it's loaded. \$\endgroup\$ – David Harkness May 13 '14 at 18:22
  • \$\begingroup\$ thanks, so in the rare case of data.get() and data.set() being called at the same time, the AtomicReference guarantees the operation is thread safe; pls correct me if my understanding is wrong? \$\endgroup\$ – sttaq May 13 '14 at 20:04
  • \$\begingroup\$ after reading up on Atomic set() method, I think that your first solution has nailed my requirements. In fact, I will have a separate thread that will call update method to insert into the database. Setting the list of someObjects in the update method saves me from calling the loadAll method as well. I don't need to read the objects from database because I know they are not being changed from some other class. I just hope that calling await() uselessly after the first time is not that much expensive or at least cheaper than checking an Atomic boolean? Thank you \$\endgroup\$ – sttaq May 13 '14 at 20:59
  • \$\begingroup\$ The calls to await once the latch has been "closed" (reached zero) should take no longer than accessing an AtomicBoolean. Since the latch is necessary to keep the first threads from reading null, there's really no choice. In use Spring to control the system startup so no client requests can come in before the first set of data has been loaded, removing the need for a latch or boolean. \$\endgroup\$ – David Harkness May 13 '14 at 23:33
1
\$\begingroup\$

I used to do that by checking the collection for null and initializing only if it is null otherwise returning the collection. Such bloc should be synchronized, of course:

synchronized (this) {
  if (someObjects == null) {
    someObjects = ...//initialize
  }
  return someObjects;
}

Besides, since your block is not synchronized, so if two threads start it simultaneously, one will start getting the objects from the database, and another will skip it (since dataLoaded will be already set) and get someObjects which is null.

\$\endgroup\$
  • \$\begingroup\$ null is not going to be a problem. I think I will also need synchronize in updateAllSomeObjects and synchronize on a flag variable instead of this? \$\endgroup\$ – sttaq May 13 '14 at 8:45
  • \$\begingroup\$ It depends. If you are ok with returning null instead of a collection, you could use your approach with the boolean value and not synchronize the initialization. If you don't want to return a null value (initialize first and then return a collection), then you should synchronize the block. And in this case, you don't need your boolean value at all, since you assure the atomicity of the whole block. As for updateAllSomeObjects, I am not sure since I can't see where it is called from... \$\endgroup\$ – Igor May 13 '14 at 13:13
  • \$\begingroup\$ updateAllSomeObjects will be called by some other thread. What I want is a way of notifying threads calling getAllSomeObjects that updateAllSomeObjects has been called. Now whoever is the first one to get this notification needs to read from the database and also make sure that no one else after goes to database. \$\endgroup\$ – sttaq May 13 '14 at 13:25
1
\$\begingroup\$

Cache invalidation is one of the two hard things in Computer Science. So I would strongly suggest rewriting the code so that the implementation of the cache is distinct from any class that has other responsibilities.

Better still, instead of rolling your own cache implementation, grab one that already has a few laps on it. Guava might be a reasonable choice.

As Igor points out, the AtomicBoolean isn't doing what you want here - it is provides a memory barrier (which provides guarantees about what memory changes are visible to other threads when), but does not provide synchronization. So you could, for example, fix the null pointer problem by initializing your list, but you would still be faced with the problem of threads seeing stale data while the latest data is getting loaded.

See also: Ability to forget the memoized supplier value

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.