What is causing racing condition in the buffered class

Question

I'm trying to create a generic class to make some synchronized buffered writing operation.

I'm trying to minimize lock time, so i used a non final lock object that im my mind would be enough to keep the class synchronized and avoid racing condition... but while testing it i'm still getting racing condition and the final result is that some entities are flushed multiple times

can anyone try to explain me what is causing racing condition, what scenario is the cause of sync problem?

PLEASE DO NOT TELL ME TO MAKE THE METHOD flushCache SYNCHRONIZED... I KNOW HOW TO SOLVE THIS PROBLEM, im just trying to understand why it is happening

public abstract class SynchronizedBufferedSaver<E> {


   private final int MAX_BUFFER_SIZE;

   private Set<E> savingBuffer;

   public SynchronizedBufferedSaver(final int max_buffer_size) {
      MAX_BUFFER_SIZE = max_buffer_size;
      savingBuffer = ConcurrentHashMap.newKeySet((int) (max_buffer_size * 1.5));
   }


   public void save(@NonNull final E entity) {
      copyToCache(entity);
      flushCache(false);
   }

   abstract protected void persist(@NonNull final Set<E> cache);

   public void flushCache(final boolean force) {
      synchronized (savingBuffer) {
         if (!savingBuffer.isEmpty() && (force || savingBuffer.size() >= MAX_BUFFER_SIZE)) {
            final Set<E> tmp = savingBuffer;
            savingBuffer = ConcurrentHashMap.newKeySet((int) (MAX_BUFFER_SIZE * 1.5));
            persist(tmp);
         }
      }
   }

   private void copyToCache(@NonNull final E entity) {
      savingBuffer.add(entity);
   }


}

Answer 1

Ended up just giving up to try to reduce the synchronization blocks, it was too much effort for a small task.

I tried volatile and atomicreference without success

the following code will solve the problem with the extra synchronization block... although if anyone has an article or description of how that could be achieved with 1 synchronization block please send me i would love to learn

package br.com.fisgar.utils;

public abstract class SynchronizedBufferedSaver<E> {


   private final int MAX_BUFFER_SIZE;

   private final Set<E> savingBuffer;

   public SynchronizedBufferedSaver(final int max_buffer_size) {
      MAX_BUFFER_SIZE = max_buffer_size;
      savingBuffer = new HashSet<>((int) (max_buffer_size * 1.5));
   }


   public void save(@NonNull final E entity) {
      copyToCache(entity);
      flushCache(false);
   }

   abstract protected void persist(@NonNull final Set<E> cache);

   public void flushCache(final boolean force) {
      final Set<E> tmp = new HashSet<>();
      synchronized (savingBuffer) {
         if (!savingBuffer.isEmpty() && (force || savingBuffer.size() >= MAX_BUFFER_SIZE)) {
            tmp.addAll(savingBuffer);
            savingBuffer.clear();
         }
      }
      if (!tmp.isEmpty()) {
         persist(tmp);
      }
   }

   private void copyToCache(@NonNull final E entity) {
      synchronized (savingBuffer) {
         savingBuffer.add(entity);
      }
   }
}

Answer 2

Besides replacing savingBuffer (as said in the comments), you're mixing two different locking mechanisms here. flushCache uses the entire set as lock, but the call to savingBuffer.add inside copyToCache locks only a portion of the backing ConcurrentHashMap. That's because ConcurrentHashMap does not synchronize on the map itself. From ConcurrentHashMap:

However, even though all operations are thread-safe, retrieval operations do not entail locking, and there is not any support for locking the entire table in a way that prevents all access.

Your own solution of using a HashSet with synchronization solves the issue because now the same locking mechanism is used - synchronized (savingBuffer). You can make life a bit easier by wrapping it in Collections.synchronizedSet because then all simple calls like add and clear will automatically use an intrinsic lock on savingBuffer. The synchronized (savingBuffer) inside copyToCache could be removed because it would already be part of the set.