Spring Framework
  1. Spring Framework
  2. SPR-9138

Race condition in AnnotationMethodHandlerExceptionResolver

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major 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.

        Activity

        Hide
        Rossen Stoyanchev added a comment -

        It does look an issue.

        As far as I can see the problem isn't that the two concurrent methods end up working on the same "handlers" Map. They're supposed to actually given the same handlerType. Or alternatively if they ended up creating separate Map instances, one would end up replacing the other causing a little extra work (but still less than the side effects of extra synchronization) but eventually the cache will be populated and no real harm will be done.

        The part in the beginning where T2 finds no handlerMethod for its exception type is fine as well:

        if (handlerMethod != null) {
            return (handlerMethod == NO_METHOD_FOUND ? null : handlerMethod);
        

        The problem is in the assumption at the end that a non-null return value from getBestMatchingMethod means there is a match. We should basically repeat the check for handlerMethod == NO_METHOD_FOUND, essentially never assuming we're the only ones populating the Map.

        Something like:

        handlers.put(thrownExceptionType, ((handlerMethod == null) || (handlerMethod == NO_METHOD_FOUND)) ? NO_METHOD_FOUND : handlerMethod));
        
        Show
        Rossen Stoyanchev added a comment - It does look an issue. As far as I can see the problem isn't that the two concurrent methods end up working on the same "handlers" Map. They're supposed to actually given the same handlerType. Or alternatively if they ended up creating separate Map instances, one would end up replacing the other causing a little extra work (but still less than the side effects of extra synchronization) but eventually the cache will be populated and no real harm will be done. The part in the beginning where T2 finds no handlerMethod for its exception type is fine as well: if (handlerMethod != null ) { return (handlerMethod == NO_METHOD_FOUND ? null : handlerMethod); The problem is in the assumption at the end that a non-null return value from getBestMatchingMethod means there is a match. We should basically repeat the check for handlerMethod == NO_METHOD_FOUND, essentially never assuming we're the only ones populating the Map. Something like: handlers.put(thrownExceptionType, ((handlerMethod == null ) || (handlerMethod == NO_METHOD_FOUND)) ? NO_METHOD_FOUND : handlerMethod));
        Hide
        Morten Andersen-Gott added a comment -

        Yes, I was trying to demonstrate under what circumstance the two lines at the end would cause a problem. Your proposed change looks like it will solve it, I was thinking of something similar. Just to be on the safe side though, how about changing the NO_METHOD_FOUND constant that is using a arbitrary (and error prone) method to a another method for this specific use, one that returns null. That would mean that in the case of any other issues, it will be invoked, return null and the next error handler (if more than the annotation based is configured) will be invoked without any errors being thrown? You can just create a method in the same class and get a Method handle for it?

        Show
        Morten Andersen-Gott added a comment - Yes, I was trying to demonstrate under what circumstance the two lines at the end would cause a problem. Your proposed change looks like it will solve it, I was thinking of something similar. Just to be on the safe side though, how about changing the NO_METHOD_FOUND constant that is using a arbitrary (and error prone) method to a another method for this specific use, one that returns null. That would mean that in the case of any other issues, it will be invoked, return null and the next error handler (if more than the annotation based is configured) will be invoked without any errors being thrown? You can just create a method in the same class and get a Method handle for it?
        Hide
        Morten Andersen-Gott added a comment -

        Actually, not so sure that will fix it. You'll still put NO_METHOD_FOUND and in a race condition:
        T2 enters this line:

        handlerMethod = getBestMatchingMethod(resolverMethods, thrownException);

        resolverMethods has just been "enriched" with NO_METHOD_FOUND, which means handlerMethod is now == NO_METHOD_FOUND
        the next line makes no difference, T2 will override the map entry, but with the exact same value (NO_METHOD_FOUND)

        handlers.put(thrownExceptionType, ((handlerMethod == null) || (handlerMethod == NO_METHOD_FOUND)) ? NO_METHOD_FOUND : handlerMethod));

        then...

        return handlerMethod

        As I see it, it won't solve anything. What you could do, is replace the final return statement with:

        return handlerMethod==NO_METHOD_FOUND?null:handlerMethod

        That should improve things.

        Show
        Morten Andersen-Gott added a comment - Actually, not so sure that will fix it. You'll still put NO_METHOD_FOUND and in a race condition: T2 enters this line: handlerMethod = getBestMatchingMethod(resolverMethods, thrownException); resolverMethods has just been "enriched" with NO_METHOD_FOUND, which means handlerMethod is now == NO_METHOD_FOUND the next line makes no difference, T2 will override the map entry, but with the exact same value (NO_METHOD_FOUND) handlers.put(thrownExceptionType, ((handlerMethod == null ) || (handlerMethod == NO_METHOD_FOUND)) ? NO_METHOD_FOUND : handlerMethod)); then... return handlerMethod As I see it, it won't solve anything. What you could do, is replace the final return statement with: return handlerMethod==NO_METHOD_FOUND? null :handlerMethod That should improve things.
        Hide
        Rossen Stoyanchev added a comment -

        You're right. We need to ensure the method never returns NO_METHOD_FOUND and currently the last line could do that as you've demonstrated. Changing it to this will ensure that never happens:

        return (handlerMethod == NO_METHOD_FOUND ? null : handlerMethod)
        
        Show
        Rossen Stoyanchev added a comment - You're right. We need to ensure the method never returns NO_METHOD_FOUND and currently the last line could do that as you've demonstrated. Changing it to this will ensure that never happens: return (handlerMethod == NO_METHOD_FOUND ? null : handlerMethod)
        Hide
        Mike Hart added a comment -

        As anyone got a fix for this yet, above suggestion does not explain how to implement this fully. Im using the latest version 3.1.1 and occcurs in my logs occasionally however it is not causing any problems, what can I do to get rid of this entry from my error log. If it means going back to an earlier version of Spring then so be it. Please Help.

        Show
        Mike Hart added a comment - As anyone got a fix for this yet, above suggestion does not explain how to implement this fully. Im using the latest version 3.1.1 and occcurs in my logs occasionally however it is not causing any problems, what can I do to get rid of this entry from my error log. If it means going back to an earlier version of Spring then so be it. Please Help.
        Hide
        Morten Andersen-Gott added a comment -

        Mike, you can implement a new com.yourcompany.AnnotationMethodHandlerExceptionResolver based on the code from Spring's AnnotationMethodHandlerExceptionResolver (copy the source). In the findBestExceptionHandlerMethod-method change the last line of code to return (handlerMethod == NO_METHOD_FOUND ? null : handlerMethod). That should work. Then you add the errorHandler to your context. If you are wiring this up with XML, replace

        <bean class="org.springframework.web.servlet.mvc.annotation.AnnotationMethodHandlerExceptionResolver"/>
        

        with

        <bean class="com.yourcompany.AnnotationMethodHandlerExceptionResolver"/>
        

        Personally, I'm waiting for Spring to fix this in the next release. We're using two ExceptionResolvers, so when the first fails we fall back to a SimpleMappingExceptionResolver.

        Show
        Morten Andersen-Gott added a comment - Mike, you can implement a new com.yourcompany.AnnotationMethodHandlerExceptionResolver based on the code from Spring's AnnotationMethodHandlerExceptionResolver (copy the source). In the findBestExceptionHandlerMethod -method change the last line of code to return (handlerMethod == NO_METHOD_FOUND ? null : handlerMethod). That should work. Then you add the errorHandler to your context. If you are wiring this up with XML, replace <bean class= "org.springframework.web.servlet.mvc.annotation.AnnotationMethodHandlerExceptionResolver" /> with <bean class= "com.yourcompany.AnnotationMethodHandlerExceptionResolver" /> Personally, I'm waiting for Spring to fix this in the next release. We're using two ExceptionResolvers, so when the first fails we fall back to a SimpleMappingExceptionResolver.
        Hide
        Rossen Stoyanchev added a comment -

        You also have the choice of using the new @MVC support classes. See the what's new section in the reference docs.

        Show
        Rossen Stoyanchev added a comment - You also have the choice of using the new @MVC support classes. See the what's new section in the reference docs.

          People

          • Assignee:
            Rossen Stoyanchev
            Reporter:
            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:
              2 years, 6 weeks ago