0

so im using CacheBuilder by Guava as a ConcurrentLRUCache meaning this cache is thread safe and has LRU properties, see CacheBuilder Guava Docs.

My assumption is when multiple threads are launched concurrently having the same key, a CyclicBarrier is used for this, then one thread will put() into the cache while the others wait. After that the remaining threads will see that a value is already in the cache and not put() into the cache.

This does not hold for the code below because each thread creates a new Object() and puts it into the cache. Verify by running the test and looking at the consol to see that different objects are created each time.

  • Is there anything inherently wrong with the way I'm using the
    CacheBuilder?
  • Is there better methods I can use?
  • Is there a library I can use?

Please and thanks!

import java.util.concurrent.CyclicBarrier;

import org.junit.Test;

import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;

public class GuavaLRUCacheTest {
    private Cache<String, Object> concurrentLRUCache = CacheBuilder.newBuilder().maximumSize(100).concurrencyLevel(1).build();

    @Test
    public void test() throws Exception {
        // The gate size is set based on the (number of threads to run) + (1 for the current thread).
        CyclicBarrier gate = new CyclicBarrier(4);

        // Same key is used for all threads
        ConcurrentLRUTestThread t1 = new ConcurrentLRUTestThread(gate, "key1");
        ConcurrentLRUTestThread t2 = new ConcurrentLRUTestThread(gate, "key1");
        ConcurrentLRUTestThread t3 = new ConcurrentLRUTestThread(gate, "key1");

        t1.start();
        t2.start();
        t3.start();

        // Open the gate on all threads.
        gate.await();

        t1.join();
        t2.join();
        t3.join();
    }

    class ConcurrentLRUTestThread extends Thread {
        private CyclicBarrier gate;
        private String key;
        public ConcurrentLRUTestThread(CyclicBarrier gate, String key) {
            this.gate = gate;
            this.key = key;
        }
        @Override
        public void run() {
            try {
                gate.await();
                if (concurrentLRUCache.getIfPresent(key) == null) {
                    System.out.println(">>>>> "+ System.nanoTime() +" - "+Thread.currentThread().getId() + " before put " + concurrentLRUCache.getIfPresent(key));
                    concurrentLRUCache.put(key, new Object());
                    System.out.println(">>>>> "+ System.nanoTime() +" - "+Thread.currentThread().getId() + " after put " + concurrentLRUCache.getIfPresent(key));
                } else{
                    System.out.println(">>>>> "+ System.nanoTime() +" - "+Thread.currentThread().getId() + " else " + concurrentLRUCache.getIfPresent(key));
                }  
            } catch (Throwable x) {
                System.out.println(">>>>> "+ System.currentTimeMillis() +" - "+Thread.currentThread().getId() + " ConcurrentLRUTestThread exception");
            }
        }
    }
}
  • 3
    Please use `LoadingCache`, or at least `get(key, callable)`. If you are on JDK8 you might consider my [rewrite](https://github.com/ben-manes/caffeine). – Ben Manes Aug 11 '16 at 21:05
  • 3
    You have no synchronization between `getIfPresent` and `put`, so all the threads can easily enter the first `if` branch, while none of them invoked `put`, and then each of them will perform `put`. – user3707125 Aug 11 '16 at 21:24

1 Answers1

2

You're first call cache.getIfPresent and then try to call cache.put. This will not work because this wasn't done as single atomic action. Multiple threads can see absence of value in the cache which will result in several cache.put calls. Straightforward way to fix this would be to make a critical section which includes check and action. Then only one thread will see absence of value in the cache. Fortunately Cache already contains method which does the thing: Cache.get(key, valueLoader). Just use it to retrieve value:


    public class GuavaLRUCacheTest {

    private Cache concurrentLRUCache = CacheBuilder.newBuilder().maximumSize(100).concurrencyLevel(1).build();

    @Test
    public void test() throws Exception {
        // The gate size is set based on the (number of threads to run) + (1 for the current thread).
        CyclicBarrier gate = new CyclicBarrier(4);

        // Same key is used for all threads
        ConcurrentLRUTestThread t1 = new ConcurrentLRUTestThread(gate, "key1");
        ConcurrentLRUTestThread t2 = new ConcurrentLRUTestThread(gate, "key1");
        ConcurrentLRUTestThread t3 = new ConcurrentLRUTestThread(gate, "key1");

        t1.start();
        t2.start();
        t3.start();

        // Open the gate on all threads.
        gate.await();

        t1.join();
        t2.join();
        t3.join();
    }

    class ConcurrentLRUTestThread extends Thread {
        private CyclicBarrier gate;
        private String key;
        public ConcurrentLRUTestThread(CyclicBarrier gate, String key) {
            this.gate = gate;
            this.key = key;
        }
        @Override
        public void run() {
            try {
                gate.await();
                concurrentLRUCache.get(key, Object::new);
            } catch (Throwable x) {
                System.out.println(">>>>> "+ System.currentTimeMillis() +" - "+Thread.currentThread().getId() + " ConcurrentLRUTestThread exception");
            }
        }
    }
}
featuredpeow
  • 2,061
  • 1
  • 19
  • 18