Uploaded image for project: 'Spring Framework'
  1. Spring Framework
  2. SPR-9138

Race condition in AnnotationMethodHandlerExceptionResolver

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Complete
    • Affects Version/s: 3.1 GA, 3.1.1
    • Fix Version/s: 3.2 M1
    • Component/s: Web
    • Labels:
    • Last commented by a User:
      false

      Description

      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.

        Attachments

          Activity

            People

            • Assignee:
              rstoya05-aop Rossen Stoyanchev
              Reporter:
              magott Morten Andersen-Gott
              Last updater:
              Trevor Marshall
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Days since last comment:
                6 years, 12 weeks, 3 days ago