Race condition in AnnotationMethodHandlerExceptionResolver [SPR-9138] #13777
Description
Morten Andersen-Gott opened SPR-9138 and commented
There seem to be a race condition in AnnotationMethodHandlerExceptionResolver. I've tried to reproduce in a unit test, but haven't succeeded. I regularly see this in the production logs though:
Invoking request method resulted in exception : public static native long java.lang.System.currentTimeMillis() [] at org.springframework.web.servlet.mvc.annotation.AnnotationMethodHandlerExceptionResolver.doResolveException(AnnotationMethodHandlerExceptionResolver.java:143)
java.lang.IllegalArgumentException: Invalid handler method return value: 1329504807993
System.currentTimeMillis() is a dummy placeholder in AnnotationMethodHandlerExceptionResolver that shouldn't be called if there isn't a race condition. This is what I believe happens. I've added comments to the findBestExceptionHandlerMethod-method from AnnotationMethodHandlerExceptionResolver which I believe causes the problem:
private Method findBestExceptionHandlerMethod(Object handler, final Exception thrownException) {
//T1 and T2 enters method
final Class<?> handlerType = ClassUtils.getUserClass(handler);
final Class<? extends Throwable> thrownExceptionType = thrownException.getClass();
Method handlerMethod = null;
Map<Class<? extends Throwable>, Method> handlers = exceptionHandlerCache.get(handlerType);
//Timing makes (handlers==null) for T1 (handler != null) for T2
if (handlers != null) {
handlerMethod = handlers.get(thrownExceptionType);
//handlerMethod is still null for T2
if (handlerMethod != null) {
return (handlerMethod == NO_METHOD_FOUND ? null : handlerMethod);
}
}
else {
//T1 does this, the hashmap created here is read by T2 in if-test above matching this else block
handlers = new ConcurrentHashMap<Class<? extends Throwable>, Method>();
exceptionHandlerCache.put(handlerType, handlers);
}
//handlers is empty for both T1 and T2, the two threads' resolverMethod variables will reference the same
// ConcurrentHashMap instance
final Map<Class<? extends Throwable>, Method> resolverMethods = handlers;
//This block does not find a match for the exception
ReflectionUtils.doWithMethods(handlerType, new ReflectionUtils.MethodCallback() {
public void doWith(Method method) {
method = ClassUtils.getMostSpecificMethod(method, handlerType);
List<Class<? extends Throwable>> handledExceptions = getHandledExceptions(method);
for (Class<? extends Throwable> handledException : handledExceptions) {
if (handledException.isAssignableFrom(thrownExceptionType)) {
if (!resolverMethods.containsKey(handledException)) {
resolverMethods.put(handledException, method);
} else {
Method oldMappedMethod = resolverMethods.get(handledException);
if (!oldMappedMethod.equals(method)) {
throw new IllegalStateException(
"Ambiguous exception handler mapped for " + handledException + "]: {" +
oldMappedMethod + ", " + method + "}.");
}
}
}
}
}
});
//T1 finds no match and puts NO_METHOD_FOUND in cache and returns null
// When T2 hits this line resolverMethods for T2 reference the same Map that T1 just put the NO_METHOD_FOUND
// as a result getBestMatchingMethod will return NO_SUCH_METHOD_FOUND (System.timeMillis()) which will be invoked
// by the doResolveException further up the callstack.
handlerMethod = getBestMatchingMethod(resolverMethods, thrownException);
handlers.put(thrownExceptionType, (handlerMethod == null ? NO_METHOD_FOUND : handlerMethod));
return handlerMethod;
}
Basically, I'd say the following
if (handlerMethod != null) {
return (handlerMethod == NO_METHOD_FOUND ? null : handlerMethod);
test at the beginning of findBestExceptionHandlerMethod does not sufficiently protect against race conditions.
Affects: 3.1 GA, 3.1.1
Sub-tasks:
- Backport "Race condition in AnnotationMethodHandlerExceptionResolver" [SPR-9147] #13785 Backport "Race condition in AnnotationMethodHandlerExceptionResolver"
0 votes, 7 watchers