Spring Security
  1. Spring Security
  2. SEC-1532

ProtectPointcutPostProcessor should not re-attempt to match the pointcuts against methods when used with prototype beans

    Details

    • Type: Defect Defect
    • Status: Closed
    • Priority: Major Major
    • Resolution: Complete
    • Affects Version/s: 2.0.5
    • Fix Version/s: 3.1.0.M1, 3.0.4
    • Component/s: Core
    • Labels:
      None
    • Environment:
      JVM: reproducable on Windows XP using jdk1.6.0_13 and on Linux (2.6.18) using Server VM (build 16.3-b01, mixed mode) on a Tomcat: 6.0.29. Spring Version is 2.5.6

      Description

      In our web application we use spring security combining url security as well as method security using XML configuration only.

      The webapp is being deployed on a stock Tomcat 6.0.29 using JDK6.

      • URL-security is configured using <sec:intercept-url /> tags only.
      • Method-security is configured using <sec:protect-pointcut /> tags only referencing a central Pointcuts-class. The pointcut class implements dummy methods being referenced in the method security XML configuration. The reason for doing so is to have descriptive names for the pointcut expressions. The pointcut expressions are being configured using @Pointcut annotations.

      The URL- and method-security XML configuration files are seperate. We don't use any annotation based security configuration. We recognized a very slow bootstrap performance as well as a very poor runtime performance being caused by spring security. When disabling method-security performance is as expected.

      The performance difference at bootstrap is about 30 sec with method-security enabled and about 6 sec with method-security disabled.

      During runtime opening pages takes a couple seconds when method-security is enabled and only a fraction of a second with method-security disabled.

      The performance difference is so huge and evident that we did not even make concrete performance measurements in order to quantize the difference.

      During bootstrap we made random thread dumps recognizing the same pattern over and over where the main-Threads is stuck:

      "main" prio=10 tid=0x080e1800 nid=0xbcc waiting on condition [0xb73e6000]
      java.lang.Thread.State: RUNNABLE
      at java.net.URLClassLoader.findClass(URLClassLoader.java:190)
      at java.lang.ClassLoader.loadClass(ClassLoader.java:307)

      • locked <0xa4000500> (a sun.misc.Launcher$AppClassLoader)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:301)
      • locked <0xa4000500> (a sun.misc.Launcher$AppClassLoader)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:248)
        at org.apache.catalina.loader.WebappClassLoader.loadClass(WebappClassLoader.java:1560)
      • locked <0xa4149438> (a org.apache.catalina.loader.WebappClassLoader)
        at org.apache.catalina.loader.WebappClassLoader.loadClass(WebappClassLoader.java:1491)
        at java.lang.Class.forName0(Native Method)
        at java.lang.Class.forName(Class.java:247)
        at org.aspectj.weaver.reflect.ReflectionBasedReferenceTypeDelegateFactory.createDelegate(ReflectionBasedReferenceTypeDelegateFactory.java:40)
        at org.aspectj.weaver.reflect.ReflectionWorld.resolveDelegate(ReflectionWorld.java:111)
        at org.aspectj.weaver.World.resolveToReferenceType(World.java:388)
        at org.aspectj.weaver.World.resolve(World.java:279)
        at org.aspectj.weaver.patterns.SimpleScope.lookupType(SimpleScope.java:57)
        at org.aspectj.weaver.bcel.AtAjAttributes$BindingScope.lookupType(AtAjAttributes.java:1680)
        at org.aspectj.weaver.patterns.WildTypePattern.lookupTypeInScope(WildTypePattern.java:716)
        at org.aspectj.weaver.patterns.WildTypePattern.resolveBindingsFromFullyQualifiedTypeName(WildTypePattern.java:703)
        at org.aspectj.weaver.patterns.WildTypePattern.resolveBindings(WildTypePattern.java:631)
        at org.aspectj.weaver.patterns.TypePattern.resolveExactType(TypePattern.java:194)
        at org.aspectj.weaver.patterns.ReferencePointcut.resolveBindings(ReferencePointcut.java:131)
        at org.aspectj.weaver.patterns.Pointcut.resolve(Pointcut.java:196)
        at org.aspectj.weaver.tools.PointcutParser.resolvePointcutExpression(PointcutParser.java:332)
        at org.aspectj.weaver.tools.PointcutParser.parsePointcutExpression(PointcutParser.java:310)
        at org.aspectj.weaver.tools.PointcutParser.parsePointcutExpression(PointcutParser.java:288)
        at org.springframework.security.intercept.method.ProtectPointcutPostProcessor.postProcessBeforeInitialization(ProtectPointcutPostProcessor.java:103)
        ...

      During runtime load we also made thread dumps randomly recognized the same pattern where the request threads are being stuck. During runtime we have another problem that other http request are being blocked. This is a production no-go.

      The blocking thread:

      "http-8120-6" daemon prio=10 tid=0x08e1a000 nid=0x7912 waiting on condition [0x9a8ff000]
      java.lang.Thread.State: RUNNABLE
      at java.lang.ClassLoader.loadClass(ClassLoader.java:307)

      • locked <0xa3fab6c0> (a sun.misc.Launcher$ExtClassLoader)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:296)
      • locked <0xa3fab678> (a sun.misc.Launcher$AppClassLoader)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:301)
      • locked <0xa3fab678> (a sun.misc.Launcher$AppClassLoader)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:248)
        at org.apache.catalina.loader.WebappClassLoader.loadClass(WebappClassLoader.java:1560)
      • locked <0xa40cf610> (a org.apache.catalina.loader.WebappClassLoader)
        at org.apache.catalina.loader.WebappClassLoader.loadClass(WebappClassLoader.java:1491)
        at java.lang.Class.forName0(Native Method)
        at java.lang.Class.forName(Class.java:247)
        at org.aspectj.weaver.reflect.ReflectionBasedReferenceTypeDelegateFactory.createDelegate(ReflectionBasedReferenceTypeDelegateFactory.java:40)
        at org.aspectj.weaver.reflect.ReflectionWorld.resolveDelegate(ReflectionWorld.java:111)
        at org.aspectj.weaver.World.resolveToReferenceType(World.java:388)
        at org.aspectj.weaver.World.resolve(World.java:279)
        at org.aspectj.weaver.patterns.SimpleScope.lookupType(SimpleScope.java:57)
        at org.aspectj.weaver.bcel.AtAjAttributes$BindingScope.lookupType(AtAjAttributes.java:1680)
        at org.aspectj.weaver.patterns.WildTypePattern.lookupTypeInScope(WildTypePattern.java:716)
        at org.aspectj.weaver.patterns.WildTypePattern.resolveBindingsFromFullyQualifiedTypeName(WildTypePattern.java:703)
        at org.aspectj.weaver.patterns.WildTypePattern.resolveBindings(WildTypePattern.java:631)
        at org.aspectj.weaver.patterns.TypePattern.resolveExactType(TypePattern.java:194)
        at org.aspectj.weaver.patterns.ReferencePointcut.resolveBindings(ReferencePointcut.java:131)
        at org.aspectj.weaver.patterns.Pointcut.resolve(Pointcut.java:196)
        at org.aspectj.weaver.tools.PointcutParser.resolvePointcutExpression(PointcutParser.java:332)
        at org.aspectj.weaver.tools.PointcutParser.parsePointcutExpression(PointcutParser.java:310)
        at org.aspectj.weaver.tools.PointcutParser.parsePointcutExpression(PointcutParser.java:288)
        at org.springframework.security.intercept.method.ProtectPointcutPostProcessor.postProcessBeforeInitialization(ProtectPointcutPostProcessor.java:103)
        ...

      The blocked threads:

      "http-8120-5" daemon prio=10 tid=0x08e74c00 nid=0x7910 waiting for monitor entry [0x9a950000]
      java.lang.Thread.State: BLOCKED (on object monitor)
      at java.lang.Class.forName0(Native Method)
      at java.lang.Class.forName(Class.java:247)
      at org.aspectj.weaver.reflect.ReflectionBasedReferenceTypeDelegateFactory.createDelegate(ReflectionBasedReferenceTypeDelegateFactory.java:40)
      at org.aspectj.weaver.reflect.ReflectionWorld.resolveDelegate(ReflectionWorld.java:111)
      ...

      "http-8120-4" daemon prio=10 tid=0x08e73000 nid=0x790f waiting for monitor entry [0x9a9a1000]
      java.lang.Thread.State: BLOCKED (on object monitor)
      at java.lang.Class.forName0(Native Method)
      at java.lang.Class.forName(Class.java:247)
      at org.aspectj.weaver.reflect.ReflectionBasedReferenceTypeDelegateFactory.createDelegate(ReflectionBasedReferenceTypeDelegateFactory.java:40)
      ...

      "http-8120-3" daemon prio=10 tid=0x09325000 nid=0x790e waiting for monitor entry [0x9a9f2000]
      java.lang.Thread.State: BLOCKED (on object monitor)
      at java.lang.Class.forName0(Native Method)
      at java.lang.Class.forName(Class.java:247)
      at org.aspectj.weaver.reflect.ReflectionBasedReferenceTypeDelegateFactory.createDelegate(ReflectionBasedReferenceTypeDelegateFactory.java:40)
      ...

      "http-8120-1" daemon prio=10 tid=0x093d8400 nid=0x3f18 waiting for monitor entry [0x9af95000]
      java.lang.Thread.State: BLOCKED (on object monitor)
      at java.lang.Class.forName0(Native Method)
      at java.lang.Class.forName(Class.java:247)
      at org.aspectj.weaver.reflect.ReflectionBasedReferenceTypeDelegateFactory.createDelegate(ReflectionBasedReferenceTypeDelegateFactory.java:40)
      ...

      Please find attached a source code and configuration sample.

      We found a couple places on the forums where the problem is being recognized but so far we did not find any JIRA issue.

      Our current planned resolution is to avoid pointcut expressions for method-security at all. Instead we will use either annotations or the intercept-methods bean decorator and hope that performance increases. It would be nice though to use pointcut expressions.

      1. git.patch
        2 kB
        Sergiusz Urbaniak
      2. ProtectPointcutPostProcessor.java
        7 kB
        Sergiusz Urbaniak

        Activity

        Hide
        Sergiusz Urbaniak added a comment -

        Another comment:

        The ProtectPointcutPostProcessor calls "PointcutExpression expression = parser.parsePointcutExpression(ex);" (in line 103) which is pretty expensive since the whole classpath is being scanned.

        This is "not a big problem" as long as you have singleton beans only in your context. Then only during bootstrap each bean is being scanned for pointcut matches.

        Once you have prototype scoped beans (as we do) then you also have a problem at runtime. The ProtectPointcutPostProcessor then calls the expensive parsePointcutExpression method also at each instantiation of a prototype scoped bean. In our case this happens at every http request.

        Show
        Sergiusz Urbaniak added a comment - Another comment: The ProtectPointcutPostProcessor calls "PointcutExpression expression = parser.parsePointcutExpression(ex);" (in line 103) which is pretty expensive since the whole classpath is being scanned. This is "not a big problem" as long as you have singleton beans only in your context. Then only during bootstrap each bean is being scanned for pointcut matches. Once you have prototype scoped beans (as we do) then you also have a problem at runtime. The ProtectPointcutPostProcessor then calls the expensive parsePointcutExpression method also at each instantiation of a prototype scoped bean. In our case this happens at every http request.
        Hide
        Luke Taylor added a comment -

        The pointcut matching takes place in a BeanPostProcessor which is applied to each bean on startup, so the startup time hit is to be expected and not much can be done about it. The same behaviour is responsible for the runtime hit, because if you are using prototypes, Spring will re-invoke each BeanPostProcessor on the bean instance. It was not really originally envisaged that people would use the pointcut matching other than with singleton beans. The best we can really do is to add some sort of cache based on bean name to the PostProcessor so that it does not re-evaluate the pointcut each time. But you are better to use native AspectJ if you want to be as efficient as possible and still use pointcuts.

        Show
        Luke Taylor added a comment - The pointcut matching takes place in a BeanPostProcessor which is applied to each bean on startup, so the startup time hit is to be expected and not much can be done about it. The same behaviour is responsible for the runtime hit, because if you are using prototypes, Spring will re-invoke each BeanPostProcessor on the bean instance. It was not really originally envisaged that people would use the pointcut matching other than with singleton beans. The best we can really do is to add some sort of cache based on bean name to the PostProcessor so that it does not re-evaluate the pointcut each time. But you are better to use native AspectJ if you want to be as efficient as possible and still use pointcuts.
        Hide
        Luke Taylor added a comment -

        Changing the title to reflect that the runtime issue is the main concern (when used with prototype beans). We can't avoid the overhead of parsing the pointcuts initially, but they are already cached and the runtime overhead is due to matching against the methods. The overhead will increase with the number of pointcuts used and the number of methods in the prototype bean.

        Show
        Luke Taylor added a comment - Changing the title to reflect that the runtime issue is the main concern (when used with prototype beans). We can't avoid the overhead of parsing the pointcuts initially, but they are already cached and the runtime overhead is due to matching against the methods. The overhead will increase with the number of pointcuts used and the number of methods in the prototype bean.
        Hide
        Luke Taylor added a comment -

        I've added a simple bean name cache to ProtectPointcutPostProcessor. If it has already done its work for a particular bean name, it will do nothing. Since the pointcut matching is purely a function of the class and method information, the security metadata should be the same each time the bean is retrieved from the bean factory.

        Show
        Luke Taylor added a comment - I've added a simple bean name cache to ProtectPointcutPostProcessor. If it has already done its work for a particular bean name, it will do nothing. Since the pointcut matching is purely a function of the class and method information, the security metadata should be the same each time the bean is retrieved from the bean factory.
        Hide
        Sergiusz Urbaniak added a comment -

        Great, thanks for the quick reaction! A cache implementation was our first idea too.

        May I request a backport merge into 2.0.5? We'll patch our version in order to reduce refactoring efforts.

        Show
        Sergiusz Urbaniak added a comment - Great, thanks for the quick reaction! A cache implementation was our first idea too. May I request a backport merge into 2.0.5? We'll patch our version in order to reduce refactoring efforts.
        Hide
        Luke Taylor added a comment -

        The commit has been applied to the 3.0.x branch and master. The 2.0.x branch is no longer under active development, but if you want to reformat the changes as a patch for it I'll certainly apply it.

        Show
        Luke Taylor added a comment - The commit has been applied to the 3.0.x branch and master. The 2.0.x branch is no longer under active development, but if you want to reformat the changes as a patch for it I'll certainly apply it.
        Hide
        Sergiusz Urbaniak added a comment -

        Based on the official git repo I locally checked out the "2.0.5.RELEASE tag" and created the attached git.patch on a local branch using "git format-patch". The changed source file for the ProtectPointcutProcessor.java is attached too.

        A "mvn compile" and "mvn test" on the root source directory resulted in a successfull build.

        Show
        Sergiusz Urbaniak added a comment - Based on the official git repo I locally checked out the "2.0.5.RELEASE tag" and created the attached git.patch on a local branch using "git format-patch". The changed source file for the ProtectPointcutProcessor.java is attached too. A "mvn compile" and "mvn test" on the root source directory resulted in a successfull build.
        Hide
        Luke Taylor added a comment -

        Thanks. I've applied the patch to the 2.0.x branch too.

        Show
        Luke Taylor added a comment - Thanks. I've applied the patch to the 2.0.x branch too.

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Sergiusz Urbaniak
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: