diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerMappingResourceNameFilter.java b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerMappingResourceNameFilter.java index 4376f52dd1b6..1ef0981ce1ba 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerMappingResourceNameFilter.java +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerMappingResourceNameFilter.java @@ -9,6 +9,7 @@ import io.opentelemetry.context.Context; import java.io.IOException; +import java.util.ArrayList; import java.util.List; import javax.servlet.Filter; import javax.servlet.FilterChain; @@ -22,6 +23,7 @@ import org.springframework.core.Ordered; import org.springframework.web.servlet.HandlerExecutionChain; import org.springframework.web.servlet.HandlerMapping; +import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping; public class HandlerMappingResourceNameFilter implements Filter, Ordered { private volatile List handlerMappings; @@ -74,8 +76,39 @@ private boolean findMapping(HttpServletRequest request) throws Exception { return false; } - public void setHandlerMappings(List handlerMappings) { - this.handlerMappings = handlerMappings; + public void setHandlerMappings(List mappings) { + List handlerMappings = new ArrayList<>(); + for (HandlerMapping mapping : mappings) { + // it may be enticing to add all HandlerMapping classes here, but DO NOT + // + // because we call getHandler() on them above, at the very beginning of the request + // and this can be a very invasive call with application-crashing side-effects + // + // for example: org.grails.web.mapping.mvc.UrlMappingsHandlerMapping.getHandler() + // 1. uses GrailsWebRequest.lookup() to get GrailsWebRequest bound to thread local + // 2. and populates the servlet request attribute "org.grails.url.match.info" + // with GrailsControllerUrlMappingInfo + // + // which causes big problems if GrailsWebRequest thread local is leaked from prior request + // (which has been observed to happen in Grails 3.0.17 at least), because then our call to + // UrlMappingsHandlerMapping.getHandler() at the very beginning of the request: + // 1. GrailsWebRequest.lookup() gets the leaked GrailsWebRequest + // 2. servlet request attribute "org.grails.url.match.info" is populated based on this leaked + // GrailsWebRequest (so in other words, most likely the wrong route is matched) + // + // and then GrailsWebRequestFilter creates a new GrailsWebRequest and binds it to the thread + // + // and then the application calls UrlMappingsHandlerMapping.getHandler() to route the request + // but it finds servlet request attribute "org.grails.url.match.info" already populated (by + // above) and so it short cuts the matching process and uses the wrong route that the agent + // populated caused to be populated into the request attribute above + if (mapping instanceof RequestMappingHandlerMapping) { + handlerMappings.add(mapping); + } + } + if (!handlerMappings.isEmpty()) { + this.handlerMappings = handlerMappings; + } } @Override