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 12:58:04 UTC
[GitHub] [skywalking-java] furaul commented on a change in pull request #34: Fix spring mvc plugin
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