Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 http://weblogs.java.net/blog/mason/archive/2006/09/rechecking_doub.html

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();
        }
    }
}

Any comments are appreciated.

share|improve this question

1 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.)

share|improve this answer
    
Thanks for your comments. I mostly agree with 1-5 (and have actually done most of these myself independently) but 7-8 are irrelevant because I need readers to block until there are no more forget() calls and create()ing is finished (see the second chapter of my post). Regarding 6 - if I understand correctly, this will make it so two create() calls can be running at the same time, right? This might or might not be desirable, I need to think more about this. –  mindas Nov 5 '12 at 15:13

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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