Safe publication of a ConcurrentHashMap into a class field

Question

I am trying to write a test that demonstrates that assigning a new reference to a class' field in a multi-threading environment is not thread-safe and more specifically has visibility problems if that field is not declared as volatile or AtomicReference.

The scenario I use is a PropertiesLoader class (shown below), which is supposed to load a set of properties (currently only one property is used) stored in a Map<String, String> and also tries to support reloading. So there are many threads reading a property and at some point in time another thread is reloading a new value that needs to be visible to the reading threads.

The test is intended to work as following:

• it invokes the reader threads which are spin-waiting until they "see" the property value change
• at some point the writer thread creates a new map with a new value for the property and assigns that map to the field in question (PropertyLoader.propertiesMap)
• if all reader threads see the new value the test is completed otherwise it hangs forever.

Now I know that strictly speaking, there is no test that can prove the thread-safeness of some code (or the lack of it) but in this case I feel like it should be relatively easy to demonstrate the problem at least empirically.

I have tried using a HashMap implementation to store the properties and in this case the test hangs as expected even if I use only one reading thread.

If however, a ConcurrentHashMap implementation is used, the test never hangs no matter how many reading threads are being used (I have also tried waiting randomly in the reader threads with no success).

As far as my understanding goes, the fact that ConcurrentHashMap is thread-safe should not affect the visibility of the field where it is assigned to. So volatile/AtomicReference is still required for that field. However the above test seems to contradicts this since it behaves as if the map is always safely published without the need of additional synchronization.

Is my understanding wrong? Perhaps ConcurrentHashMap makes some additional synchronization promises that I am not aware of?

Any help would be highly appreciated.

P.S. The code below should be executable as is as a Junit test. I have run it in a machine with AMD Ryzen 5, Windows 10, JDK 1.8.0_201 and in a second machine i7 Intel, Fedora 30, JDK 1.8.xx (not remember the exact version of JDK) with the same results.

import org.junit.Test;

import java.util.HashMap;
import java.util.Map;
import java.util.Random;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CountDownLatch;

public class PropertiesLoaderTest {

    private static final String NEW_VALUE = "newValue";
    private static final String OLD_VALUE = "oldValue";
    private static final String PROPERTY = "property";

    /**
     *  Controls if the reference we are testing for visibility issues ({@link PropertiesLoader#propertyMap} will
     *  be assigned a HashMap or ConcurrentHashMap implementation during {@link PropertiesLoader#load(boolean)}
     */
    private static boolean USE_SIMPLE_MAP = false;

    @Test
    public void testReload() throws Exception {
        PropertiesLoader loader = new PropertiesLoader();
        Random random = new Random();

        int readerThreads = 5;
        int totalThreads = readerThreads + 1;

        final CountDownLatch startLatch = new CountDownLatch(1);
        final CountDownLatch finishLatch = new CountDownLatch(totalThreads);

        // start reader threads that read the property trying to see the new property value
        for (int i = 0; i < readerThreads; i++) {
            startThread("reader-thread-" + i, startLatch, finishLatch, () -> {
                while (true) {
                    String value = loader.getProperty(PROPERTY);
                    if (NEW_VALUE.equals(value)) {
                        log("Saw new value: " + value + " for property: " + PROPERTY);
                        break;
                    }
                }
            });
        }

        // start writer thread (i.e. the thread that reloads the properties)
        startThread("writer-thread", startLatch, finishLatch, () -> {
            Thread.sleep(random.nextInt(500));

            log("starting reload...");
            loader.reloadProperties();
            log("finished reload...");
        });

        log("Firing " + readerThreads + " threads and 1 writer thread...");
        startLatch.countDown();

        log("Waiting for all threads to finish...");
        finishLatch.await();
        log("All threads finished. Test successful");
    }

    static class PropertiesLoader {
        // The reference in question: this is assigned in the constructor and again when calling reloadProperties()
        // It is not volatile nor AtomicReference so there are visibility concerns
        Map<String, String> propertyMap;

        PropertiesLoader() {
            this.propertyMap = load(false);
        }

        public void reloadProperties() {
            this.propertyMap = load(true);
        }

        public String getProperty(String propertyName) {
            return propertyMap.get(propertyName);
        }

        private static Map<String, String> load(boolean isReload) {
            // using a simple HashMap always hang the test as expected: the new reference cannot be
            // seen by the reader thread

            // using a ConcurrentHashMap always allow the test to finish no matter how many reader
            // threads are used
            Map<String, String> newMap = USE_SIMPLE_MAP ? new HashMap<>() : new ConcurrentHashMap<>();
            newMap.put(PROPERTY, isReload ? NEW_VALUE : OLD_VALUE);
            return newMap;
        }
    }

    static void log(String msg) {
        //System.out.println(Thread.currentThread().getName() + " - " + msg);
    }

    static void startThread(String name, CountDownLatch start, CountDownLatch finish, ThreadTask task) {
        Thread t = new Thread(new ThreadTaskRunner(name, start, finish, task));
        t.start();
    }

    @FunctionalInterface
    interface ThreadTask {
        void execute() throws Exception;
    }

    static class ThreadTaskRunner implements Runnable {
        final CountDownLatch start;
        final CountDownLatch finish;
        final ThreadTask task;
        final String name;

