fix hystrix plugin cause biz error #4857
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4857 +/- ##
============================================
+ Coverage 50.71% 51.17% +0.45%
- Complexity 2745 2754 +9
============================================
Files 752 1331 +579
Lines 18556 28926 +10370
Branches 1798 3159 +1361
============================================
+ Hits 9411 14802 +5391
- Misses 8394 13447 +5053
+ Partials 751 677 -74 Continue to review full report at Codecov.
|
wu-sheng
added
agent
| if (!HystrixRequestContext.isCurrentThreadInitialized()) { | ||
| return null; | ||
| } | ||
| return super.get(); |
There was a problem hiding this comment.
So, this could be NPE, right? In some old versions.
There was a problem hiding this comment.
this is super.get() method
public T get() {
if (HystrixRequestContext.getContextForCurrentThread() == null) {
throw new IllegalStateException(HystrixRequestContext.class.getSimpleName() + ".initializeContext() must be called at the beginning of each request before RequestVariable functionality can be used.");
} else {
ConcurrentHashMap<HystrixRequestVariableDefault<?>, HystrixRequestVariableDefault.LazyInitializer<?>> variableMap = HystrixRequestContext.getContextForCurrentThread().state;
HystrixRequestVariableDefault.LazyInitializer<?> v = (HystrixRequestVariableDefault.LazyInitializer)variableMap.get(this);
if (v != null) {
return v.get();
} else {
HystrixRequestVariableDefault.LazyInitializer<T> l = new HystrixRequestVariableDefault.LazyInitializer(this);
HystrixRequestVariableDefault.LazyInitializer<?> existing = (HystrixRequestVariableDefault.LazyInitializer)variableMap.putIfAbsent(this, l);
return existing == null ? l.get() : existing.get();
}
}
}
super.get() will trigger IllegalStateException, in some old versions
There was a problem hiding this comment.
Then next question is, why this method is being called? Is it called by SkyWalking plugin codes?
There was a problem hiding this comment.
it is called by hystrix AbstractCommand constructor.
There was a problem hiding this comment.
My question is, the old version caller, AbstractCommand, should not call this, right? Otherwise, it triggers Illegal issue itself. Could you be more clear what context is missing
There was a problem hiding this comment.
concurrencyStrategy is HystrixConcurrencyStrategyDefault instance in no skywalking agent, but skywalking plugin wrapped it to HystrixConcurrencyStrategy instance. not HystrixConcurrencyStrategyDefault. thus skip "concurrencyStrategy instanceof HystrixConcurrencyStrategyDefault" condition. i don't know where context be initialize, but when "concurrencyStrategy instanceof HystrixConcurrencyStrategyDefault" condition, it checkd this "HystrixRequestContext.isCurrentThreadInitialized()".
There was a problem hiding this comment.
Then, why don't we change the SkyWalking wrapper logic? It seems more reasonable, right?
There was a problem hiding this comment.
With offline communication, right now, this seems the easiest solution w/o harm. I will keep this for now. Thanks.
Please answer these questions before submitting pull request
Why submit this pull request?
Bug fix
New feature provided
Improve performance
Related issues
hystrix plugin cause biz error #4850
Bug fix
this problem is, origin object HystrixConcurrencyStrategyDefault is wrapped to HystrixConcurrencyStrategy. under hystrix 1.4.3 com.netflix.hystrix.HystrixRequestLog#getCurrentRequest(com.netflix.hystrix.strategy.concurrency.HystrixConcurrencyStrategy) this method doesn't check isCurrentThreadInitialized, but checked in other high version
like hystrix high version, Return null if the {@link HystrixRequestContext} has not been initialized for the current thread.
New feature or improvement