开源软件名称：

开源软件地址：

开源编程语言：

开源软件介绍：

Code Review Checklist: Java Concurrency

Design

• Concurrency is rationalized?
• Can use patterns to simplify concurrency?
  • Immutability/Snapshotting
  • Divide and conquer
  • Producer-consumer
  • Instance confinement
  • Thread/Task/Serial thread confinement
  • Active object
• Code smells, identifying that a class or a subsystem could potentially be redesigned for better:
  • Usage of synchronized with wait/notify instead of concurrency utilities
  • Nested critical sections
  • Extension API call within a critical section
  • Large critical section
  • Waiting in a loop for some result
  • Non-static ThreadLocal
  • Thread.sleep()

Documentation

• Thread safety is justified in comments?
• Class (method, field) has concurrent access documentation?
• Threading model of a subsystem (class) is described?
• Concurrent control flow (or data flow) of a subsystem (class) is described?
• Class is documented as immutable, thread-safe, or not thread-safe?
• Used concurrency patterns are pronounced?
• ConcurrentHashMap is not stored in a variable of Map type?
• compute()-like methods are not called on a variable of ConcurrentMap type?
• @GuardedBy annotation is used?
• Safety of a benign race (e. g. unbalanced synchronization) is explained?
• Each use of volatile is justified?
• Each field that is neither volatile nor annotated with @GuardedBy has a comment?

Insufficient synchronization

• Static methods and fields are thread-safe?
• Thread doesn't wait in a loop for a non-volatile field to be updated by another thread?
• Read access to a non-volatile, concurrently updatable primitive field is protected?
• Servlets, Controllers, Filters, Handlers, @Get/@Post methods are thread-safe?
• Calls to DateFormat.parse() and format() are synchronized?

Excessive thread safety

• No "extra" (pseudo) thread safety?
• No atomics on which only get() and set() are called?
• Class (method) needs to be thread-safe?
• ReentrantLock (ReentrantReadWriteLock, Semaphore) needs to be fair?

Race conditions

• No put() or remove() calls on a ConcurrentMap (or Cache) after get() or containsKey()?
• No point accesses to a non-thread-safe collection outside of critical sections?
• Iteration over a non-thread-safe collection doesn't leak outside of a critical section?
• A non-thread-safe collection is not returned wrapped in Collections.unmodifiable*() from a getter in a thread-safe class?
• A synchronized collection is not returned from a getter? in a thread-safe class?
• Non-trivial mutable object is not returned from a getter in a thread-safe class?
• No separate getters to an atomically updated state?
• No check-then-act race conditions (state used inside a critical section is read outside of it)?
• coll.toArray(new E[coll.size()]) is not called on a synchronized collection?
• No race conditions with user or programmatic input or interop between programs?
• No check-then-act race conditions with file system operations?
• No concurrent invalidate(key) and get() calls on Guava's loading Cache?
• Cache.put() is not used (nor exposed in the own Cache interface)?
• Concurrent invalidation race is not possible on a lazily initialized state?
• Iteration, Stream pipeline, or copying a Collections.synchronized*() collection is protected by the lock?
• A synchronized collection is passed into containsAll(), addAll(), removeAll(), or putAll() under the lock?

Testing

• Considered adding multi-threaded unit tests for a thread-safe class or method?
• What is the worst thing that might happen if the code has a concurrency bug?
• A shared Random instance is not used from concurrent test workers?
• Concurrent test workers coordinate their start?
• There are more test threads than CPUs (if possible for the test)?
• Assertions in parallel threads and asynchronous code are handled properly?
• Checked the result of CountDownLatch.await()?

Locks

• Can use some concurrency utility instead of a lock with conditional wait (await) calls?
• Can use Guava’s Monitor instead of a lock with conditional wait (await) calls?
• Can use synchronized instead of a ReentrantLock?
• lock() is called outside of try {}? No statements between lock() and try {}?

Avoiding deadlocks

