ThreadLocal pollution in HystrixTimer causing metric attribution errors due to unsafe thread reuse

[QiuYucheng2003]

I identified a ThreadLocal pollution issue in com.netflix.hystrix.util.HystrixTimer.

The HystrixTimer uses a ScheduledThreadPoolExecutor to reuse threads for timer tasks. However, it lacks a mechanism to clean up ThreadLocal context after task execution. Critically, the Javadoc for TimerListener.tick() explicitly encourages users to set ThreadLocals inside the method (Lines 190-192), but the run() implementation (Lines 96-106) does not ensure these are cleared, especially if an exception occurs or the user forgets the finally block.

Source Code Location:
// HystrixTimer.java

// 1. Misleading Javadoc (Lines 190-192):
// "If you need a ThreadLocal set, you can store the state in the TimerListener,
// then when tick() is called, set the ThreadLocal to your desired value."

// 2. Unsafe Execution (Lines 96-106):
Runnable r = new Runnable() {
@OverRide
public void run() {
try {
listener.tick();
} catch (Exception e) {
logger.error("Failed while ticking TimerListener", e);
}
// MISSING: Finally block to clear/reset ThreadLocals or HystrixRequestContext
}
};

Impact

1. Metric Deviation: Latency and timeout metrics may be recorded under the wrong CommandKey or CommandGroup if the HystrixRequestContext leaks from a previous execution on the same thread.

2. Context Pollution: Downstream logic executed by the timer thread may inherit stale user contexts (e.g., User ID, Trace ID).

Expected behavior
The HystrixTimer should either:

1. Sanitize the thread state before/after execution.

2. Or, at minimum, the Javadoc should strictly warn users that try-finally cleanup is mandatory because threads are reused.

Actual behavior
Threads in the HystrixTimer pool retain ThreadLocal values from previous tick() executions, leading to "cross-talk" between different Hystrix commands.

Suggested Fix
Update the Runnable wrapper to ensure cleanup, or update documentation to warn about thread pooling risks.

Code Fix Example:
Runnable r = new Runnable() {
@OverRide
public void run() {
try {
listener.tick();
} catch (Exception e) {
logger.error(...);
} finally {
// Ideally: HystrixRequestContext.getContextForCurrentThread().shutdown();
// Or explicitly warn users to clean up.
}
}
};

[ThinkingMan007]

好的，我已收到。