CODEWITHSUNDEEP
javathread-safety
Jörn Horstmann
Jörn Horstmann

Reputation: 34024

Is an assignment inside ConcurrentHashMap.computeIfAbsent threadsafe?

Consider the following implementation of some kind of fixed size cache, that allows lookup by an integer handle:

static class HandleCache {
    private final AtomicInteger counter = new AtomicInteger();
    private final Map<Data, Integer> handles = new ConcurrentHashMap<>();
    private final Data[] array = new Data[100_000];

    int getHandle(Data data) {
        return handles.computeIfAbsent(data, k -> {
            int i = counter.getAndIncrement();
            if (i >= array.length) {
                throw new IllegalStateException("array overflow");
            }
            array[i] = data;
            return i;
        });

    }

    Data getData(int handle) {
        return array[handle];
    }
}

There is an array store inside the compute function, which is not synchronized in any way. Would it be allowed by the java memory model for other threads to read a null value from this array later on?

PS: Would the outcome change if the id returned from getHandle was stored in a final field and only accessed through this field from other threads?

Upvotes: 3

Views: 1049

Answers (3)

David Soroko
David Soroko

Reputation: 9096

Assuming that you determine the index for the array read from the value of counter than yes - you may get a null read

The simplest example (there are others) is a follows:

T1 calls getHandle(data) and is suspended just after int i = counter.getAndIncrement(); T2 calls handles[counter.get()] and reads null.

You should be able to easily verify this with a strategically placed sleep and two threads.

Upvotes: 1

Peter Lawrey
Peter Lawrey

Reputation: 533660

The read access isn't thread safe. You could make it thread safe indirectly however it's likely to be brittle. I would implemented it in a much simpler way and only optimise it later should it prove to a performance problem. e.g. because you see it in a profiler for a realistic test.

static class HandleCache {
    private final Map<Data, Integer> handles = new HashMap<>();
    private final List<Data> dataByIndex = new ArrayList<>();

    synchronized int getHandle(Data data) {
        Integer id = handles.get(data);
        if (id == null) {
             id = handles.size();
             handles.put(data, id);
             dataByIndex.add(id);
        }
        return id;
    }

    synchronized Data getData(int handle) {
        return dataByIndex.get(handle);
    }
}

Upvotes: 1

Jacob G.
Jacob G.

Reputation: 29710

From the documentation of ConcurrentHashMap#computeIfAbsent:

The entire method invocation is performed atomically, so the function is applied at most once per key. Some attempted update operations on this map by other threads may be blocked while computation is in progress, so the computation should be short and simple, and must not attempt to update any other mappings of this map.

The documentation's reference to blocking refers only to update operations on the Map, so if any other thread attempts to access array directly (rather than through an update operation on the Map), there can be race conditions and null can be read.

Upvotes: 0

Related Questions