• Can avoid nested critical sections?
• Locking order for nested critical sections is documented?
• Dynamically determined locks for nested critical sections are ordered?
• No extension API calls within critical sections?
• No calls to ConcurrentHashMap's methods (incl. get()) in compute()-like lambdas on the same map?

Improving scalability

• Critical section is as small as possible?
• Can use ConcurrentHashMap.compute() or Guava's Striped for per-key locking?
• Can replace a blocking collection or a queue with a concurrent one?
• Can use ClassValue instead of ConcurrentHashMap<Class, ...>?
• Considered ReadWriteLock (or StampedLock) instead of a simple lock?
• StampedLock is used instead of ReadWriteLock when reentrancy is not needed?
• Considered LongAdder instead of an AtomicLong for a "hot field"?
• Considered queues from JCTools instead of the standard concurrent queues?
• Considered Caffeine cache instead of other caching libraries?
• Can apply speculation (optimistic concurrency) technique?
• Considered ForkJoinPool instead of newFixedThreadPool(N)?

Lazy initialization and double-checked locking

• Lazy initialization of a field should be thread-safe?
• Considered double-checked locking for a lazy initialization to improve performance?
• Double-checked locking follows the SafeLocalDCL pattern?
• Considered eager initialization instead of a lazy initialization to simplify code?
• Can do lazy initialization with a benign race and without locking to improve performance?
• Holder class idiom is used for lazy static fields rather than double-checked locking?

Non-blocking and partially blocking code

• Non-blocking code has enough comments to make line-by-line checking as easy as possible?
• Can use immutable POJO + compare-and-swap operations to simplify non-blocking code?
• Boundaries of non-blocking or benignly racy code are identified with WARNING comments?
• Busy waiting (spin loop), all calls to Thread.yield() and Thread.onSpinWait() are justified?

Threads and Executors

• Thread is named?
• Can use ExecutorService instead of creating a new Thread each time some method is called?
• ExecutorServices are not created within short-lived objects (but rather reused)?
• No network I/O in a CachedThreadPool?
• No blocking (incl. I/O) operations in a ForkJoinPool or in a parallel Stream pipeline?
• Can execute non-blocking computation in FJP.commonPool() instead of a custom thread pool?
• ExecutorService is shut down explicitly?
• Callback is attached to a CompletableFuture (SettableFuture) in non-async mode only if either:
  • the callback is lightweight and non-blocking; or
  • the future is completed and the callback is attached from the same thread pool?
• Adding a callback to a CompletableFuture (SettableFuture) in non-async mode is justified?
• Actions are delayed via a ScheduledExecutorService rather than Thread.sleep()?
• Checked the result of awaitTermination()?
• ExecutorService is not assigned into a variable of Executor type?
• ScheduledExecutorService is not assigned into a variable of ExecutorService type?

Parallel Streams

• Parallel Stream computation takes more than 100us in total?
• Comment before a parallel Streams pipeline explains how it takes more than 100us in total?

Futures

• Non-blocking computation needs to be decorated as a Future?
• Method returning a Future doesn't block?
• In a method returning a Future, considered wrapping an "expected" exception as a failed Future?

Thread interruption and Future cancellation

• Interruption status is restored before wrapping InterruptedException with another exception?
• InterruptedException is swallowed only in the following kinds of methods:
  • Runnable.run(), Callable.call(), or methods to be passed to executors as lambda tasks; or
  • Methods with "try" or "best effort" semantics?
• InterruptedException swallowing is documented for a method?
• Can use Guava's Uninterruptibles to avoid InterruptedException swallowing?
• Future is canceled upon catching an InterruptedException or a TimeoutException on get()?

Time

• nanoTime() values are compared in an overflow-aware manner?
• currentTimeMillis() is not used to measure time intervals and timeouts?
• Units for a time variable are identified in the variable's name or via TimeUnit?
• Negative timeouts and delays are treated as zeros?
• Tasks connected to system time or UTC time are not scheduled using ScheduledThreadPoolExecutor?
• Human and external interactions on consumer devices are not scheduled using ScheduledThreadPoolExecutor?

