Race condition when creating HystrixThreadPool

[nurkiewicz]

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.

[cloudbees-pull-request-builder]

Hystrix-pull-requests #169 FAILURE
Looks like there's a problem with this pull request

[benjchristensen]

Great bug report, thank you for fixing this.