druid/dev/code-review/concurrency.md

Druid's Checklist for Concurrency Code

Design

• Concurrency is rationalized in the PR description?
• Can use patterns to simplify concurrency?
  • Immutability/Snapshotting
  • Divide and conquer
  • Producer-consumer
  • Instance confinement
  • Thread/Task/Serial thread confinement

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?
• Applied concurrency patterns are pronounced?
• @GuardedBy annotation is used?
• Safety of benign races is explained?
• Each use of volatile is justified?
• Field that is neither volatile nor annotated with @GuardedBy has a comment?

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?

Race conditions

• No put() or remove() calls on a ConcurrentHashMap 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?
• Non-trivial object is not returned from a getter in a thread-safe class?
• No separate getters to an atomically updated state?
• No state used for making decisions or preparing data inside a critical section is read outside?
• No race conditions are possible between the program and users or other programs?
• No race conditions are possible on the file system?

Replacing locks with concurrency utilities

• Can use LifecycleLock instead of a standard lock in a lifecycled object?
• Can use concurrency utility instead of Object.wait()/notify()?
• Can use Guava’s Monitor instead of a standard lock with conditional waits?

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?

Improving scalability

• Critical section is as small as possible?
• Can use ConcurrentHashMap.compute() or Guava's Striped for per-key locking?
• Can replace 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"?

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?
• Considered eager initialization instead of a lazy initialization to simplify code?
• Double-checked locking follows the SafeLocalDCL pattern?
• Can do lazy initialization with a benign race and without locking to improve performance?

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 by WARNING comments?

Threads and Executors

• Thread is named?
• Thread is daemon?
• Using Execs to create an ExecutorService?
• Can use ExecutorService instead of creating a new Thread each time some method is called?
• 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?

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?

Thread interruption and Future cancellation

• Interruption status is restored before propagating a wrapped InterruptedException?
• InterruptedException is swallowed only in the following kinds of methods?
  • Runnable.run(), Callable.call(), or methods to be passed to executors as lambda tasks
  • 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?

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?

---

# Lk.D1. Is it possible to use Druid's LifecycleLock utility instead of a standard lock and "started" flag in lifecycled objects with start() and stop() methods? See the Javadoc comment for LifecycleLock for more details.

# TE.D1. Are Threads created directly or via a ThreadFactory configured to be daemon via setDaemon(true)? Note that by default, ThreadFactories constructed via Execs.makeThreadFactory() methods create daemon threads already.

# TE.D2. Is it possible to use one of the static factory methods in Druid's Execs utility class to create an ExecutorService instead of Java's standard ExecutorServices? This is recommended because Execs configure ThreadFactories to create daemon threads by default, as required by the previous item.