ThreadLocal

• ThreadLocal can be static final?
• Can redesign a subsystem to avoid usage of ThreadLocal (esp. non-static one)?
• ThreadLocal is not used just to avoid moderate amount of allocation?
• Considered replacing a non-static ThreadLocal with an instance-confined Map<Thread, ...>?

Thread safety of Cleaners and native code

• close() is concurrently idempotent in a class with a Cleaner or finalize()?
• Method accessing native state calls reachabilityFence() in a class with a Cleaner or finalize()?
• Cleaner or finalize() is used for real cleanup, not mere reporting?
• Considered making a class with native state thread-safe?

Design

# Dn.1. If the patch introduces a new subsystem (class, method) with concurrent code, is the necessity for concurrency or thread safety 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)?

A way to nudge thinking about concurrency design is demanding the usage of concurrency tools and language constructs to be justified in comments.

See also an item about unneeded thread-safety of classes and methods.

# Dn.2. Is it possible to apply one or several design patterns (some of them are listed below) to significantly simplify the concurrency model of the code, while not considerably compromising other quality aspects, such as overall simplicity, efficiency, testability, extensibility, etc?

Immutability/Snapshotting. When some state should be updated, a new immutable object (or a snapshot within a mutable object) is created, published and used, while some concurrent threads may still use older copies or snapshots. See [EJ Item 17], [JCIP 3.4], RC.5 and NB.2, CopyOnWriteArrayList, CopyOnWriteArraySet, persistent data structures.

Divide and conquer. Work is split into several parts that are processed independently, each part in a single thread. Then the results of processing are combined. Parallel Streams or ForkJoinPool (see TE.4 and TE.5) can be used to apply this pattern.

Producer-consumer. Pieces of work are transmitted between worker threads via queues. See [JCIP 5.3], Dl.1, CSP, SEDA.

Instance confinement. Objects of some root type encapsulate some complex hierarchical child state. Root objects are solitarily responsible for the safety of accesses and modifications to the child state from multiple threads. In other words, composed objects are synchronized rather than synchronized objects are composed. See [JCIP 4.2, 10.1.3, 10.1.4]. RC.3, RC.4, and RC.5 describe race conditions that could happen to instance-confined state. TL.4 touches on instance confinement of thread-local state.

Thread/Task/Serial thread confinement. Some state is made local to a thread using top-down pass-through parameters or ThreadLocal. See [JCIP 3.3] and the checklist section about ThreadLocals. Task confinement is a variation of the idea of thread confinement that is used in conjunction with the divide-and-conquer pattern. It usually comes in the form of lambda-captured "context" parameters or fields in the per-thread task objects. Serial thread confinement is an extension of the idea of thread confinement for the producer-consumer pattern, see [JCIP 5.3.2].

Active object. An object manages its own ExecutorService or Thread to do the work. See the article on Wikipedia and TE.6.

Documentation

# Dc.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 access 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?

Wherever some logic is parallelized or the execution is delegated to another thread, are there comments explaining why it’s worse or inappropriate to execute the logic sequentially or in the same thread? See PS.1 regarding this.

See also NB.3 and NB.4 regarding justification of non-blocking code, racy code, and busy waiting.

If the usage of concurrency tools is not justified, there is a possibility of code ending up with unnecessary thread-safety, redundant atomics, redundant volatile modifiers, or unneeded Futures.

# Dc.2. If the patch introduces a new subsystem that uses threads or thread pools, are there high-level descriptions of the threading model, the concurrent control flow (or the data flow) of the subsystem somewhere, e. g. in the Javadoc comment for the package in package-info.java or for the main class of the subsystem? Are these descriptions kept up-to-date when new threads or thread pools are added or some old ones deleted from the system?

Description of the threading model includes the enumeration of threads and thread pools created and managed in the subsystem, and external pools used in the subsystem (such as ForkJoinPool.commonPool()), their sizes and other important characteristics such as thread priorities, and the lifecycle of the managed threads and thread pools.

