mirror of https://github.com/apache/druid.git
251 lines
16 KiB
Markdown
251 lines
16 KiB
Markdown
<!--
|
||
~ Licensed to the Apache Software Foundation (ASF) under one
|
||
~ or more contributor license agreements. See the NOTICE file
|
||
~ distributed with this work for additional information
|
||
~ regarding copyright ownership. The ASF licenses this file
|
||
~ to you under the Apache License, Version 2.0 (the
|
||
~ "License"); you may not use this file except in compliance
|
||
~ with the License. You may obtain a copy of the License at
|
||
~
|
||
~ http://www.apache.org/licenses/LICENSE-2.0
|
||
~
|
||
~ Unless required by applicable law or agreed to in writing,
|
||
~ software distributed under the License is distributed on an
|
||
~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
|
||
~ KIND, either express or implied. See the License for the
|
||
~ specific language governing permissions and limitations
|
||
~ under the License.
|
||
-->
|
||
|
||
## Druid's Checklist for Concurrency Code
|
||
|
||
Design
|
||
- [Concurrency is rationalized in the PR description?](
|
||
https://github.com/code-review-checklists/java-concurrency#rationalize)
|
||
- [Can use patterns to simplify concurrency?](https://github.com/code-review-checklists/java-concurrency#use-patterns)
|
||
- Immutability/Snapshotting
|
||
- Divide and conquer
|
||
- Producer-consumer
|
||
- Instance confinement
|
||
- Thread/Task/Serial thread confinement
|
||
- Active object
|
||
|
||
Documentation
|
||
- [Thread safety is justified in comments?](
|
||
https://github.com/code-review-checklists/java-concurrency#justify-document)
|
||
- [Class (method, field) has concurrent access documentation?](
|
||
https://github.com/code-review-checklists/java-concurrency#justify-document)
|
||
- [Threading model of a subsystem (class) is described?](
|
||
https://github.com/code-review-checklists/java-concurrency#threading-flow-model)
|
||
- [Concurrent control flow (or data flow) of a subsystem (class) is described?](
|
||
https://github.com/code-review-checklists/java-concurrency#threading-flow-model)
|
||
- [Class is documented as immutable, thread-safe, or not thread-safe?](
|
||
https://github.com/code-review-checklists/java-concurrency#immutable-thread-safe)
|
||
- [Used concurrency patterns are pronounced?](
|
||
https://github.com/code-review-checklists/java-concurrency#name-patterns)
|
||
- [`@GuardedBy` annotation is used?](https://github.com/code-review-checklists/java-concurrency#guarded-by)
|
||
- [Safety of a benign race (e. g. unbalanced synchronization) is explained?](
|
||
https://github.com/code-review-checklists/java-concurrency#document-benign-race)
|
||
- [Each use of `volatile` is justified?](https://github.com/code-review-checklists/java-concurrency#justify-volatile)
|
||
- [Each field that is neither `volatile` nor annotated with `@GuardedBy` has a comment?](
|
||
https://github.com/code-review-checklists/java-concurrency#plain-field)
|
||
|
||
Insufficient synchronization
|
||
- [Static methods and fields are thread-safe?](
|
||
https://github.com/code-review-checklists/java-concurrency#static-thread-safe)
|
||
- [Thread *doesn't* wait in a loop for a non-volatile field to be updated by another thread?](
|
||
https://github.com/code-review-checklists/java-concurrency#non-volatile-visibility)
|
||
- [Read access to a non-volatile, concurrently updatable primitive field is protected?](
|
||
https://github.com/code-review-checklists/java-concurrency#non-volatile-protection)
|
||
- [`@GET`/`@POST` methods, Jetty Filters and Handlers are thread-safe?](
|
||
https://github.com/code-review-checklists/java-concurrency#server-framework-sync)
|
||
- [Calls to `DateFormat.parse()` and `format()` are synchronized?](
|
||
https://github.com/code-review-checklists/java-concurrency#dateformat)
|
||
|
||
Excessive thread safety
|
||
- [No "extra" (pseudo) thread safety?](https://github.com/code-review-checklists/java-concurrency#pseudo-safety)
|
||
- [No atomics on which only `get()` and `set()` are called?](
|
||
https://github.com/code-review-checklists/java-concurrency#redundant-atomics)
|
||
- [Class (method) needs to be thread-safe?](
|
||
https://github.com/code-review-checklists/java-concurrency#unneeded-thread-safety)
|
||
- [`ReentrantLock` (`ReentrantReadWriteLock`, `Semaphore`) needs to be fair?](
|
||
https://github.com/code-review-checklists/java-concurrency#unneeded-fairness)
|
||
|
||
Race conditions
|
||
- [No `put()` or `remove()` calls on a `ConcurrentMap` (or Cache) after `get()` or `containsKey()`?](
|
||
https://github.com/code-review-checklists/java-concurrency#chm-race)
|
||
- [No point accesses to a non-thread-safe collection outside of critical sections?](
|
||
https://github.com/code-review-checklists/java-concurrency#unsafe-concurrent-point-read)
|
||
- [Iteration over a non-thread-safe collection doesn't leak outside of a critical section?](
|
||
https://github.com/code-review-checklists/java-concurrency#unsafe-concurrent-iteration)
|
||
- [Non-trivial object is *not* returned from a getter in a thread-safe class?](
|
||
https://github.com/code-review-checklists/java-concurrency#concurrent-mutation-race)
|
||
- [No separate getters to an atomically updated state?](
|
||
https://github.com/code-review-checklists/java-concurrency#moving-state-race)
|
||
- [No *check-then-act* race conditions (state used inside a critical section is read outside of it)?](
|
||
https://github.com/code-review-checklists/java-concurrency#read-outside-critical-section-race)
|
||
- [`coll.toArray(new E[coll.size()])` is *not* called on a synchronized collection?](
|
||
https://github.com/code-review-checklists/java-concurrency#read-outside-critical-section-race)
|
||
- [No race conditions with user or programmatic input or interop between programs?](
|
||
https://github.com/code-review-checklists/java-concurrency#outside-world-race)
|
||
- [No check-then-act race conditions with file system operations?](
|
||
https://github.com/code-review-checklists/java-concurrency#outside-world-race)
|
||
- [No concurrent `invalidate(key)` and `get()` calls on Guava's loading `Cache`?](
|
||
https://github.com/code-review-checklists/java-concurrency#guava-cache-invalidation-race)
|
||
- [`Cache.put()` is not used (nor exposed in the own Cache interface)?](
|
||
https://github.com/code-review-checklists/java-concurrency#cache-invalidation-race)
|
||
- [Concurrent invalidation race is not possible on a lazily initialized state?](
|
||
https://github.com/code-review-checklists/java-concurrency#cache-invalidation-race)
|
||
- [Iteration, Stream pipeline, or copying a `Collections.synchronized*()` collection is protected by a lock?](
|
||
https://github.com/code-review-checklists/java-concurrency#synchronized-collection-iter)
|
||
|
||
Testing
|
||
- [Unit tests for thread-safe classes are multi-threaded?](
|
||
https://github.com/code-review-checklists/java-concurrency#multi-threaded-tests)
|
||
- [A shared `Random` instance is *not* used from concurrent test workers?](
|
||
https://github.com/code-review-checklists/java-concurrency#concurrent-test-random)
|
||
- [Concurrent test workers coordinate their start?](
|
||
https://github.com/code-review-checklists/java-concurrency#coordinate-test-workers)
|
||
- [There are more test threads than CPUs (if possible for the test)?](
|
||
https://github.com/code-review-checklists/java-concurrency#test-workers-interleavings)
|
||
- [Assertions in parallel threads and asynchronous code are handled properly?](
|
||
https://github.com/code-review-checklists/java-concurrency#concurrent-assert)
|
||
|
||
Locks
|
||
- [Can use `LifecycleLock` instead of a standard lock in a lifecycled object?](#use-lifecycle-lock)
|
||
- [Can use some concurrency utility instead of a lock with conditional `wait` (`await`) calls?](
|
||
https://github.com/code-review-checklists/java-concurrency#avoid-wait-notify)
|
||
- [Can use Guava’s `Monitor` instead of a lock with conditional `wait` (`await`) calls?](
|
||
https://github.com/code-review-checklists/java-concurrency#guava-monitor)
|
||
- [Can use `synchronized` instead of a `ReentrantLock`?](
|
||
https://github.com/code-review-checklists/java-concurrency#use-synchronized)
|
||
- [`lock()` is called outside of `try {}`? No statements between `lock()` and `try {}`?](
|
||
https://github.com/code-review-checklists/java-concurrency#lock-unlock)
|
||
|
||
Avoiding deadlocks
|
||
- [Can avoid nested critical sections?](
|
||
https://github.com/code-review-checklists/java-concurrency#avoid-nested-critical-sections)
|
||
- [Locking order for nested critical sections is documented?](
|
||
https://github.com/code-review-checklists/java-concurrency#document-locking-order)
|
||
- [Dynamically determined locks for nested critical sections are ordered?](
|
||
https://github.com/code-review-checklists/java-concurrency#dynamic-lock-ordering)
|
||
- [No extension API calls within critical sections?](
|
||
https://github.com/code-review-checklists/java-concurrency#non-open-call)
|
||
- [No calls to `ConcurrentHashMap`'s methods (incl. `get()`) in `compute()`-like lambdas on the same map?](
|
||
https://github.com/code-review-checklists/java-concurrency#chm-nested-calls)
|
||
|
||
Improving scalability
|
||
- [Critical section is as small as possible?](
|
||
https://github.com/code-review-checklists/java-concurrency#minimize-critical-sections)
|
||
- [Can use `ConcurrentHashMap.compute()` or Guava's `Striped` for per-key locking?](
|
||
https://github.com/code-review-checklists/java-concurrency#increase-locking-granularity)
|
||
- [Can replace blocking collection or a queue with a concurrent one?](
|
||
https://github.com/code-review-checklists/java-concurrency#non-blocking-collections)
|
||
- [Can use `ClassValue` instead of `ConcurrentHashMap<Class, ...>`?](
|
||
https://github.com/code-review-checklists/java-concurrency#use-class-value)
|
||
- [Considered `ReadWriteLock` (or `StampedLock`) instead of a simple lock?](
|
||
https://github.com/code-review-checklists/java-concurrency#read-write-lock)
|
||
- [`StampedLock` is used instead of `ReadWriteLock` when reentrancy is not needed?](
|
||
https://github.com/code-review-checklists/java-concurrency#use-stamped-lock)
|
||
- [Considered `LongAdder` instead of an `AtomicLong` for a "hot field"?](
|
||
https://github.com/code-review-checklists/java-concurrency#long-adder-for-hot-fields)
|
||
- [Considered queues from JCTools instead of the standard concurrent queues?](
|
||
https://github.com/code-review-checklists/java-concurrency#jctools)
|
||
- [Caffeine cache is used instead of Guava?](https://github.com/apache/druid/issues/8399)
|
||
- [Can apply speculation (optimistic concurrency) technique?](
|
||
https://github.com/code-review-checklists/java-concurrency#speculation)
|
||
|
||
Lazy initialization and double-checked locking
|
||
- [Lazy initialization of a field should be thread-safe?](
|
||
https://github.com/code-review-checklists/java-concurrency#lazy-init-thread-safety)
|
||
- [Considered double-checked locking for a lazy initialization to improve performance?](
|
||
https://github.com/code-review-checklists/java-concurrency#use-dcl)
|
||
- [Double-checked locking follows the SafeLocalDCL pattern?](
|
||
https://github.com/code-review-checklists/java-concurrency#safe-local-dcl)
|
||
- [Considered eager initialization instead of a lazy initialization to simplify code?](
|
||
https://github.com/code-review-checklists/java-concurrency#eager-init)
|
||
- [Can do lazy initialization with a benign race and without locking to improve performance?](
|
||
https://github.com/code-review-checklists/java-concurrency#lazy-init-benign-race)
|
||
- [Holder class idiom is used for lazy static fields rather than double-checked locking?](
|
||
https://github.com/code-review-checklists/java-concurrency#no-static-dcl)
|
||
|
||
Non-blocking and partially blocking code
|
||
- [Non-blocking code has enough comments to make line-by-line checking as easy as possible?](
|
||
https://github.com/code-review-checklists/java-concurrency#check-non-blocking-code)
|
||
- [Can use immutable POJO + compare-and-swap operations to simplify non-blocking code?](
|
||
https://github.com/code-review-checklists/java-concurrency#swap-state-atomically)
|
||
- [Boundaries of non-blocking or benignly racy code are identified with WARNING comments?](
|
||
https://github.com/code-review-checklists/java-concurrency#non-blocking-warning)
|
||
|
||
Threads and Executors
|
||
- [Thread is named?](https://github.com/code-review-checklists/java-concurrency#name-threads)
|
||
- [Thread is daemon?](#daemon-threads)
|
||
- [Using `Execs` to create an `ExecutorService`?](#use-execs)
|
||
- [Can use `ExecutorService` instead of creating a new `Thread` each time some method is called?](
|
||
https://github.com/code-review-checklists/java-concurrency#reuse-threads)
|
||
- [No network I/O in a CachedThreadPool?](
|
||
https://github.com/code-review-checklists/java-concurrency#cached-thread-pool-no-io)
|
||
- [No blocking (incl. I/O) operations in a `ForkJoinPool` or in a parallel Stream pipeline?](
|
||
https://github.com/code-review-checklists/java-concurrency#fjp-no-blocking)
|
||
- [Can execute non-blocking computation in `FJP.commonPool()` instead of a custom thread pool?](
|
||
https://github.com/code-review-checklists/java-concurrency#use-common-fjp)
|
||
- [`ExecutorService` is shutdown explicitly?](
|
||
https://github.com/code-review-checklists/java-concurrency#explicit-shutdown)
|
||
|
||
Parallel Streams
|
||
- [Parallel Stream computation takes more than 100us in total?](
|
||
https://github.com/code-review-checklists/java-concurrency#justify-parallel-stream-use)
|
||
- [Comment before a parallel Streams pipeline explains how it takes more than 100us in total?](
|
||
https://github.com/code-review-checklists/java-concurrency#justify-parallel-stream-use)
|
||
|
||
Thread interruption and `Future` cancellation
|
||
- [Interruption status is restored before wrapping `InterruptedException` with another exception?](
|
||
https://github.com/code-review-checklists/java-concurrency#restore-interruption)
|
||
- [`InterruptedException` is swallowed only in the following kinds of methods](
|
||
https://github.com/code-review-checklists/java-concurrency#interruption-swallowing):
|
||
- `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?](
|
||
https://github.com/code-review-checklists/java-concurrency#interruption-swallowing)
|
||
- [Can use Guava's `Uninterruptibles` to avoid `InterruptedException` swallowing?](
|
||
https://github.com/code-review-checklists/java-concurrency#interruption-swallowing)
|
||
- [`Future` is canceled upon catching an `InterruptedException` or a `TimeoutException` on `get()`?](
|
||
https://github.com/code-review-checklists/java-concurrency#cancel-future)
|
||
|
||
Time
|
||
- [`nanoTime()` values are compared in an overflow-aware manner?](
|
||
https://github.com/code-review-checklists/java-concurrency#nano-time-overflow)
|
||
- [`currentTimeMillis()` is *not* used to measure time intervals and timeouts?](
|
||
https://github.com/code-review-checklists/java-concurrency#time-going-backward)
|
||
- [Units for a time variable are identified in the variable's name or via `TimeUnit`?](
|
||
https://github.com/code-review-checklists/java-concurrency#time-units)
|
||
- [Negative timeouts and delays are treated as zeros?](
|
||
https://github.com/code-review-checklists/java-concurrency#treat-negative-timeout-as-zero)
|
||
|
||
Thread safety of Cleaners and native code
|
||
- [`close()` is concurrently idempotent in a class with a `Cleaner` or `finalize()`?](
|
||
https://github.com/code-review-checklists/java-concurrency#thread-safe-close-with-cleaner)
|
||
- [Method accessing native state calls `reachabilityFence()` in a class with a `Cleaner` or `finalize()`?](
|
||
https://github.com/code-review-checklists/java-concurrency#reachability-fence)
|
||
- [`Cleaner` or `finalize()` is used for real cleanup, not mere reporting?](
|
||
https://github.com/code-review-checklists/java-concurrency#finalize-misuse)
|
||
- [Considered making a class with native state thread-safe?](
|
||
https://github.com/code-review-checklists/java-concurrency#thread-safe-native)
|
||
|
||
<hr>
|
||
|
||
<a name="use-lifecycle-lock"></a>
|
||
[#](#use-lifecycle-lock) 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.
|
||
|
||
<a name="daemon-threads"></a>
|
||
[#](#daemon-threads) 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.
|
||
|
||
<a name="use-execs"></a>
|
||
[#](#use-execs) 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.
|