Why doesn't Java Concurrency In Practice Listing 5.18 run atomically with a lock?

Question

In Java Concurrency in Practice on page 106 it says: " Memoizer3 is vulnerable to the problem [two threads see zero and start costly calculations] because the composite action (put-if-absent) is performed on a substrate map that cannot be made atomic, using lock. " I do not understand why they say that it is impossible to make atomic using a lock. Here is the source code:

 package net.jcip.examples; import java.util.*; import java.util.concurrent.*; /** * Memoizer3 * <p/> * Memoizing wrapper using FutureTask * * @author Brian Goetz and Tim Peierls */ public class Memoizer3 <A, V> implements Computable<A, V> { private final Map<A, Future<V>> cache = new ConcurrentHashMap<A, Future<V>>(); private final Computable<A, V> c; public Memoizer3(Computable<A, V> c) { this.c = c; } public V compute(final A arg) throws InterruptedException { 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 = ft; cache.put(arg, ft); ft.run(); // call to c.compute happens here } try { return f.get(); } catch (ExecutionException e) { throw LaunderThrowable.launderThrowable(e.getCause()); } } } 

Why not try this job?

 ... public V compute(final A arg) throws InterruptedException { Future<V> f = null; FutureTask<V> ft = null; synchronized(this){ f = cache.get(arg); if (f == null) { Callable<V> eval = new Callable<V>() { public V call() throws InterruptedException { return c.compute(arg); } }; ft = new FutureTask<V>(eval); f = ft; cache.put(arg, ft); } } if (f==ft) ft.run(); // call to c.compute happens here ... 

Answer 1

Of course, you can make atomic using the lock, imagine the most primitive case: you have a global lock around your entire function, then everything is single-threaded and, therefore, thread-safe. I assume that either they had in mind something else, or there was a general misunderstanding.

Your code can even be improved using the putIfAbsent method for ConcurrentHashMap as follows:

 public V compute(final A arg) throws InterruptedException { Future<V> f = cache.get(arg); if (f == null) { final Callable<V> eval = new Callable<V>() { public V call() throws InterruptedException { return c.compute(arg); } }; final FutureTask<V> ft = new FutureTask<V>(eval); final Future<V> previousF = cache.putIfAbsent(arg, ft); if (previousF == null) { f = ft; ft.run(); } else { f = previousF; // someone else will do the compute } } return f.get(); } 

f at the end will be either the previous value that was added between or the initial value, and the potential cost of the additional creation of Callable, but the call for calculation is not performed more than once.