A high-level description of concurrent control flow should be an overview and tie together concurrent control flow documentation for individual classes, see the previous item. If the producer-consumer pattern is used, the concurrent control flow is trivial and the data flow should be documented instead.

Describing threading models and control/data flow greatly improves the maintainability of the system, because in the absence of descriptions or diagrams developers spend a lot of time and effort to create and refresh these models in their minds. Putting the models down also helps to discover bottlenecks and the ways to simplify the design (see Dn.2).

# Dc.3. For classes and methods that are parts of the public API or the extensions API of the project: is it specified in their Javadoc comments whether they are (or in case of interfaces and abstract classes designed for subclassing in extensions, should they be implemented as) immutable, thread-safe, or not thread-safe? For classes and methods that are (or should be implemented as) thread-safe, is it documented precisely with what other methods (or themselves) they may be called concurrently from multiple threads? See also [EJ Item 82], [JCIP 4.5], CON52-J.

If the @com.google.errorprone.annotations.Immutable annotation is used to mark immutable classes, Error Prone static analysis tool is capable to detect when a class is not actually immutable (see a relevant bug pattern).

# Dc.4. For subsystems, classes, methods, and fields that use some concurrency design patterns, either high-level, such as those mentioned in Dn.2, or low-level, such as double-checked locking or spin looping: are the used concurrency patterns pronounced in the design or implementation comments for the respective subsystems, classes, methods, and fields? This helps readers to make sense out of the code quicker.

Pronouncing the used patterns in comments may be replaced with more succinct documentation annotations, such as @Immutable (Dc.3), @GuardedBy (Dc.7), @LazyInit (LI.5), or annotations that you define yourself for specific patterns which appear many times in your project.

# Dc.5. Are ConcurrentHashMap and ConcurrentSkipListMap objects stored in fields and variables of ConcurrentHashMap or ConcurrentSkipListMap or ConcurrentMap type, but not just Map?

This is important, because in code like the following:

ConcurrentMap<String, Entity> entities = getEntities();
if (!entities.containsKey(key)) {
  entities.put(key, entity);
} else {
  ...
}

It should be pretty obvious that there might be a race condition because an entity may be put into the map by a concurrent thread between the calls to containsKey() and put() (see RC.1 about this type of race conditions). While if the type of the entities variable was just Map<String, Entity> it would be less obvious and readers might think this is only slightly suboptimal code and pass by.

It’s possible to turn this advice into an inspection in IntelliJ IDEA.

# Dc.6. An extension of the previous item: are ConcurrentHashMaps on which compute(), computeIfAbsent(), computeIfPresent(), or merge() methods are called stored in fields and variables of ConcurrentHashMap type rather than ConcurrentMap? This is because ConcurrentHashMap (unlike the generic ConcurrentMap interface) guarantees that the lambdas passed into compute()-like methods are performed atomically per key, and the thread safety of the class may depend on that guarantee.

This advice may seem to be overly pedantic, but if used in conjunction with a static analysis rule that prohibits calling compute()-like methods on ConcurrentMap-typed objects that are not ConcurrentHashMaps (it’s possible to create such inspection in IntelliJ IDEA too) it could prevent some bugs: e. g. calling compute() on a ConcurrentSkipListMap might be a race condition and it’s easy to overlook that for somebody who is used to rely on the strong semantics of compute() in ConcurrentHashMap.

# Dc.7. Is @GuardedBy annotation used? If accesses to some fields should be protected by some lock, are those fields annotated with @GuardedBy? Are private methods that are called from within critical sections in other methods annotated with @GuardedBy? If the project doesn’t depend on any library containing this annotation (it’s provided by jcip-annotations, error_prone_annotations, jsr305, and other libraries) and for some reason it’s undesirable to add such dependency, it should be mentioned in Javadoc comments for the respective fields and methods that accesses and calls to them should be protected by some specified locks.

