From c862311811c750bd5375506c7187af3501400be9 Mon Sep 17 00:00:00 2001 From: vicasong Date: Wed, 19 Oct 2022 18:22:30 +0800 Subject: [PATCH 1/2] Release RequestAttributes when called asynchronously. Fixes gh-716 (#775) --- .../FeignCircuitBreakerInvocationHandler.java | 19 +++++++++++- .../AsyncCircuitBreakerTest.java | 30 +++++++++++++++++-- 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignCircuitBreakerInvocationHandler.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignCircuitBreakerInvocationHandler.java index a85cf471..77ba428e 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignCircuitBreakerInvocationHandler.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignCircuitBreakerInvocationHandler.java @@ -36,6 +36,14 @@ import org.springframework.web.context.request.RequestContextHolder; import static feign.Util.checkNotNull; +/** + * @author Grzejszczak + * @author Sharma + * @author Niang + * @author Bohutskyi + * @author kim + * @author Vicasong + */ class FeignCircuitBreakerInvocationHandler implements InvocationHandler { private final CircuitBreakerFactory factory; @@ -122,9 +130,13 @@ class FeignCircuitBreakerInvocationHandler implements InvocationHandler { private Supplier asSupplier(final Method method, final Object[] args) { final RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes(); + final Thread caller = Thread.currentThread(); return () -> { + boolean isAsync = caller != Thread.currentThread(); try { - RequestContextHolder.setRequestAttributes(requestAttributes); + if (isAsync) { + RequestContextHolder.setRequestAttributes(requestAttributes); + } return dispatch.get(method).invoke(args); } catch (RuntimeException throwable) { @@ -133,6 +145,11 @@ class FeignCircuitBreakerInvocationHandler implements InvocationHandler { catch (Throwable throwable) { throw new RuntimeException(throwable); } + finally { + if (isAsync) { + RequestContextHolder.resetRequestAttributes(); + } + } }; } diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/circuitbreaker/AsyncCircuitBreakerTest.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/circuitbreaker/AsyncCircuitBreakerTest.java index 863381a9..36980217 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/circuitbreaker/AsyncCircuitBreakerTest.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/circuitbreaker/AsyncCircuitBreakerTest.java @@ -19,6 +19,9 @@ package org.springframework.cloud.openfeign.circuitbreaker; import java.time.Duration; import java.util.Objects; import java.util.UUID; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.function.Function; import javax.servlet.http.HttpServletRequest; @@ -28,6 +31,7 @@ import org.junit.jupiter.api.Test; import org.springframework.beans.factory.ObjectProvider; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; import org.springframework.boot.test.context.SpringBootTest; @@ -41,6 +45,7 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; import org.springframework.http.HttpHeaders; +import org.springframework.scheduling.concurrent.CustomizableThreadFactory; import org.springframework.test.web.servlet.MockMvc; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; @@ -48,6 +53,7 @@ import org.springframework.web.bind.annotation.RestController; import org.springframework.web.context.request.RequestContextHolder; import org.springframework.web.context.request.ServletRequestAttributes; +import static org.assertj.core.api.Assertions.assertThat; import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment.RANDOM_PORT; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print; @@ -67,6 +73,10 @@ class AsyncCircuitBreakerTest { @Autowired MockMvc mvc; + @Autowired + @Qualifier("asyncWorker") + ExecutorService asyncCircuitBreakerExecutor; + @Test void shouldWorkNormally() throws Exception { mvc.perform(get("/hello/proxy")).andDo(print()).andExpect(status().isOk()) @@ -86,14 +96,30 @@ class AsyncCircuitBreakerTest { authorization)).andDo(print()).andExpect(status().isOk()).andExpect(content().string(authorization)); } + @Test + void shouldProxyHeaderWhenHeaderSetAndCleanRequestAttributesAfterReturn() throws Exception { + shouldNotProxyAnyHeadersWithoutHeaderSet(); + Future future = asyncCircuitBreakerExecutor.submit(() -> (ServletRequestAttributes) + RequestContextHolder.getRequestAttributes()); + assertThat(future.get()) + .as("the RequestAttributes has been cleared") + .isNull(); + } + @EnableAutoConfiguration @Configuration(proxyBeanMethods = false) @EnableFeignClients(clients = { TestClient.class }) @Import({ NoSecurityConfiguration.class, TestController.class }) static class Application { + @Bean(name = "asyncWorker", destroyMethod = "shutdown") + ExecutorService asyncCircuitBreakerExecutor() { + return Executors.newSingleThreadExecutor(new CustomizableThreadFactory("async")); + } + @Bean - CircuitBreakerFactory> circuitBreakerFactory() { + CircuitBreakerFactory> circuitBreakerFactory( + @Qualifier("asyncWorker") ExecutorService asyncCircuitBreakerExecutor) { return new CircuitBreakerFactory>() { Function defaultConfiguration = id -> Duration.ofMillis(1000); @@ -101,7 +127,7 @@ class AsyncCircuitBreakerTest { @Override public CircuitBreaker create(String id) { Duration timeout = super.getConfigurations().computeIfAbsent(id, defaultConfiguration); - return new AsyncCircuitBreaker(timeout); + return new AsyncCircuitBreaker(timeout, asyncCircuitBreakerExecutor); } @Override From 172fcbbedf671252ddeb80707f57ebfcecd6b827 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Wed, 19 Oct 2022 12:26:07 +0200 Subject: [PATCH 2/2] Fix test class name. --- ...akerTest.java => AsyncCircuitBreakerTests.java} | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) rename spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/circuitbreaker/{AsyncCircuitBreakerTest.java => AsyncCircuitBreakerTests.java} (94%) diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/circuitbreaker/AsyncCircuitBreakerTest.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/circuitbreaker/AsyncCircuitBreakerTests.java similarity index 94% rename from spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/circuitbreaker/AsyncCircuitBreakerTest.java rename to spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/circuitbreaker/AsyncCircuitBreakerTests.java index 36980217..3c7a7cb6 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/circuitbreaker/AsyncCircuitBreakerTest.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/circuitbreaker/AsyncCircuitBreakerTests.java @@ -65,10 +65,10 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers. * * @author John Niang */ -@SpringBootTest(classes = AsyncCircuitBreakerTest.Application.class, webEnvironment = RANDOM_PORT, +@SpringBootTest(classes = AsyncCircuitBreakerTests.Application.class, webEnvironment = RANDOM_PORT, properties = "feign.circuitbreaker.enabled=true") @AutoConfigureMockMvc -class AsyncCircuitBreakerTest { +class AsyncCircuitBreakerTests { @Autowired MockMvc mvc; @@ -99,11 +99,9 @@ class AsyncCircuitBreakerTest { @Test void shouldProxyHeaderWhenHeaderSetAndCleanRequestAttributesAfterReturn() throws Exception { shouldNotProxyAnyHeadersWithoutHeaderSet(); - Future future = asyncCircuitBreakerExecutor.submit(() -> (ServletRequestAttributes) - RequestContextHolder.getRequestAttributes()); - assertThat(future.get()) - .as("the RequestAttributes has been cleared") - .isNull(); + Future future = asyncCircuitBreakerExecutor + .submit(() -> (ServletRequestAttributes) RequestContextHolder.getRequestAttributes()); + assertThat(future.get()).as("the RequestAttributes has been cleared").isNull(); } @EnableAutoConfiguration @@ -119,7 +117,7 @@ class AsyncCircuitBreakerTest { @Bean CircuitBreakerFactory> circuitBreakerFactory( - @Qualifier("asyncWorker") ExecutorService asyncCircuitBreakerExecutor) { + @Qualifier("asyncWorker") ExecutorService asyncCircuitBreakerExecutor) { return new CircuitBreakerFactory>() { Function defaultConfiguration = id -> Duration.ofMillis(1000);