CODEWITHSUNDEEP
javaconcurrencyjava.util.concurrent
Yang Lifan
Yang Lifan

Reputation: 465

In JCIP sample "Listing 5.19 Memoizer", the cache should be removed when ExecutionException happen

Many people read "Java Concurrency in Practice", but paste the code in here for convenience.

5.19 Final implementation of Memoizer:

public class Memoizer<A, V> implements Computable<A, V> {
    private final ConcurrentMap<A, Future<V>> cache = new ConcurrentHashMap<A, Future<V>>();
    private final Computable<A, V> c;

    public Memoizer(Computable<A, V> c) {
        this.c = c;
    }

    public V compute(final A arg) throws InterruptedException {
        while (true) {
            Future<V> f = cache.get(arg);
            if (f == null) {
                Callable<V> eval = new Callable<V>() {
                    public V call() throws InterruptedException {
                        return c.compute(arg);
                    }
                };
                FutureTask<V> ft = new FutureTask<V>(eval);
                f = cache.putIfAbsent(arg, ft);
                if (f == null) {
                    f = ft;
                    ft.run();
                }
            }
            try {
                return f.get();
            } catch (CancellationException e) {
                cache.remove(arg, f);
            } catch (ExecutionException e) {
                throw LaunderThrowable.launderThrowable(e.getCause());
            }
        }
    }
}

My problem is in catch (ExecutionException e), the following snippet should be added:

cache.remove(arg, f);

Correct?

Upvotes: 1

Views: 182

Answers (2)

David Moles
David Moles

Reputation: 51169

Per JCIP (p. 106):

...Memoizer removes the Future from the cache if it detects that the computation was cancelled; it might also be desirable to remove the Future upon detecting a RuntimeException if the computation might succeed on a future attempt.

(Emphasis added.)

If not, there's no point, as assylias says above.

Upvotes: 1

assylias
assylias

Reputation: 328815

If compute is idempotent, then removing it form the cache will incur unnecessary calculations, because the result of the next call to compute will also be an ExecutionException.

If you leave f in the cache, the next call will run f.get() again, which will immediately return with an ExecutionException without having to actually perform the computation.

So it seems more efficient the way it is.

Note that one would expect compute to be idempotent: it makes no sense to cache the result if it could change in the future.

Upvotes: 1

Related Questions