See [JCIP 2.4] for more information about @GuardedBy.

Usage of @GuardedBy is especially beneficial in conjunction with Error Prone tool which is able to statically check for unguarded accesses to fields and methods with @GuardedBy annotations. There is also an inspection "Unguarded field access" in IntelliJ IDEA with the same effect.

# Dc.8. If in a thread-safe class some fields are accessed both from within critical sections and outside of critical sections, is it explained in comments why this is safe? For example, unprotected read-only access to a reference to an immutable object might be benignly racy (see RC.5). Answering this question also helps to prevent problems described in IS.2, IS.3, and RC.2.

Instead of writing a comment explaining that access to a lazily initialized field outside of a critical section is safe, the field could just be annotated with @LazyInit from error_prone_annotations (but make sure to read the Javadoc for this annotation and to check that the field conforms to the description; LI.3 and LI.5 mention potential pitfalls).

Apart from the explanations why the partially blocking or racy code is safe, there should also be comments justifying such error-prone code and warning the developers that the code should be modified and reviewed with double attention: see NB.3.

There is an inspection "Field accessed in both synchronized and unsynchronized contexts" in IntelliJ IDEA which helps to find classes with unbalanced synchronization.

# Dc.9. Regarding every field with a volatile modifier: does it really need to be volatile? Does the Javadoc comment for the field explain why the semantics of volatile field reads and writes (as defined in the Java Memory Model) are required for the field?

Similarly to what is noted in the previous item, justification for a lazily initialized field to be volatile could be omitted if the lazy initialization pattern itself is identified, according to Dc.4. When volatile on a field is needed to ensure safe publication of objects written into it (see [JCIP 3.5] or here) or linearizability of values observed by reader threads, then just mentioning "safe publication" or "linearizable reads" in the Javadoc comment for the field is sufficient, it's not needed to elaborate the semantics of volatile which ensure the safe publication or the linearizability.

By extension, this item also applies when an AtomicReference (or a primitive atomic) is used instead of raw volatile, along with the consideration about unnecessary atomic which might also be relevant in this case.

# Dc.10. Is it explained in the Javadoc comment for each mutable field in a thread-safe class that is neither volatile nor annotated with @GuardedBy, why that is safe? Perhaps, the field is only accessed and mutated from a single method or a set of methods that are specified to be called only from a single thread sequentially (described as per Dc.1). This recommendation also applies to final fields that store objects of non-thread-safe classes when those objects could be mutated from some methods of the enclosing thread-safe class. See IS.2, IS.3, RC.2, RC.3, and RC.4 about what could go wrong with such code.

Insufficient synchronization

# IS.1. Can non-private static methods be called concurrently from multiple threads? If there is a non-private static field with mutable state, such as a collection, is it an instance of a thread-safe class or synchronized using some Collections.synchronizedXxx() method?

Note that calls to DateFormat.parse() and format() must be synchronized because they mutate the object: see IS.5.

# IS.2. There is no situation where some thread awaits until a non-volatile field has a certain value in a loop, expecting it to be updated from another thread? The field should at least be volatile to ensure eventual visibility of concurrent updates. See [JCIP 3.1, 3.1.4], and VNA00-J for more details and examples.

Even if the respective field is volatile, busy waiting for a condition in a loop can be abused easily and therefore should be justified in a comment: see NB.4.

Dc.10 also demands adding explaining comments to mutable fields which are neither volatile nor annotated with @GuardedBy which should inevitably lead to the discovery of the visibility issue.

# IS.3. Read accesses to non-volatile primitive fields which can be updated concurrently are protected with a lock as well as the writes? The minimum reason for this is that reads of long and double fields are non-atomic (see JLS 17.7, [JCIP 3.1.2], VNA05-J). But even with other types of fields, unbalanced synchronization creates possibilities for downstream bugs related to the visibility (see the previous item) and the lack of the expected happens-before relationships.

As well as with the previous item, accurate documentation of benign races (Dc.8 and Dc.10) should reliably expose the cases when unbalanced synchronization is problematic.

