From 099e9e22d29d7ae81ce5e188dfd3fb8b67972180 Mon Sep 17 00:00:00 2001 From: Michael Axiak Date: Wed, 25 Mar 2015 10:49:04 -0400 Subject: [PATCH] Check if the request log is enabled before requiring the context is initialized. In https://github.com/Netflix/Hystrix/commit/706a0f267ed65a215de1d813598c6c72b00f76eb, the guard to initialize the request log based on the property was removed. This causes a dependency on hystrix context initialization for all custom concurrency strategies. Here we add that back in, along with a test to ensure that a custom strategy could still run without an initialized context. --- .../com/netflix/hystrix/AbstractCommand.java | 24 +++++++------ .../HystrixConcurrencyStrategyTest.java | 35 +++++++++++++++---- 2 files changed, 42 insertions(+), 17 deletions(-) diff --git a/hystrix-core/src/main/java/com/netflix/hystrix/AbstractCommand.java b/hystrix-core/src/main/java/com/netflix/hystrix/AbstractCommand.java index a1013ce81..d023e43eb 100644 --- a/hystrix-core/src/main/java/com/netflix/hystrix/AbstractCommand.java +++ b/hystrix-core/src/main/java/com/netflix/hystrix/AbstractCommand.java @@ -257,21 +257,25 @@ protected AbstractCommand(HystrixCommandGroupKey group, HystrixCommandKey key, H /* setup the request cache for this instance */ this.requestCache = HystrixRequestCache.getInstance(this.commandKey, this.concurrencyStrategy); - /* store reference to request log regardless of which thread later hits it */ - if (concurrencyStrategy instanceof HystrixConcurrencyStrategyDefault) { - // if we're using the default we support only optionally using a request context - if (HystrixRequestContext.isCurrentThreadInitialized()) { + if (properties.requestLogEnabled().get()) { + /* store reference to request log regardless of which thread later hits it */ + if (concurrencyStrategy instanceof HystrixConcurrencyStrategyDefault) { + // if we're using the default we support only optionally using a request context + if (HystrixRequestContext.isCurrentThreadInitialized()) { currentRequestLog = HystrixRequestLog.getCurrentRequest(concurrencyStrategy); - } else { + } else { currentRequestLog = null; - } - } else { - // if it's a custom strategy it must ensure the context is initialized - if (HystrixRequestLog.getCurrentRequest(concurrencyStrategy) != null) { - currentRequestLog = HystrixRequestLog.getCurrentRequest(concurrencyStrategy); + } } else { + // if it's a custom strategy it must ensure the context is initialized + if (HystrixRequestLog.getCurrentRequest(concurrencyStrategy) != null) { + currentRequestLog = HystrixRequestLog.getCurrentRequest(concurrencyStrategy); + } else { currentRequestLog = null; + } } + } else { + currentRequestLog = null; } } diff --git a/hystrix-core/src/test/java/com/netflix/hystrix/strategy/concurrency/HystrixConcurrencyStrategyTest.java b/hystrix-core/src/test/java/com/netflix/hystrix/strategy/concurrency/HystrixConcurrencyStrategyTest.java index 90deca570..2fd4d37f5 100644 --- a/hystrix-core/src/test/java/com/netflix/hystrix/strategy/concurrency/HystrixConcurrencyStrategyTest.java +++ b/hystrix-core/src/test/java/com/netflix/hystrix/strategy/concurrency/HystrixConcurrencyStrategyTest.java @@ -16,9 +16,13 @@ package com.netflix.hystrix.strategy.concurrency; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import java.lang.IllegalStateException; import java.util.concurrent.atomic.AtomicBoolean; +import com.netflix.hystrix.strategy.concurrency.HystrixConcurrencyStrategy; +import com.netflix.hystrix.strategy.concurrency.HystrixRequestContext; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -41,11 +45,7 @@ public void prepareForTest() { @After public void cleanup() { - // instead of storing the reference from initialize we'll just get the current state and shutdown - if (HystrixRequestContext.getContextForCurrentThread() != null) { - // it could have been set NULL by the test - HystrixRequestContext.getContextForCurrentThread().shutdown(); - } + shutdownContextIfExists(); // force properties to be clean as well ConfigurationManager.getConfigInstance().clear(); @@ -82,7 +82,9 @@ public SimpleCommand() { @Override protected String run() throws Exception { - System.out.println("Executing => Commands: " + HystrixRequestLog.getCurrentRequest().getAllExecutedCommands()); + if (HystrixRequestContext.isCurrentThreadInitialized()) { + System.out.println("Executing => Commands: " + HystrixRequestLog.getCurrentRequest().getAllExecutedCommands()); + } return "Hello"; } @@ -106,7 +108,26 @@ public void call(Throwable throwable) { assertEquals(true, isInitialized.get()); } - public static class TimeoutCommand extends HystrixCommand { + @Test + public void testNoRequestContextOnSimpleConcurencyStrategyWithoutException() throws Exception { + shutdownContextIfExists(); + ConfigurationManager.getConfigInstance().setProperty("hystrix.command.default.requestLog.enabled", "false"); + + new SimpleCommand().execute(); + + assertTrue("We are able to run the simple command without a context initialization error.", true); + } + + private void shutdownContextIfExists() { + // instead of storing the reference from initialize we'll just get the current state and shutdown + if (HystrixRequestContext.getContextForCurrentThread() != null) { + // it could have been set NULL by the test + HystrixRequestContext.getContextForCurrentThread().shutdown(); + } + } + private static class DummyHystrixConcurrencyStrategy extends HystrixConcurrencyStrategy {} + + public static class TimeoutCommand extends HystrixCommand { static final HystrixCommand.Setter properties = HystrixCommand.Setter .withGroupKey(HystrixCommandGroupKey.Factory.asKey("TimeoutTest")) .andCommandPropertiesDefaults(HystrixCommandProperties.Setter()