From 8854921f9e23650e4dcc185eefe16da6136bf6f6 Mon Sep 17 00:00:00 2001 From: YunaiV <> Date: Thu, 11 Jan 2018 09:48:42 +0800 Subject: [PATCH 1/3] fixes #735 Concurrency conflicts in Spring plugin --- .../mvc/commons/EnhanceRequireObjectCache.java | 18 ++++++++++-------- .../InvokeHandlerMethodInterceptor.java | 9 ++++++--- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/EnhanceRequireObjectCache.java b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/EnhanceRequireObjectCache.java index 914882eed..a17d8eea0 100644 --- a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/EnhanceRequireObjectCache.java +++ b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/EnhanceRequireObjectCache.java @@ -18,25 +18,26 @@ package org.apache.skywalking.apm.plugin.spring.mvc.commons; -import java.lang.reflect.Method; -import javax.servlet.http.HttpServletResponse; import org.springframework.web.context.request.NativeWebRequest; +import javax.servlet.http.HttpServletResponse; +import java.lang.reflect.Method; + public class EnhanceRequireObjectCache { private PathMappingCache pathMappingCache; - private NativeWebRequest nativeWebRequest; - private HttpServletResponse httpResponse; + private ThreadLocal nativeWebRequest = new ThreadLocal(); + private ThreadLocal httpResponse = new ThreadLocal(); public void setPathMappingCache(PathMappingCache pathMappingCache) { this.pathMappingCache = pathMappingCache; } public HttpServletResponse getHttpServletResponse() { - return httpResponse == null ? (HttpServletResponse)nativeWebRequest.getNativeResponse() : httpResponse; + return httpResponse.get() == null ? (HttpServletResponse) nativeWebRequest.get().getNativeResponse() : httpResponse.get(); } public void setNativeWebRequest(NativeWebRequest nativeWebRequest) { - this.nativeWebRequest = nativeWebRequest; + this.nativeWebRequest.set(nativeWebRequest); } public String findPathMapping(Method method) { @@ -52,10 +53,11 @@ public class EnhanceRequireObjectCache { } public void setHttpResponse(HttpServletResponse httpResponse) { - this.httpResponse = httpResponse; + this.httpResponse.set(httpResponse); } public HttpServletResponse getHttpResponse() { - return httpResponse; + return httpResponse.get(); } + } diff --git a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/InvokeHandlerMethodInterceptor.java b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/InvokeHandlerMethodInterceptor.java index 7a502d21f..061d505d4 100644 --- a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/InvokeHandlerMethodInterceptor.java +++ b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/InvokeHandlerMethodInterceptor.java @@ -18,13 +18,14 @@ package org.apache.skywalking.apm.plugin.spring.mvc.commons.interceptor; -import java.lang.reflect.Method; -import javax.servlet.http.HttpServletResponse; import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance; import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor; import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult; import org.apache.skywalking.apm.plugin.spring.mvc.commons.EnhanceRequireObjectCache; +import javax.servlet.http.HttpServletResponse; +import java.lang.reflect.Method; + public class InvokeHandlerMethodInterceptor implements InstanceMethodsAroundInterceptor { @Override public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class[] argumentsTypes, @@ -37,11 +38,13 @@ public class InvokeHandlerMethodInterceptor implements InstanceMethodsAroundInte @Override public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class[] argumentsTypes, Object ret) throws Throwable { + if (allArguments[2] instanceof EnhancedInstance) { + ((EnhanceRequireObjectCache)((EnhancedInstance)allArguments[2]).getSkyWalkingDynamicField()).setHttpResponse(null); + } return ret; } @Override public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, Class[] argumentsTypes, Throwable t) { - } } From 647f4f00a7405be79d5a9594b4ae4a03c1a4a862 Mon Sep 17 00:00:00 2001 From: YunaiV Date: Thu, 11 Jan 2018 16:22:37 +0800 Subject: [PATCH 2/3] =?UTF-8?q?fixes=20#735=20Concurrency=20conflicts=20in?= =?UTF-8?q?=20Spring=20plugin=20=EF=BC=9Aclear=20ThreadLocal?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../mvc/commons/EnhanceRequireObjectCache.java | 4 ---- .../interceptor/AbstractMethodInteceptor.java | 17 ++++++++++------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/EnhanceRequireObjectCache.java b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/EnhanceRequireObjectCache.java index a17d8eea0..7623582f2 100644 --- a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/EnhanceRequireObjectCache.java +++ b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/EnhanceRequireObjectCache.java @@ -56,8 +56,4 @@ public class EnhanceRequireObjectCache { this.httpResponse.set(httpResponse); } - public HttpServletResponse getHttpResponse() { - return httpResponse.get(); - } - } diff --git a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/AbstractMethodInteceptor.java b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/AbstractMethodInteceptor.java index ca7d97597..8941cd10b 100644 --- a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/AbstractMethodInteceptor.java +++ b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/AbstractMethodInteceptor.java @@ -19,23 +19,24 @@ package org.apache.skywalking.apm.plugin.spring.mvc.commons.interceptor; -import java.lang.reflect.Method; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; +import org.apache.skywalking.apm.agent.core.context.CarrierItem; import org.apache.skywalking.apm.agent.core.context.ContextCarrier; +import org.apache.skywalking.apm.agent.core.context.ContextManager; import org.apache.skywalking.apm.agent.core.context.tag.Tags; import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer; import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance; -import org.apache.skywalking.apm.network.trace.component.ComponentsDefine; -import org.apache.skywalking.apm.plugin.spring.mvc.commons.EnhanceRequireObjectCache; -import org.apache.skywalking.apm.agent.core.context.CarrierItem; -import org.apache.skywalking.apm.agent.core.context.ContextManager; import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor; import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult; +import org.apache.skywalking.apm.network.trace.component.ComponentsDefine; +import org.apache.skywalking.apm.plugin.spring.mvc.commons.EnhanceRequireObjectCache; import org.springframework.web.context.request.RequestContextHolder; import org.springframework.web.context.request.ServletRequestAttributes; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import java.lang.reflect.Method; + /** * the abstract method inteceptor */ @@ -80,6 +81,8 @@ public abstract class AbstractMethodInteceptor implements InstanceMethodsAroundI Tags.STATUS_CODE.set(span, Integer.toString(response.getStatus())); } ContextManager.stopSpan(); + + ((EnhanceRequireObjectCache)objInst.getSkyWalkingDynamicField()).setNativeWebRequest(null); return ret; } From 44a5526c78770578bc9ced9911ba32c95e923f70 Mon Sep 17 00:00:00 2001 From: YunaiV Date: Thu, 11 Jan 2018 20:51:19 +0800 Subject: [PATCH 3/3] fixes #735 Concurrency conflicts in Spring plugin * EnhanceRequireObjectCache add clearRequestAndResponse method * invoke EnhanceRequireObjectCache clearRequestAndResponse At AbstractMethodInterceptor --- .../commons/EnhanceRequireObjectCache.java | 5 +++++ ...or.java => AbstractMethodInterceptor.java} | 22 ++++++++++--------- .../InvokeHandlerMethodInterceptor.java | 3 --- .../RequestMappingMethodInterceptor.java | 5 +++-- .../RestMappingMethodInterceptor.java | 9 +++----- 5 files changed, 23 insertions(+), 21 deletions(-) rename apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/{AbstractMethodInteceptor.java => AbstractMethodInterceptor.java} (85%) diff --git a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/EnhanceRequireObjectCache.java b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/EnhanceRequireObjectCache.java index 7623582f2..cbadbfdab 100644 --- a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/EnhanceRequireObjectCache.java +++ b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/EnhanceRequireObjectCache.java @@ -56,4 +56,9 @@ public class EnhanceRequireObjectCache { this.httpResponse.set(httpResponse); } + public void clearRequestAndResponse() { + setNativeWebRequest(null); + setHttpResponse(null); + } + } diff --git a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/AbstractMethodInteceptor.java b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/AbstractMethodInterceptor.java similarity index 85% rename from apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/AbstractMethodInteceptor.java rename to apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/AbstractMethodInterceptor.java index 8941cd10b..798a2216e 100644 --- a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/AbstractMethodInteceptor.java +++ b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/AbstractMethodInterceptor.java @@ -40,7 +40,7 @@ import java.lang.reflect.Method; /** * the abstract method inteceptor */ -public abstract class AbstractMethodInteceptor implements InstanceMethodsAroundInterceptor { +public abstract class AbstractMethodInterceptor implements InstanceMethodsAroundInterceptor { public abstract String getRequestURL(Method method); @Override @@ -73,17 +73,19 @@ public abstract class AbstractMethodInteceptor implements InstanceMethodsAroundI @Override public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class[] argumentsTypes, Object ret) throws Throwable { - HttpServletResponse response = ((EnhanceRequireObjectCache)objInst.getSkyWalkingDynamicField()).getHttpServletResponse(); + try { + HttpServletResponse response = ((EnhanceRequireObjectCache) objInst.getSkyWalkingDynamicField()).getHttpServletResponse(); - AbstractSpan span = ContextManager.activeSpan(); - if (response.getStatus() >= 400) { - span.errorOccurred(); - Tags.STATUS_CODE.set(span, Integer.toString(response.getStatus())); + AbstractSpan span = ContextManager.activeSpan(); + if (response.getStatus() >= 400) { + span.errorOccurred(); + Tags.STATUS_CODE.set(span, Integer.toString(response.getStatus())); + } + ContextManager.stopSpan(); + return ret; + } finally { + ((EnhanceRequireObjectCache)objInst.getSkyWalkingDynamicField()).clearRequestAndResponse(); } - ContextManager.stopSpan(); - - ((EnhanceRequireObjectCache)objInst.getSkyWalkingDynamicField()).setNativeWebRequest(null); - return ret; } @Override diff --git a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/InvokeHandlerMethodInterceptor.java b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/InvokeHandlerMethodInterceptor.java index 061d505d4..c2d555608 100644 --- a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/InvokeHandlerMethodInterceptor.java +++ b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/InvokeHandlerMethodInterceptor.java @@ -38,9 +38,6 @@ public class InvokeHandlerMethodInterceptor implements InstanceMethodsAroundInte @Override public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class[] argumentsTypes, Object ret) throws Throwable { - if (allArguments[2] instanceof EnhancedInstance) { - ((EnhanceRequireObjectCache)((EnhancedInstance)allArguments[2]).getSkyWalkingDynamicField()).setHttpResponse(null); - } return ret; } diff --git a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/RequestMappingMethodInterceptor.java b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/RequestMappingMethodInterceptor.java index 58c4bcf05..1dd6b7748 100644 --- a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/RequestMappingMethodInterceptor.java +++ b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/RequestMappingMethodInterceptor.java @@ -19,16 +19,17 @@ package org.apache.skywalking.apm.plugin.spring.mvc.commons.interceptor; -import java.lang.reflect.Method; import org.springframework.web.bind.annotation.RequestMapping; +import java.lang.reflect.Method; + /** * The RequestMappingMethodInterceptor only use the first mapping value. * it will inteceptor with @RequestMapping * * @author clevertension */ -public class RequestMappingMethodInterceptor extends AbstractMethodInteceptor { +public class RequestMappingMethodInterceptor extends AbstractMethodInterceptor { @Override public String getRequestURL(Method method) { String requestURL = ""; diff --git a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/RestMappingMethodInterceptor.java b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/RestMappingMethodInterceptor.java index faee5b4cb..5cea58d0d 100644 --- a/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/RestMappingMethodInterceptor.java +++ b/apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/RestMappingMethodInterceptor.java @@ -19,12 +19,9 @@ package org.apache.skywalking.apm.plugin.spring.mvc.commons.interceptor; +import org.springframework.web.bind.annotation.*; + import java.lang.reflect.Method; -import org.springframework.web.bind.annotation.DeleteMapping; -import org.springframework.web.bind.annotation.GetMapping; -import org.springframework.web.bind.annotation.PatchMapping; -import org.springframework.web.bind.annotation.PostMapping; -import org.springframework.web.bind.annotation.PutMapping; /** * The RestMappingMethodInterceptor only use the first mapping value. @@ -34,7 +31,7 @@ import org.springframework.web.bind.annotation.PutMapping; * * @author clevertension */ -public class RestMappingMethodInterceptor extends AbstractMethodInteceptor { +public class RestMappingMethodInterceptor extends AbstractMethodInterceptor { @Override public String getRequestURL(Method method) { String requestURL = "";