Code review checklist: how to tackle issues with Java concurrency

Photo by J. Kelly Brito on Unsplash

1. Design

1.1. If the patch introduces a new subsystem with concurrent code, is the necessity for concurrency rationalized in the patch description? Is there a discussion of alternative design approaches that could simplify the concurrency model of the code (see the next item)?

2. Documentation

2.1. For every class, method, and field that has signs of being thread-safe, such as the synchronized keyword, volatile modifiers on fields, use of any classes from java.util.concurrent.*, or third-party concurrency primitives, or concurrent collections: do their Javadoc comments include

  • The justification for thread safety: is it explained why a particular class, method or field has to be thread-safe?
  • Concurrent control flow documentation: is it enumerated from what methods and in contexts of what threads (executors, thread pools) each specific method of a thread-safe class is called?
ConcurrentMap<String, Entity> entities = getEntities();
if (!entities.containsKey(key)) {
entities.put(key, entity);
} else {

3. Excessive thread safety

3.1. An example of excessive thread safety is a class where every modifiable field is volatile or an AtomicReference or other atomic, and every collection field stores a concurrent collection (e. g. ConcurrentHashMap), although all accesses to those fields are synchronized.

4. Race conditions

4.1. Aren’t ConcurrentHashMaps updated with multiple separate containsKey(), get(), put() and remove() calls instead of a single call to compute()/computeIfAbsent()/computeIfPresent()/replace()?

5. Replacing locks with concurrency utilities

5.1. Is it possible to use concurrent collections and/or utilities from java.util.concurrent.* and avoid using locks with Object.wait()/notify()/notifyAll()? Code redesigned around concurrent collections and utilities is often both clearer and less error-prone than code implementing the equivalent logic with intrinsic locks, Object.wait() and notify() (Lock objects with await() and signal() are not different in this regard). See [EJ Item 81] for more information.

6. Avoiding deadlocks

6.1. If a thread-safe class is implemented so that there are nested critical sections protected by different locks, is it possible to redesign the code to get rid of nested critical sections? Sometimes a class could be split into several distinct classes, or some work that is done within a single thread could be split between several threads or tasks which communicate via concurrent queues. See [JCIP 5.3] for more information about the producer-consumer pattern.

7. Improving scalability

7.1. Are critical sections as small as possible? For every critical section: can’t some statements in the beginning and the end of the section be moved out of it? Not only minimizing critical sections improves scalability, but also makes it easier to review them and spot race conditions and deadlocks.

  • Collections.synchronizedMap(HashMap), HashtableConcurrentHashMap
  • Collections.synchronizedSet(HashSet)ConcurrentHashMap.newKeySet()
  • Collections.synchronizedMap(TreeMap)ConcurrentSkipListMap. By the way, ConcurrentSkipListMap is not the state of the art concurrent sorted dictionary implementation. SnapTree is more efficient than ConcurrentSkipListMap and there have been some research papers presenting algorithms that are claimed to be more efficient than SnapTree.
  • Collections.synchronizedSet(TreeSet)ConcurrentSkipListSet
  • Collections.synchronizedList(ArrayList), VectorCopyOnWriteArrayList
  • LinkedBlockingQueueConcurrentLinkedQueue
  • LinkedBlockingDequeConcurrentLinkedDeque

8. Lazy initialization and double-checked locking

8.1. For every lazily initialized field: is the initialization code thread-safe and might it be called from multiple threads concurrently? If the answers are “no” and “yes”, either double-checked locking should be used or the initialization should be eager.

private MyClass lazilyInitializedField;void foo() {
if (lazilyInitializedField != null) { // (1)
// Can throw NPE!; // (2)
void foo() {
MyClass lazilyInitialized = this.lazilyInitializedField;
if (lazilyInitialized != null) {;

9. Non-blocking and partially blocking code

9.1. If there is some non-blocking or semi-symmetrically blocking code that mutates the state of a thread-safe class, was it deliberately checked that if a thread on a non-blocking mutation path is preempted after each statement, the object is still in a valid state? Are there enough comments, perhaps before almost every statement where the state is changed, to make it relatively easy for readers of the code to repeat and verify the check?

10. Threads and Executors

10.1. Are Threads given names when created? Are ExecutorServices created with thread factories that name threads?

11. Thread interruption and Future cancellation

11.1. If some code propagates InterruptedException wrapped into another exception (e. g. RuntimeException), is the interruption status of the current thread restored before the wrapping exception is thrown?

  • or themselves, or methods that are intended to be submitted as tasks to some Executors as method references. Thread.currentThread().interrupt() should still be called before returning from the method, assuming that the interruption policy of the threads in the Executor is unknown.
  • Methods with “try” or “best effort” semantics. Documentation for such methods should be clear that they stop attempting to do something when the thread is interrupted, restore the interruption status of the thread and return.

12. Time

12.1. Are values returned from System.nanoTime() compared in an overflow-aware manner, as described in the documentation for this method?

13. Thread safety of Cleaners and native code

13.1. If a class manages native resources and employs java.lang.ref.Cleaner (or sun.misc.Cleaner; or overrides Object.finalize()) to ensure that resources are freed when objects of the class are garbage collected, and the class implements Closeable with the same cleanup logic executed from close() directly rather than through Cleanable.clean() (or sun.misc.Cleaner.clean()) to be able to distinguish between explicit close() and cleanup through a cleaner (for example, clean() can log a warning about the object not being closed explicitly before freeing the resources), is it ensured that even if the cleanup logic is called concurrently from multiple threads, the actual cleanup is performed only once? The cleanup logic in such classes should obviously be idempotent because it’s usually expected to be called twice: the first time from the close() method and the second time from the cleaner or finalize(). The catch is that the cleanup must be concurrently idempotent, even if close() is never called concurrently on objects of the class. That’s because the garbage collector may consider the object to become unreachable before the end of a close() call and initiate cleanup through the cleaner or finalize() while close() is still being executed.

14. Parallel Streams

14.1. For every use of parallel Streams via Collection.parallelStream() or Stream.parallel(): is it explained why parallel Stream is used in a comment preceding the stream operation? Are there back-of-the-envelope calculations or references to benchmarks showing that the total CPU time cost of the parallelized computation exceeds 100 microseconds?

Reading List



We’ve moved to and publish tons of tutorials each week. See you there.

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store