See also RC.2 regarding unbalanced synchronization of read accesses to mutable objects, such as collections.

There is a relevant inspection "Field accessed in both synchronized and unsynchronized contexts" in IntelliJ IDEA.

# IS.4. Is the business logic written for server frameworks thread-safe? This includes:

• Servlet implementations
• @(Rest)Controller-annotated classes, @Get/PostMapping-annotated methods in Spring
• @SessionScoped and @ApplicationScoped managed beans in JSF
• Filter and Handler implementations in various synchronous and asynchronous frameworks (including Jetty, Netty, Undertow)
• @GET- and @POST-annotated methods (resources) in JAX-RS (RESTful APIs)

It's easy to forget that if such code mutates some state (e. g. fields in the class), it must be properly synchronized or access only concurrent collections and classes.

# IS.5. Calls to parse() and format() on a shared instance of DateFormat are synchronized, e. g. if a DateFormat is stored in a static field? Although parse() and format() may look "read-only", they actually mutate the receiving DateFormat object.

An inspection "Non thread-safe static field access" in IntelliJ IDEA helps to catch such concurrency bugs.

Excessive thread safety

# ETS.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.

There shouldn’t be any "extra" thread safety in code, there should be just enough of it. Duplication of thread safety confuses readers because they might think the extra thread safety precautions are (or used to be) needed for something but will fail to find the purpose.

The exception from this principle is the volatile modifier on the lazily initialized field in the safe local double-checked locking pattern which is the recommended way to implement double-checked locking, despite that volatile is excessive for correctness when the lazily initialized object has all final fields*. Without that volatile modifier the thread safety of the double-checked locking could easily be broken by a change (addition of a non-final field) in the class of lazily initialized objects, though that class should not be aware of subtle concurrency implications. If the class of lazily initialized objects is specified to be immutable (see Dc.3) the volatile is still unnecessary and the UnsafeLocalDCL pattern could be used safely, but the fact that some class has all final fields doesn’t necessarily mean that it’s immutable.

See also the section about double-checked locking.

# ETS.2. Aren’t there AtomicReference, AtomicBoolean, AtomicInteger or AtomicLong fields on which only get() and set() methods are called? Simple fields with volatile modifiers can be used instead, but volatile might not be needed too; see Dc.9.

# ETS.3. Does a class (method) need to be thread-safe? May a class be accessed (method called) concurrently from multiple threads (without happens-before relationships between the accesses or calls)? Can a class (method) be simplified by making it non-thread-safe?

See also Ft.1 about unneeded wrapping of a computation into a Future and Dc.9 about potentially unneeded volatile modifiers.

This item is a close relative of Dn.1 (about rationalizing concurrency and thread safety in the patch description) and Dc.1 (about justifying concurrency in Javadocs for classes and methods, and documenting concurrent access). If these actions are done, it should be self-evident whether the class (method) needs to be thread-safe or not. There may be cases, however, when it might be desirable to make the class (method) thread-safe although it's not supposed to be accessed or called concurrently as of the moment of the patch. For example, thread safety may be needed to ensure memory safety (see CN.4 about this). Anticipating some changes to the codebase that make the class (method) being accessed from multiple threads may be another reason to make the class (method) thread-safe up front.

# ETS.4. Does a ReentrantLock (or ReentrantReadWriteLock, Semaphore) need to be fair? To justify the throughput penalty of making a lock fair it should be demonstrated that a lack of fairness leads to unacceptably long starvation periods in some threads trying to acquire the lock or pass the semaphore. This should be documented in the Javadoc comment for the field holding the lock or the semaphore. See [JCIP 13.3] for more details.

Race conditions

# RC.1. Aren’t ConcurrentMap (or Cache) objects updated with separate containsKey(), get(), put() and remove() calls instead of a single call to compute()/computeIfAbsent()/computeIfPresent()/replace()?