        protected ThreadTaskRunner(String name, CountDownLatch start, CountDownLatch finish, ThreadTask task) {
            this.start = start;
            this.finish = finish;
            this.task = task;
            this.name = name;
        }

        @Override
        public void run() {
            try {
                Thread.currentThread().setName(name);
                start.await();
                log("thread started");
                task.execute();
                log("thread finished successfully");
            } catch (Exception e) {
                log("Error: " + e.getMessage());
            }
            finish.countDown();
        }
    }
}

Answer 1

It's a bit worse than you might think but there is also a saving grace.

The bit worse part: constructors are not synchronized. In this case that means that the PropertiesLoader.propertyMap which is created in the constructor is not guaranteed to be visible to the other threads (reader or writer). Your saving grace here is the CountDownLatches you use (these establish a happen-before relation) as well as the Thread.start (which also establish a happen-before relation) . Also, in practice "constructors are not synchronized" is rarely a problem and difficult to reproduce (see also test-code below). For more information on the matter, please read this question. Conclusion is that the PropertiesLoader.propertyMap must either be volatile / AtomicReference or final (final could be used in combination with the ConcurrentHashMap).

The reason you cannot reproduce the synchronization issue with a ConcurrentHashMap is the same reason it is difficult to reproduce the "constructors are not synchronized" problem: a ConcurrentHashMap uses synchronization internally (see this answer) which triggers a memory flush that not only makes the new values in the map visible to other threads, but also the new PropertiesLoader.propertyMap value.

Note that a volatile PropertiesLoader.propertyMap will guarantee (and not just make it likely) that new values are visible to other threads (ConcurrentHashMap is not required, see also this answer). I usually set these kind of maps to a read-only map (with the help of Collections.unmodifiableMap()) to broadcast to other programmers that this is not an ordinary map that can be updated or changed at will.

Below some more test-code which tries to eliminate as much synchronization as possible. The end-result for the test is exactly the same but it also shows the side-effect of having a volatile boolean in a loop and that the non-null assignment of propertyMap somehow is always seen by other threads.

package so;

import java.util.HashMap;
import java.util.Map;
import java.util.Random;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import java.util.stream.IntStream;

public class MapVisibility {

    static int readerThreadsAmount = 2;

    public static void main(String[] args) {

        ExecutorService executors = Executors.newFixedThreadPool(readerThreadsAmount);
        try {
            new MapVisibility().run(executors);
        } catch (Exception e) {
            e.printStackTrace();
        } finally {
            executors.shutdownNow(); // Does not work on FAIL, manually kill reader-task from task-manager.
        }
    }

    //final boolean useConcurrentMap = false;
    // When ConcurrentHashMap is used, test is always a success.
    final boolean useConcurrentMap = true;

    final boolean useStopBoolean = false;
    // When volatile stop boolean is used, test is always a success.
    //final boolean useStopBoolean = true;

    //final boolean writeToConsole = false;
    // Writing to System.out is synchronized, this can make a test succeed that would otherwise fail.
    final boolean writeToConsole = true;

    Map<String, String> propertyMap;
    // When the map is volatile, test is always a success.
    //volatile Map<String, String> propertyMap;

    final String oldValue = "oldValue";
    final String newValue = "newValue";
    final String key = "key";
    volatile boolean stop;

    void run(ExecutorService executors) throws Exception {

        IntStream.range(0,  readerThreadsAmount).forEach(i -> {
            executors.execute(new MapReader());
        });
        sleep(500); // give readers a chance to start
        setMap(oldValue);
        sleep(100); // give readers a chance to read map
        setMap(newValue);
        sleep(100); // give readers a chance to read new value in new map
        executors.shutdown();
        if (!executors.awaitTermination(100L, TimeUnit.MILLISECONDS)) {
            System.out.println("FAIL");
            stop = true;
        } else {
            System.out.println("Success");
        }
    }

    void setMap(String value) {

        Map<String, String> newMap = (useConcurrentMap ? new ConcurrentHashMap<>() : new HashMap<>());
        newMap.put(key, value);
        propertyMap = newMap;
    }

    class MapReader implements Runnable {

        @Override
        public void run() {
            print("Reader started.");
            final long startTime = System.currentTimeMillis();
            while (propertyMap == null) {
                // In worse case, this loop should never exit but it always does.
                // No idea why.
                sleep(1);
            }
            print((System.currentTimeMillis() - startTime) + " Reader got map.");
            if (useStopBoolean) {
                while (!stop) {
                    if (newValue.equals(propertyMap.get(key))) {
                        break;
                    }
                }
            } else {
                while (true) {
                    if (newValue.equals(propertyMap.get(key))) {
                        break;
                    }
                }
            }
            print((System.currentTimeMillis() - startTime) + " Reader got new value.");
        }
    }

    void print(String msg) {
        if (writeToConsole) {
            System.out.println(msg);
        }
    }

    void sleep(int timeout) {

        // instead of using Thread.sleep, do some busy-work instead.
        final long startTime = System.currentTimeMillis();
        Random r = new Random();
        @SuppressWarnings("unused")
        long loopCount = 0;
        while (System.currentTimeMillis() - startTime < timeout) {
            for (int i = 0; i < 100_000; i++) {
                double d = r.nextDouble();
                double v = r.nextDouble();
                @SuppressWarnings("unused")
                double dummy = d / v;
            }
            loopCount++;
        }
        //print("Loops: " + loopCount);
    }

}