Netflix/Hystrix — pull request #340 by nurkiewicz (2014-11-10 13:28:56+00:00)
Title: Race condition when creating HystrixThreadPool

# Symptoms

Different thread pool is referenced in `AbstractCommand` and a different one in `HystrixThreadPoolMetrics` for the same key. This means monitoring always shows `0` in active thread, etc. because that pool is never touched. This happens only when pool is first initialized from multiple threads. Pseudo code exposing bug:

final ExecutorService pool = Executors.newFixedThreadPool(10);
new SomeCommand().execute();
pool.submit(() -> {
new SomeCommand().execute();
});

Code above shows correct pool statistics because second line safely initialized one thread pool. However if second line is commented, race condition causes different thread pool to be registered for statistics and different one for actual execution. I believe I can turn this into a test-case if necessary.

# Steps to reproduce:

1. Two or more threads are executing instances of the same command for the first time
2. Both threads enter `HystrixThreadPool.Factory.getInstance()` via `AbstractCommand` c-tor
3. `threadPools` static map is empty for both threads, thus they both hit `threadPools.putIfAbsent(key, new HystrixThreadPoolDefault(...));`
4. Two instances of `HystrixThreadPoolDefault` are created eagerly, first one (arbitrary) registers itself in `HystrixThreadPoolMetrics.metrics` inside `HystrixThreadPoolMetrics#getInstance(...)`
5. Back in `HystrixThreadPool.Factory.getInstance()` this: `threadPools.putIfAbsent(key, ...)` is executed. However there is no guarantee that `threadPools.putIfAbsent()` will put the same thread pool as the one in `HystrixThreadPoolMetrics.metrics`

# Severity

This bug is not critical, but causes dashboard to show incorrect statistics.

# Fix

My fix is straightforward: just put `synchronized` around lazy initialization. Previous code was more clever and probably faster - but broken. I understand `synchronized` was almost absent in this class, moreover we already deal with thread-safe collections. I considered another approach: instead of eagerly creating new `HystrixThreadPoolDefault` in `putIfAbsent()`, use another (new) implementation of `HystrixThreadPool` as a marker:

HystrixThreadPool poolForKey = threadPools.putIfAbsent(key, DummyHystrixThreadPool.INSTANCE);
if (poolForKey == null) {
final HystrixThreadPoolDefault threadPoolJustCreated = new HystrixThreadPoolDefault(threadPoolKey, propertiesBuilder);
threadPools.put(key, threadPoolJustCreated);
return threadPoolJustCreated;
} else {
return poolForKey;
}

But this seems hacky and hard to read. Also by introducing second implementation of `HystrixThreadPool` JVM can't make such aggressive optimizations (inlining) due to multiple implementations loaded. This is largely a premature optimization.