You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2021/09/22 11:29:57 UTC
[GitHub] [skywalking-java] furaul opened a new pull request #34: Fix spring mvc plugin
furaul opened a new pull request #34:
URL: https://github.com/apache/skywalking-java/pull/34
Once the springMVC span is created in beforeMethod, it must be stopped in afterMethod. Or it may cause memory leak.
- [x] Add a unit test to verify that the fix works.
- [x] Explain briefly why the bug exists and how to fix it.
- When Exception throws in afterMethod of AbstractMethodInterceptor, the span will not be stopped, and it is not excepted.
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #7734 .
- This PR will not fix why the exception is throwed, but it will eliminate effects that the exception takes.
- The new plugin has been used in our company, on product enviroment.
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking-java/blob/main/CHANGES.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] furaul commented on a change in pull request #34: Fix spring mvc plugin
Posted by GitBox <gi...@apache.org>.
furaul commented on a change in pull request #34:
URL: https://github.com/apache/skywalking-java/pull/34#discussion_r713914390
##########
File path: apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/AbstractMethodInterceptor.java
##########
@@ -198,56 +198,58 @@ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allA
Object request = runtimeContext.get(REQUEST_KEY_IN_RUNTIME_CONTEXT);
if (request != null) {
- StackDepth stackDepth = (StackDepth) runtimeContext.get(CONTROLLER_METHOD_STACK_DEPTH);
- if (stackDepth == null) {
- throw new IllegalMethodStackDepthException();
- } else {
- stackDepth.decrement();
- }
-
- AbstractSpan span = ContextManager.activeSpan();
-
- if (stackDepth.depth() == 0) {
- Object response = runtimeContext.get(RESPONSE_KEY_IN_RUNTIME_CONTEXT);
- if (response == null) {
- throw new ServletResponseNotFoundException();
+ try {
+ StackDepth stackDepth = (StackDepth) runtimeContext.get(CONTROLLER_METHOD_STACK_DEPTH);
+ if (stackDepth == null) {
+ throw new IllegalMethodStackDepthException();
+ } else {
+ stackDepth.decrement();
}
- Integer statusCode = null;
+ AbstractSpan span = ContextManager.activeSpan();
- if (IS_SERVLET_GET_STATUS_METHOD_EXIST && HttpServletResponse.class.isAssignableFrom(response.getClass())) {
- statusCode = ((HttpServletResponse) response).getStatus();
- } else if (ServerHttpResponse.class.isAssignableFrom(response.getClass())) {
- if (IS_SERVLET_GET_STATUS_METHOD_EXIST) {
- statusCode = ((ServerHttpResponse) response).getRawStatusCode();
+ if (stackDepth.depth() == 0) {
+ Object response = runtimeContext.get(RESPONSE_KEY_IN_RUNTIME_CONTEXT);
+ if (response == null) {
+ throw new ServletResponseNotFoundException();
}
- Object context = runtimeContext.get(REACTIVE_ASYNC_SPAN_IN_RUNTIME_CONTEXT);
- if (context != null) {
- ((AbstractSpan[]) context)[0] = span.prepareForAsync();
+
+ Integer statusCode = null;
+
+ if (IS_SERVLET_GET_STATUS_METHOD_EXIST && HttpServletResponse.class.isAssignableFrom(response.getClass())) {
+ statusCode = ((HttpServletResponse) response).getStatus();
+ } else if (ServerHttpResponse.class.isAssignableFrom(response.getClass())) {
+ if (IS_SERVLET_GET_STATUS_METHOD_EXIST) {
+ statusCode = ((ServerHttpResponse) response).getRawStatusCode();
+ }
+ Object context = runtimeContext.get(REACTIVE_ASYNC_SPAN_IN_RUNTIME_CONTEXT);
+ if (context != null) {
+ ((AbstractSpan[]) context)[0] = span.prepareForAsync();
+ }
}
- }
- if (statusCode != null && statusCode >= 400) {
- span.errorOccurred();
- Tags.HTTP_RESPONSE_STATUS_CODE.set(span, statusCode);
- }
+ if (statusCode != null && statusCode >= 400) {
+ span.errorOccurred();
+ Tags.HTTP_RESPONSE_STATUS_CODE.set(span, statusCode);
+ }
- runtimeContext.remove(REACTIVE_ASYNC_SPAN_IN_RUNTIME_CONTEXT);
- runtimeContext.remove(REQUEST_KEY_IN_RUNTIME_CONTEXT);
- runtimeContext.remove(RESPONSE_KEY_IN_RUNTIME_CONTEXT);
- runtimeContext.remove(CONTROLLER_METHOD_STACK_DEPTH);
- }
+ runtimeContext.remove(REACTIVE_ASYNC_SPAN_IN_RUNTIME_CONTEXT);
+ runtimeContext.remove(REQUEST_KEY_IN_RUNTIME_CONTEXT);
+ runtimeContext.remove(RESPONSE_KEY_IN_RUNTIME_CONTEXT);
+ runtimeContext.remove(CONTROLLER_METHOD_STACK_DEPTH);
+ }
- // Active HTTP parameter collection automatically in the profiling context.
- if (!SpringMVCPluginConfig.Plugin.SpringMVC.COLLECT_HTTP_PARAMS && span.isProfiling()) {
- if (HttpServletRequest.class.isAssignableFrom(request.getClass())) {
- RequestUtil.collectHttpParam((HttpServletRequest) request, span);
- } else if (ServerHttpRequest.class.isAssignableFrom(request.getClass())) {
- RequestUtil.collectHttpParam((ServerHttpRequest) request, span);
+ // Active HTTP parameter collection automatically in the profiling context.
+ if (!SpringMVCPluginConfig.Plugin.SpringMVC.COLLECT_HTTP_PARAMS && span.isProfiling()) {
+ if (HttpServletRequest.class.isAssignableFrom(request.getClass())) {
+ RequestUtil.collectHttpParam((HttpServletRequest) request, span);
+ } else if (ServerHttpRequest.class.isAssignableFrom(request.getClass())) {
+ RequestUtil.collectHttpParam((ServerHttpRequest) request, span);
+ }
}
+ } finally {
+ ContextManager.stopSpan();
Review comment:
> Are you only adding this `try/finally` to stop span? Could you share when this last step goes missed?
Yes, I only did this.
The last step goes missed when the response == null and the exception ServletResponseNotFoundException throws.
It is complicated to reproduced why the response becomes null, since it is only appear in a few requests. And if I debug into the request, drop the frame, the response becomes not null.
Maybe I could read more springMVC source codes later and find out the root cause.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng merged pull request #34: Fix a tracing context leak in SpringMVC plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #34:
URL: https://github.com/apache/skywalking-java/pull/34
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a change in pull request #34: Fix spring mvc plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #34:
URL: https://github.com/apache/skywalking-java/pull/34#discussion_r713904599
##########
File path: apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/AbstractMethodInterceptor.java
##########
@@ -198,56 +198,58 @@ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allA
Object request = runtimeContext.get(REQUEST_KEY_IN_RUNTIME_CONTEXT);
if (request != null) {
- StackDepth stackDepth = (StackDepth) runtimeContext.get(CONTROLLER_METHOD_STACK_DEPTH);
- if (stackDepth == null) {
- throw new IllegalMethodStackDepthException();
- } else {
- stackDepth.decrement();
- }
-
- AbstractSpan span = ContextManager.activeSpan();
-
- if (stackDepth.depth() == 0) {
- Object response = runtimeContext.get(RESPONSE_KEY_IN_RUNTIME_CONTEXT);
- if (response == null) {
- throw new ServletResponseNotFoundException();
+ try {
+ StackDepth stackDepth = (StackDepth) runtimeContext.get(CONTROLLER_METHOD_STACK_DEPTH);
+ if (stackDepth == null) {
+ throw new IllegalMethodStackDepthException();
+ } else {
+ stackDepth.decrement();
}
- Integer statusCode = null;
+ AbstractSpan span = ContextManager.activeSpan();
- if (IS_SERVLET_GET_STATUS_METHOD_EXIST && HttpServletResponse.class.isAssignableFrom(response.getClass())) {
- statusCode = ((HttpServletResponse) response).getStatus();
- } else if (ServerHttpResponse.class.isAssignableFrom(response.getClass())) {
- if (IS_SERVLET_GET_STATUS_METHOD_EXIST) {
- statusCode = ((ServerHttpResponse) response).getRawStatusCode();
+ if (stackDepth.depth() == 0) {
+ Object response = runtimeContext.get(RESPONSE_KEY_IN_RUNTIME_CONTEXT);
+ if (response == null) {
+ throw new ServletResponseNotFoundException();
}
- Object context = runtimeContext.get(REACTIVE_ASYNC_SPAN_IN_RUNTIME_CONTEXT);
- if (context != null) {
- ((AbstractSpan[]) context)[0] = span.prepareForAsync();
+
+ Integer statusCode = null;
+
+ if (IS_SERVLET_GET_STATUS_METHOD_EXIST && HttpServletResponse.class.isAssignableFrom(response.getClass())) {
+ statusCode = ((HttpServletResponse) response).getStatus();
+ } else if (ServerHttpResponse.class.isAssignableFrom(response.getClass())) {
+ if (IS_SERVLET_GET_STATUS_METHOD_EXIST) {
+ statusCode = ((ServerHttpResponse) response).getRawStatusCode();
+ }
+ Object context = runtimeContext.get(REACTIVE_ASYNC_SPAN_IN_RUNTIME_CONTEXT);
+ if (context != null) {
+ ((AbstractSpan[]) context)[0] = span.prepareForAsync();
+ }
}
- }
- if (statusCode != null && statusCode >= 400) {
- span.errorOccurred();
- Tags.HTTP_RESPONSE_STATUS_CODE.set(span, statusCode);
- }
+ if (statusCode != null && statusCode >= 400) {
+ span.errorOccurred();
+ Tags.HTTP_RESPONSE_STATUS_CODE.set(span, statusCode);
+ }
- runtimeContext.remove(REACTIVE_ASYNC_SPAN_IN_RUNTIME_CONTEXT);
- runtimeContext.remove(REQUEST_KEY_IN_RUNTIME_CONTEXT);
- runtimeContext.remove(RESPONSE_KEY_IN_RUNTIME_CONTEXT);
- runtimeContext.remove(CONTROLLER_METHOD_STACK_DEPTH);
- }
+ runtimeContext.remove(REACTIVE_ASYNC_SPAN_IN_RUNTIME_CONTEXT);
+ runtimeContext.remove(REQUEST_KEY_IN_RUNTIME_CONTEXT);
+ runtimeContext.remove(RESPONSE_KEY_IN_RUNTIME_CONTEXT);
+ runtimeContext.remove(CONTROLLER_METHOD_STACK_DEPTH);
+ }
- // Active HTTP parameter collection automatically in the profiling context.
- if (!SpringMVCPluginConfig.Plugin.SpringMVC.COLLECT_HTTP_PARAMS && span.isProfiling()) {
- if (HttpServletRequest.class.isAssignableFrom(request.getClass())) {
- RequestUtil.collectHttpParam((HttpServletRequest) request, span);
- } else if (ServerHttpRequest.class.isAssignableFrom(request.getClass())) {
- RequestUtil.collectHttpParam((ServerHttpRequest) request, span);
+ // Active HTTP parameter collection automatically in the profiling context.
+ if (!SpringMVCPluginConfig.Plugin.SpringMVC.COLLECT_HTTP_PARAMS && span.isProfiling()) {
+ if (HttpServletRequest.class.isAssignableFrom(request.getClass())) {
+ RequestUtil.collectHttpParam((HttpServletRequest) request, span);
+ } else if (ServerHttpRequest.class.isAssignableFrom(request.getClass())) {
+ RequestUtil.collectHttpParam((ServerHttpRequest) request, span);
+ }
}
+ } finally {
+ ContextManager.stopSpan();
Review comment:
Are you only adding this `try/finally` to stop span? Could you share when this last step goes missed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng removed a comment on pull request #34: Fix spring mvc plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng removed a comment on pull request #34:
URL: https://github.com/apache/skywalking-java/pull/34#issuecomment-924891557
I would recommend you to upgrade this interceptor to v2 API. Take a look at this, https://skywalking.apache.org/docs/skywalking-java/latest/en/setup/service-agent/java-agent/java-plugin-development-guide/#v2-apis-1
Once span created, you could use `MethodInvocationContext` to propagate for the `afterMethod`. Then you don't need to check depth anymore, which is actually not being used by any plugin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #34: Fix spring mvc plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #34:
URL: https://github.com/apache/skywalking-java/pull/34#issuecomment-924891557
I would recommend you to upgrade this interceptor to v2 API. Take a look at this, https://skywalking.apache.org/docs/skywalking-java/latest/en/setup/service-agent/java-agent/java-plugin-development-guide/#v2-apis-1
Once span created, you could use `MethodInvocationContext` to propagate for the `afterMethod`. Then you don't need to check depth anymore, which is actually not being used by any plugin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a change in pull request #34: Fix spring mvc plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #34:
URL: https://github.com/apache/skywalking-java/pull/34#discussion_r713922398
##########
File path: apm-sniffer/apm-sdk-plugin/spring-plugins/mvc-annotation-commons/src/main/java/org/apache/skywalking/apm/plugin/spring/mvc/commons/interceptor/AbstractMethodInterceptor.java
##########
@@ -198,56 +198,58 @@ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allA
Object request = runtimeContext.get(REQUEST_KEY_IN_RUNTIME_CONTEXT);
if (request != null) {
- StackDepth stackDepth = (StackDepth) runtimeContext.get(CONTROLLER_METHOD_STACK_DEPTH);
- if (stackDepth == null) {
- throw new IllegalMethodStackDepthException();
- } else {
- stackDepth.decrement();
- }
-
- AbstractSpan span = ContextManager.activeSpan();
-
- if (stackDepth.depth() == 0) {
- Object response = runtimeContext.get(RESPONSE_KEY_IN_RUNTIME_CONTEXT);
- if (response == null) {
- throw new ServletResponseNotFoundException();
+ try {
+ StackDepth stackDepth = (StackDepth) runtimeContext.get(CONTROLLER_METHOD_STACK_DEPTH);
+ if (stackDepth == null) {
+ throw new IllegalMethodStackDepthException();
+ } else {
+ stackDepth.decrement();
}
- Integer statusCode = null;
+ AbstractSpan span = ContextManager.activeSpan();
- if (IS_SERVLET_GET_STATUS_METHOD_EXIST && HttpServletResponse.class.isAssignableFrom(response.getClass())) {
- statusCode = ((HttpServletResponse) response).getStatus();
- } else if (ServerHttpResponse.class.isAssignableFrom(response.getClass())) {
- if (IS_SERVLET_GET_STATUS_METHOD_EXIST) {
- statusCode = ((ServerHttpResponse) response).getRawStatusCode();
+ if (stackDepth.depth() == 0) {
+ Object response = runtimeContext.get(RESPONSE_KEY_IN_RUNTIME_CONTEXT);
+ if (response == null) {
+ throw new ServletResponseNotFoundException();
}
- Object context = runtimeContext.get(REACTIVE_ASYNC_SPAN_IN_RUNTIME_CONTEXT);
- if (context != null) {
- ((AbstractSpan[]) context)[0] = span.prepareForAsync();
+
+ Integer statusCode = null;
+
+ if (IS_SERVLET_GET_STATUS_METHOD_EXIST && HttpServletResponse.class.isAssignableFrom(response.getClass())) {
+ statusCode = ((HttpServletResponse) response).getStatus();
+ } else if (ServerHttpResponse.class.isAssignableFrom(response.getClass())) {
+ if (IS_SERVLET_GET_STATUS_METHOD_EXIST) {
+ statusCode = ((ServerHttpResponse) response).getRawStatusCode();
+ }
+ Object context = runtimeContext.get(REACTIVE_ASYNC_SPAN_IN_RUNTIME_CONTEXT);
+ if (context != null) {
+ ((AbstractSpan[]) context)[0] = span.prepareForAsync();
+ }
}
- }
- if (statusCode != null && statusCode >= 400) {
- span.errorOccurred();
- Tags.HTTP_RESPONSE_STATUS_CODE.set(span, statusCode);
- }
+ if (statusCode != null && statusCode >= 400) {
+ span.errorOccurred();
+ Tags.HTTP_RESPONSE_STATUS_CODE.set(span, statusCode);
+ }
- runtimeContext.remove(REACTIVE_ASYNC_SPAN_IN_RUNTIME_CONTEXT);
- runtimeContext.remove(REQUEST_KEY_IN_RUNTIME_CONTEXT);
- runtimeContext.remove(RESPONSE_KEY_IN_RUNTIME_CONTEXT);
- runtimeContext.remove(CONTROLLER_METHOD_STACK_DEPTH);
- }
+ runtimeContext.remove(REACTIVE_ASYNC_SPAN_IN_RUNTIME_CONTEXT);
+ runtimeContext.remove(REQUEST_KEY_IN_RUNTIME_CONTEXT);
+ runtimeContext.remove(RESPONSE_KEY_IN_RUNTIME_CONTEXT);
+ runtimeContext.remove(CONTROLLER_METHOD_STACK_DEPTH);
+ }
- // Active HTTP parameter collection automatically in the profiling context.
- if (!SpringMVCPluginConfig.Plugin.SpringMVC.COLLECT_HTTP_PARAMS && span.isProfiling()) {
- if (HttpServletRequest.class.isAssignableFrom(request.getClass())) {
- RequestUtil.collectHttpParam((HttpServletRequest) request, span);
- } else if (ServerHttpRequest.class.isAssignableFrom(request.getClass())) {
- RequestUtil.collectHttpParam((ServerHttpRequest) request, span);
+ // Active HTTP parameter collection automatically in the profiling context.
+ if (!SpringMVCPluginConfig.Plugin.SpringMVC.COLLECT_HTTP_PARAMS && span.isProfiling()) {
+ if (HttpServletRequest.class.isAssignableFrom(request.getClass())) {
+ RequestUtil.collectHttpParam((HttpServletRequest) request, span);
+ } else if (ServerHttpRequest.class.isAssignableFrom(request.getClass())) {
+ RequestUtil.collectHttpParam((ServerHttpRequest) request, span);
+ }
}
+ } finally {
+ ContextManager.stopSpan();
Review comment:
OK, to add this to keep tracing context safe is correct. I just think, this may not fix the real issue about why response is null, and we can't get all response related information.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org