Ability to forget the memoized supplier value

Question

Guava has a feature request which is asking to provide the ability to forget memoized value on a given supplier.

On top on that I needed another functionality where if during the calculation of expensive value another thread would invalidate the data, the memoisation should retry (guava LoadableCache doesn't provide this feature: invalidation during calculation is ignored).

I couldn't wait for this feature so I wrote this myself. First, an interface for forgettable supplier:

public interface ForgettableSupplier<T> extends Supplier<T> {
    public void forget();
}

Then, a base class, mostly taken from here:

public abstract class DoubleCheck<T> extends ReentrantReadWriteLock {

    protected abstract T create();

    protected abstract T retrieve();

    protected abstract boolean invalid(T value);

    public T get() {
        try {
            readLock().lock();
            T value = retrieve();
            while (invalid(value)) { // this spins until no more invalidations are made
                readLock().unlock();
                writeLock().lock();
                value = retrieve();
                try {
                    if (invalid(value)) {
                        value = create();
                    }
                } finally {
                    readLock().lock();
                    writeLock().unlock();
                }
            }
            return value;
        } finally {
            readLock().unlock();
        }
    }
}

And finally, the static utility that combines everything together:

public class Suppliers {

    private Suppliers() {
    }

    public static <T> ForgettableSupplier<T> memoize(Supplier<T> delegate) {
        return new ForgettableMemoizingSupplier<>(delegate);
    }

    private static final class ForgettableMemoizingSupplier<T> implements ForgettableSupplier<T> {

        private final Supplier<T> delegate;

        private volatile T t;

        private final AtomicBoolean needsToBeForgotten = new AtomicBoolean(false);

        private final DoubleCheck<T> doubleCheck = new DoubleCheck<T>() {
            @Override
            protected T create() {
                needsToBeForgotten.set(false);
                return ForgettableMemoizingSupplier.this.t = delegate.get();
            }

            @Override
            protected T retrieve() {
                return ForgettableMemoizingSupplier.this.t;
            }

            @Override
            protected boolean invalid(final T t) {
                return (ForgettableMemoizingSupplier.this.t == null) || needsToBeForgotten.get();
            }
        };

        private ForgettableMemoizingSupplier(final Supplier<T> delegate) {
            this.delegate = delegate;
        }

        @Override
        public void forget() {
            needsToBeForgotten.set(true);
        }

        @Override
        public T get() {
            return doubleCheck.get();
        }
    }
}

Answer 1

1. Class names should be nouns. I'd rename DoubleCheck to DoubleCheckedResource or something similar.

2. ForgettableMemoizingSupplier should not allow null suppliers. You should check that and throw a NullPointerException.

3. The name of the invalid method in the DoubleCheck class is ambiguous. Does it invalidate something or does it check that something is valid or not? It seems that the latter is true, so I'd call this isInvalid (or isValid to eliminate the negation).

4. The T t field could be in the DoubleCheck anonymous subclass because this is the only class which uses it.

5. Is DoubleCheck a ReentrantReadWriteLock? Currently it is. I think your clients should not be able to call getOwner(), writeLock() or isFair() on a DoubleCheck instance. I'd use composition instead of inheritance here:

   public abstract class DoubleCheck<T> {
   
       private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
   
       private final ReadLock readLock = lock.readLock();
   
       private final WriteLock writeLock = lock.writeLock();
   
       ...
   
   }
   

   Check Effective Java, Second Edition, Item 16: Favor composition over inheritance.

6. The get method with the lock() call in the finally block smells a little bit. Are you sure that the object will always be in a consistent state? What happens when retrieve(), create() or invalid() throws an exception? Will all locks be released properly?

   If I'm right the following is the same but use the usual lock(); try { use() } finally { unlock(); } construction:

   public T get() {
       readLock.lock();
       try {
           final T value = retrieve();
           if (!isInvalid(value)) {
               return value;
           }
       } finally {
           readLock.unlock();
       }
   
       writeLock.lock();
       try {
           T value = retrieve();
           while (isInvalid(value)) { // this spins until no more invalidations
                                      // are made
               value = retrieve();
               if (isInvalid(value)) {
                   value = create();
               }
           }
           return value;
       } finally {
           writeLock.unlock();
       }
   }
   

   (Please note that in the original code between the readLock.unlock() and writeLock.lock() calls another threads could get the read lock, therefore more than one thread could acquire the write lock at the same time.)

7. If you can ignore the forget calls which are parallel with get calls it could be more simple:

   public T get() {
       readLock.lock();
       try {
           final T value = retrieve();
           if (!isInvalid(value)) {
               return value;
           }
       } finally {
           readLock.unlock();
       }
   
       writeLock.lock();
       try {
           T value = retrieve();
           if (!isInvalid(value)) {
               return value;
           }
           return create();
       } finally {
           writeLock.unlock();
       }
   }
   

   (Here if (isValid(value)) would be more natural than the current if (!isInvalid(value) ....)

8. I'd consider using an AtomicReference and a simple ReentrantLock too:

   public final class ForgettableMemoizingSupplier<T> 
           implements ForgettableSupplier<T> {
   
       private final Supplier<T> delegate;
   
       private final AtomicReference<T> resourceRef = new AtomicReference<T>(); 
   
       private final ReentrantLock lock = new ReentrantLock();
   
       public ForgettableMemoizingSupplier(final Supplier<T> delegate) {
           this.delegate = checkNotNull(delegate, "delegate cannot be null");
       }
   
       @Override
       public void forget() {
           resourceRef.set(null);
       }
   
       @Override
       public T get() {
           final T resource = resourceRef.get();
           if (resource != null) {
               return resource;
           }
           return getAndSetResource();
       }
   
       private T getAndSetResource() {
           lock.lock();
           try {
               final T resource = resourceRef.get();
               if (resource != null) {
                   return resource;
               }
               final T newResource = delegate.get();
               resourceRef.set(newResource);
               return newResource;
           } finally {
               lock.unlock();
           }
       }
   }
   

   (Untested code.)