Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Grails application error #2815

Merged
merged 2 commits into from
May 12, 2021
Merged

Fix Grails application error #2815

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09ab78c
Fix Grails application error
trask Apr 19, 2021
f97d46d
Merge remote-tracking branch 'upstream/main' into grails-problem
trask May 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 35 additions & 2 deletions ...pentelemetry/javaagent/instrumentation/springwebmvc/HandlerMappingResourceNameFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<HandlerMapping> handlerMappings;
Expand Down Expand Up @@ -74,8 +76,39 @@ private boolean findMapping(HttpServletRequest request) throws Exception {
return false;
}

public void setHandlerMappings(List<HandlerMapping> handlerMappings) {
this.handlerMappings = handlerMappings;
public void setHandlerMappings(List<HandlerMapping> mappings) {
List<HandlerMapping> 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
Comment on lines +87 to +90
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤯

Ah, side effects ❤️

//
// 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
Expand Down