1
\$\begingroup\$

In my project, there is an internal cache api to use. I have noticed an emerging pattern such that:

class SomeService {
   private final CacheApi cache = CacheApi.newCache();
   private final Object cacheAccessLock = new Object();

   public List<Something> getSomethings() {
       synchronized (cacheAccessLock) {
            if (cache.exists("cache-key")) {
                return cache.get("cache-key");
            }

            List<Something> somethings = loadSomethingsQuiteSlow();
            cache.put("cache-key", somethings);
            return somethings;
       }
   }

}

This pattern is implemented on several places. So I wrote a class called CacheReadWriteGuard to see if such task can be abstracted into an utility class.

The usage of the class

class SomeService {
   private final CacheApi cache = CacheApi.newCache();

   private final CacheReadWriteGuard<List<Something>> cacheGuard =
        new CacheReadWriteGuard.Builder<List<Something>>()
            .readBy(() -> cache.get("cache-key"))

            .expiredWhen(() -> cache.exists("cache-key") == false)

            .writeIfExpired(() -> {
                 List<Something> somethings = loadSomethingsQuiteSlow();
                cache.put("cache-key", somethings);
            });

   public List<Something> getSomethings() {
       return cacheGuard.get();
   }

}

The idea is to use ReadWriteLock to guard access to the cache such that if a write-action is needed, all Threads attempting to read will be halted until the write-action completes.

The implementation of the class is as follow:

public class CacheReadWriteGuard<T> {

    @FunctionalInterface
    public static interface ExpiredCacheCondition  {
        boolean isExpired();
    }

    @FunctionalInterface
    public static interface CacheRead<T> {
        T read();
    }

    @FunctionalInterface
    public static interface CacheWrite extends Runnable {
    }

    public static class Builder<T> {

        private ExpiredCacheCondition cacheCondition;
        private CacheRead<T> readAction;
        private CacheWrite writeAction;

        public Builder() {
        }

        /**
         * Provides a way that the guard can tell if the cache is expired.
         * In case if no {@code ExpiredCacheCondition} is supplied, this 
         * builder by default will use CacheRead.read() == null as the condition.
         */
        public Builder<T> expiredIf(ExpiredCacheCondition cacheCondition) {
            this.cacheCondition = cacheCondition;
            return this;
        }

        public Builder<T> readBy(CacheRead<T> cacheRead) {
            this.readAction = cacheRead;
            return this;
        }

        public Builder<T> writeOnExpired(CacheWrite cacheWrite) {
            this.writeAction = cacheWrite;
            return this;
        }

        public CacheReadWriteGuard<T> build() {
            Objects.requireNonNull(readAction, "CacheRead must not be null");
            Objects.requireNonNull(writeAction, "CacheWrite must not be null");
            if (cacheCondition == null) {
                cacheCondition = () -> readAction.read() == null;
            }

            return new CacheReadWriteGuard<>(cacheCondition, readAction, writeAction);
        }
    }

    private final ExpiredCacheCondition cacheCondition;
    private final CacheRead<T> readAction;
    private final CacheWrite writeAction;

    private final ReadWriteLock readWriteLock = new ReentrantReadWriteLock();
    private final Lock readLock  = readWriteLock.readLock();
    private final Lock writeLock = readWriteLock.writeLock();

    private CacheReadWriteGuard(
            ExpiredCacheCondition condition,
            CacheRead<T> readAction,
            CacheWrite writeAction)  {

        this.cacheCondition = condition;
        this.readAction = readAction;
        this.writeAction = writeAction;
    }

    /**
     * Returns a new Builder. The {@code Class<T>} is solely
     * for ensuring the compiler can infer the type of the
     * cached value.
     */
    public static <T> Builder<T> of(Class<T> type) {
        return new Builder<>();
    }

    public T get() {
        T  cachedValue = null;

        loadCache();

        try  {
            readLock.lock();
            cachedValue = readAction.read();
        } finally  {
            readLock.unlock();
        }

        return cachedValue;
    }

    private void loadCache()  {
        /*
         * Keeping all the other threads which don't hold the
         * write lock busy in the while loop until the one
         * which holds finish the writeAction.
         * 
         * If the cacheCondition returns false, the loop exits
         * immediately, thus making no attempt acquiring
         * the writeLock.
         */
        while (cacheCondition.isExpired())  {
            try {
                /*
                 * Attempts to get the write lock within 5 seconds.
                 * 
                 * If the current Thread fails to get the lock, it will
                 * be kept busy by the outer while loop.
                 * 
                 * And yes, the comment block wrapping the number 5 is 
                 * for visual effect making the number 5 stand out.
                 */
                if (writeLock.tryLock(/**/ 5 /**/, SECONDS))  {
                    try {
                        /*
                         * Re-check the state of the cache in case if some other
                         * Thread has acquired the writeLock and
                         * finish loading the cache _before_ this Thread's turn.
                         */
                        if(cacheCondition.isExpired()) {
                            writeAction.run();
                        }
                    } finally {
                        writeLock.unlock();
                    }
                }
            } catch (InterruptedException interrupted) {
                Thread.currentThread().interrupt();
            }
        }
    }
}

I have tested this class and it works.

I just don't know whether:

  • it is the right approach to go?
  • it is implemented correctly? Did I miss something that could cause a locking issue (deadlock, etc) in the future?
  • how should InterruptedException be handled in this situation?

Thanks a lot for your time. Genzer

\$\endgroup\$
  • \$\begingroup\$ Why is that builder so complicated? You should only need to pass cache object, key("cache-key") and loadSomethingsQuiteSlow() \$\endgroup\$ – user158037 Oct 25 '18 at 13:51
  • \$\begingroup\$ That means CacheReadWriteGuard needs to know how to use CacheApi. My intention is that the two should not be coupled with each other. \$\endgroup\$ – Genzer Oct 26 '18 at 3:11

Your Answer

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

Browse other questions tagged or ask your own question.