# RC.2. Aren’t there point read accesses such as Map.get(), containsKey() or List.get() outside of critical sections to a non-thread-safe collection such as HashMap or ArrayList, while new entries can be added to the collection concurrently, even though there is a happens-before edge between the moment when some entry is put into the collection and the moment when the same entry is point-queried outside of a critical section?

The problem is that when new entries can be added to a collection, it grows and changes its internal structure from time to time (HashMap rehashes the hash table, ArrayList reallocates the internal array). At such moments races might happen and unprotected point read accesses might fail with NullPointerException, ArrayIndexOutOfBoundsException, or return null or some random entry.

Note that this concern applies to ArrayList even when elements are only added to the end of the list. However, a small change in ArrayList’s implementation in OpenJDK could have disallowed data races in such cases at very little cost. If you are subscribed to the concurrency-interest mailing list, you could help to bring attention to this problem by reviving this thread.

See also IS.3 regarding unbalanced synchronization of accesses to primitive fields.

# RC.3. A variation of the previous item: isn’t a non-thread-safe collection such as HashMap or ArrayList iterated outside of a critical section, while it may be modified concurrently? This could happen by accident when an Iterable, Iterator or Stream over a collection is returned from a method of a thread-safe class, even though the iterator or stream is created within a critical section. Note that returning unmodifiable collection views like Collections.unmodifiableList() from getters wrapping collection fields that may be modified concurrently doesn't solve this problem. If the collection is relatively small, it should be copied entirely, or a copy-on-write collection (see Sc.3) should be used instead of a non-thread-safe collection.

Note that calling toString() on a collection (e. g. in a logging statement) implicitly iterates over it.

Like the previous item, this one applies to growing ArrayLists too.

This item applies even to synchronized collections: see RC.10 for details.

# RC.4. Generalization of the previous item: aren’t non-trivial objects that can be mutated concurrently returned from getters in a thread-safe class (and thus inevitably leaking outside of critical sections)?

# RC.5. If there are multiple variables in a thread-safe class that are updated at once but have individual getters, isn’t there a race condition in the code that calls those getters? If there is, the variables should be made final fields in a dedicated POJO, that serves as a snapshot of the updated state. The POJO is stored in a field of the thread-safe class, directly or as an AtomicReference. Multiple getters to individual fields should be replaced with a single getter that returns the POJO. This allows avoiding a race condition in the client code by reading a consistent snapshot of the state at once.

This pattern is also very useful for creating safe and reasonably simple non-blocking code: see NB.2 and [JCIP 15.3.1].

# RC.6. If some logic within some critical section depends on some data that principally is part of the internal mutable state of the class, but was read outside of the critical section or in a different critical section, isn’t there a race condition because the local copy of the data may become out of sync with the internal state by the time when the critical section is entered? This is a typical variant of check-then-act race condition, see [JCIP 2.2.1].

An example of this race condition is calling toArray() on synchronized collections with a sized array:

list = Collections.synchronizedList(new ArrayList<>());
...
elements = list.toArray(new Element[list.size()]);

This might unexpectedly leave the elements array with some nulls in the end if there are some concurrent removals from the list. Therefore, toArray() on a synchronized collection should be called with a zero-length array: toArray(new Element[0]), which is also not worse from the performance perspective: see "Arrays of Wisdom of the Ancients".

See also RC.9 about cache invalidation races which are similar to check-then-act races.

# RC.7. Aren't there race conditions between the code (i. e. program runtime actions) and some actions in the outside world or actions performed by some other programs running on the machine? For example, if some configurations or credentials are hot reloaded from some file or external registry, reading separate configuration parameters or separate credentials (such as username and password) in separate transactions with the file or the registry may be racing with a system operator updating those configurations or credentials.

Another example is checking that a file exists (or not exists) and then reading, deleting, or creating it, respectively, while another program or a user may delete or create the file between the check and the act. It's not always possible to cope with such race conditions, but it's useful to keep such possibilities in mind. Prefer static methods from java.nio.file.Files class and NIO file reading/writing API to methods from the old java.io.File for file system operations. Methods from Files are more sensitive to file system race conditions and tend to throw exceptions in adverse cases, while methods on File swallow errors and make it hard even to detect race conditions. Static methods from Files also support StandardOpenOption.CREATE and CREATE_NEW which may help to ensure some extra atomicity.

