Java Threading Unexpected Behavior

Question

We have been looking at a threading error for a while and are not sure how this is possible. Below is a minimized example from our code. There is a cache holding data retrieved from a database (or: "a lengthy synchronous operation", as far as this example is concerned). There is a thread for reloading the cache, while other threads try to query the cache. There is a period of time when the cache is null, waiting to be reloaded. It should not be queryable during this time, and we tried to enforce this by synchronizing the methods that access the cache - both for reading and writing. Yet if you run this class for a while, you will get NPEs in search(). How is this possible?

Java docs state that "it is not possible for two invocations of synchronized methods on the same object to interleave. When one thread is executing a synchronized method for an object, all other threads that invoke synchronized methods for the same object block (suspend execution) until the first thread is done with the object".

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

public class CacheMultithreading01 {
    private long dt = 1000L;

    public static void main(String[] args) {
        CacheMultithreading01 cm = new CacheMultithreading01();
        cm.demonstrateProblem();
    }

    void demonstrateProblem() {
        QueryableCache cache = new QueryableCache();
        runInLoop("Reload", new Runnable() {
            @Override
            public void run() {
                cache.reload();
            }
        });
        runInLoop("Search", new Runnable() {
            @Override
            public void run() {
                cache.search(2);
            }
        });
        // If the third "runInLoop" is commented out, no NPEs
        runInLoop("_Clear", new Runnable() {
            @Override
            public void run() {
                cache.clear();
            }
        });
    }

    void runInLoop(String threadName, Runnable r) {
        new Thread(new Runnable() {
            @Override
            public synchronized void run() {
                while (true) {
                    try {
                        r.run();
                    } catch (Exception e) {
                        log("Error");
                        e.printStackTrace();
                    }
                }
            }
        }, threadName).start();
    }

    void log(String s) {
        System.out.format("%d %s %s\n", System.currentTimeMillis(), Thread
                .currentThread().getName(), s);
    }

    class QueryableCache {
        private List<Integer> cache = new ArrayList<>();

        public synchronized void reload() {
            clear();
            slowOp(); // simulate retrieval from database
            cache = new ArrayList<>(Arrays.asList(1, 2, 3));
        }

        public synchronized void clear() {
            cache = null;
        }

        public synchronized Integer search(Integer element) {
            if (cache.contains(element))
                return element;
            else
                return null;
        }

        private void slowOp() {
            try {
                Thread.sleep(dt);
            } catch (InterruptedException e) {
            }
        }
    }
}
//java.lang.NullPointerException
//at examples.multithreading.cache.CacheMultithreading01$QueryableCache.search(CacheMultithreading01.java:73)
//at examples.multithreading.cache.CacheMultithreading01$2.run(CacheMultithreading01.java:26)
//at examples.multithreading.cache.CacheMultithreading01$4.run(CacheMultithreading01.java:44)
//at java.lang.Thread.run(Thread.java:745)

We do not understand why the NPEs can happen even though the code is synchronized. We also do not understand why the NPEs stop happening if we comment out the third call to runInLoop (the one that does cache.clear). We have also tried to implement locking using a ReentrantReadWriteLock - and the result is the same.

Answer 1

Since you don't have any more advanced locking, you can call clear() and search() consecutively. That will obviously cause a NPE.

Calling reload() and search() won't cause problems, since in reload the cache is cleared, then rebuilt, inside a synchronized block, preventing other (search) operations from being executed in between.

Why is there a clear() method that will leave cache in a "bad" state (which search() doesn't even check for)?

Answer 2

You have to check in the search method if cache is null. Otherwise calling contains on it in search can throw a NullPointerException in the case that you have previously set cache to null in the clear-method.