# RC.8. If you are using Guava Cache and invalidate(key), are you not affected by the race condition which can leave a Cache with an invalid (stale) value mapped for a key? Consider using Caffeine cache which doesn't have this problem. Caffeine is also faster and more scalable than Guava Cache: see Sc.9.

# RC.9. Generalization of the previous item: isn't there a potential cache invalidation race in the code? There are several ways to get into this problem:

• Using Cache.put() method concurrently with invalidate(). Unlike RC.8, this is a race regardless of what caching library is used, not necessarily Guava. This is also similar to RC.1.
• Having put() and invalidate() methods exposed in the own Cache interface. This places the burden of synchronizing put() (together the preceding "checking" code, such as get()) and invalidate() calls on the users of the API which really should be the job of the Cache implementation.
• There is some lazily initialized state in a mutable object which can be invalidated upon mutation of the object, and can also be accessed concurrently with the mutation. This means the class is in the category of non-blocking concurrency: see the corresponding checklist items. A way to avoid cache invalidation race in this case is to wrap the primary state and the cached state into a POJO and replace it atomically, as described in NB.2.

# RC.10. The whole iteration loop over a synchronized collection (i. e. obtained from one of the Collections.synchronizedXxx() static factory methods), or a Stream pipeline using a synchronized collection as a source is protected by synchronized (coll)? See the Javadoc for examples and details.

This also applies to passing synchronized collections into:

• Copy constructors of other collections, e. g. new ArrayList<>(synchronizedColl)
• Static factory methods of other collections, e. g. List.copyOf(), Set.copyOf(), ImmutableMap.copyOf()
• Bulk methods on other collections:
  • otherColl.containsAll(synchronizedColl)
  • otherColl.addAll(synchronizedColl)
  • otherColl.removeAll(synchronizedColl)
  • otherMap.putAll(synchronizedMap)
  • otherColl.containsAll(synchronizedMap.keySet())
  • Etc.

Because in all these cases there is an implicit iteration on the source collection.

See also RC.3 about unprotected iteration over non-thread-safe collections.

Testing

# T.1. Was it considered to add multi-threaded unit tests for a thread-safe class or method? Single-threaded tests don't really test the thread safety and concurrency. Note that this question doesn't mean to indicate that there must be concurrent unit tests for every piece of concurrent code in the project because correct concurrent tests take a lot of effort to write and therefore they might often have low ROI.

What is the worst thing that might happen if this code has a concurrency bug? This is a useful question to inform the decision about writing concurrent tests. The consequences may range from a tiny, entirely undetectable memory leak, to storing corrupted data in a durable database or a security breach.

# T.2. Isn't a shared java.util.Random object used for data generation in a concurrency test? java.util.Random is synchronized internally, so if multiple test threads (which are conceived to access the tested class concurrently) access the same java.util.Random object then the test might degenerate to a mostly synchronous one and fail to exercise the concurrency properties of the tested class. See [JCIP 12.1.3]. Math.random() is a subject for this problem too because internally Math.random() uses a globally shared java.util.Random instance. Use ThreadLocalRandom instead.

# T.3. Do concurrent test workers coordinate their start using a latch such as CountDownLatch? If they don't much or even all of the test work might be done by the first few started workers. See [JCIP 12.1.3] for more information.

# T.4. Are there more test threads than there are available processors if possible for the test? This will help to generate more thread scheduling interleavings and thus test the logic for the absence of race conditions more thoroughly. See [JCIP 12.1.6] for more information. The number of available processors on the machine can be obtained as Runtime.getRuntime().availableProcessors().

# T.5. Are there no regular assertions in code that is executed not in the main thread running the unit test? Consider the following example: