Velocity
  1. Velocity
  2. VELOCITY-536

Velocity Engine throws NullPointer Exception when two people click on the same page at the same time for the first time

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5
    • Fix Version/s: 1.6
    • Component/s: Engine
    • Labels:
      None

      Description

      Multi-thread concurrency issue

      During our concurrency testing, we observed NullPointer exceptions being thrown when two people hit the same page at the same time for the first time. Upon further investigation, it turns out that we need to synchronize the init method on ASTDirective, ASTSetDirective, and render method on ASTSetDirective, and VelocimacroProxy.

      Basically, the problem is introduced as the following; when two threads attempts to parse and render the same template at the same time. Thread1 finishes parsing first and proceeds to the render method call, while thread 2 is still busy parsing and will overwrite the existing parse tree that is being used by thread 1 for rendering purpose. Thus under certainly condition a NullPointer exception will be thrown.

      1. 536-patch.txt
        7 kB
        Lei Gu
      2. VelocimacroProxy.java
        14 kB
        Lei Gu
      3. ASTDirective.java
        7 kB
        Lei Gu
      4. ASTSetDirective.java
        7 kB
        Lei Gu

        Activity

        Hide
        Lei Gu added a comment -

        Fixes for the multi-threaded concurrency are to synchronized properly on init method on ASTSetDirective, ASTDirective, render method on ASTSetDirective and VelocimacroProxy.

        Show
        Lei Gu added a comment - Fixes for the multi-threaded concurrency are to synchronized properly on init method on ASTSetDirective, ASTDirective, render method on ASTSetDirective and VelocimacroProxy.
        Hide
        Lei Gu added a comment -

        Patch fixes

        Show
        Lei Gu added a comment - Patch fixes
        Hide
        Will Glass-Husain added a comment -
        Show
        Will Glass-Husain added a comment - Thanks for this as well. More info at: http://www.mail-archive.com/dev@velocity.apache.org/msg01553.html
        Hide
        Will Glass-Husain added a comment -

        Incidentally, there's no need to attach the complete files - the patch file is enough. (Using Eclipse or the utility "patch" it's easy to generate the modified source).

        Show
        Will Glass-Husain added a comment - Incidentally, there's no need to attach the complete files - the patch file is enough. (Using Eclipse or the utility "patch" it's easy to generate the modified source).
        Hide
        Will Glass-Husain added a comment -

        Quick question on the patch. Should the null check be inside the synchronized loop? I worry this might fall into the anti-pattern "double-checked locking" when the null test is outside of the loop. Anyone experienced in such matters want to take a look?

        Show
        Will Glass-Husain added a comment - Quick question on the patch. Should the null check be inside the synchronized loop? I worry this might fall into the anti-pattern "double-checked locking" when the null test is outside of the loop. Anyone experienced in such matters want to take a look?
        Hide
        Nathan Bubna added a comment -

        Null check? I see boolean checks...

        Anyway, double-checked locking is apparently OK in JDK 1.5 if you are checking a field declared as volatile. I learned this here:
        http://crazybob.org/2007/01/lazy-loading-singletons.html Since we are not targetting JDK 1.5 yet, we probably should not use DCL.

        Regardless of that, i thought the issues with double-checked locking were surrounding lack of atomicity in construction of the checked object (and perhaps other surprising memory model nuances). Since the object being checked is an already created boolean primitive, i don't think this is a problem. Also, it solved the problem they were experiencing...

        Of course, i'm definitely not a memory model expert and would happily take correction here in order to learn better.

        Show
        Nathan Bubna added a comment - Null check? I see boolean checks... Anyway, double-checked locking is apparently OK in JDK 1.5 if you are checking a field declared as volatile. I learned this here: http://crazybob.org/2007/01/lazy-loading-singletons.html Since we are not targetting JDK 1.5 yet, we probably should not use DCL. Regardless of that, i thought the issues with double-checked locking were surrounding lack of atomicity in construction of the checked object (and perhaps other surprising memory model nuances). Since the object being checked is an already created boolean primitive, i don't think this is a problem. Also, it solved the problem they were experiencing... Of course, i'm definitely not a memory model expert and would happily take correction here in order to learn better.
        Hide
        Christopher Schultz added a comment -

        Regarding double-checked locking (DCL), it is typically used in conjunction with a reference type where many things can go wrong.

        In this case, the DCL is being performed on a 32-bit primitive type, which is guaranteed by the VM to be assigned atomically. Since there's no object initialization to perform (as there would be if you do something like "foo = new FooType()"), there are no set-but-not-initialized issues, here.

        The only potential problem is that the VM or JIT could re-order instructions within the synchronized block and end up setting init=true before the constructor has completed and/or the reference has been set. So, although 32-bit primitives will work with DCL, the fact that we are using a 32-bit primitive to protect a reference complicates things.

        The question to ask ourselves is "how bad is the contention for synchronization"? Is this a method that will be called a lot? How any threads should be expected to call this method? Is it only an issue during the initial parsing, etc. of the template? Or, will it be called many times leading to a severe performance degradation? Uncontested locks are pretty fast these days in Java, so it might not be worth the risk.

        Another option would be to modify the architecture of the code such that we minimize the use of this method such that the synchronization has less of an impact in a steady-state scenario. It's fine if the initialization code for a template or macro has some slowdown, but repeated processing should avoid this if possible.

        My recommendation would be to remove the use of the DCL idiom, and remove "volatile" from the declaration of "init", since it can't be relied upon. It looks like it's just in there to support the DCL in the first place.

        More information on DCL can be found here:
        http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-double.html
        and here:
        http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

        Show
        Christopher Schultz added a comment - Regarding double-checked locking (DCL), it is typically used in conjunction with a reference type where many things can go wrong. In this case, the DCL is being performed on a 32-bit primitive type, which is guaranteed by the VM to be assigned atomically. Since there's no object initialization to perform (as there would be if you do something like "foo = new FooType()"), there are no set-but-not-initialized issues, here. The only potential problem is that the VM or JIT could re-order instructions within the synchronized block and end up setting init=true before the constructor has completed and/or the reference has been set. So, although 32-bit primitives will work with DCL, the fact that we are using a 32-bit primitive to protect a reference complicates things. The question to ask ourselves is "how bad is the contention for synchronization"? Is this a method that will be called a lot? How any threads should be expected to call this method? Is it only an issue during the initial parsing, etc. of the template? Or, will it be called many times leading to a severe performance degradation? Uncontested locks are pretty fast these days in Java, so it might not be worth the risk. Another option would be to modify the architecture of the code such that we minimize the use of this method such that the synchronization has less of an impact in a steady-state scenario. It's fine if the initialization code for a template or macro has some slowdown, but repeated processing should avoid this if possible. My recommendation would be to remove the use of the DCL idiom, and remove "volatile" from the declaration of "init", since it can't be relied upon. It looks like it's just in there to support the DCL in the first place. More information on DCL can be found here: http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-double.html and here: http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
        Hide
        Nathan Bubna added a comment -

        "The only potential problem is that the VM or JIT could re-order instructions within the synchronized block and end up setting init=true before the constructor has completed and/or the reference has been set. So, although 32-bit primitives will work with DCL, the fact that we are using a 32-bit primitive to protect a reference complicates things."

        But the synchronization should only allow one thread at a time into the block in which setup() is called and init is set to true. Even though those instructions may be re-ordered, they should both be completed before another thread enters the block. So their order doesn't matter. The reference being checked (the init boolean) will be set properly before a second thread comes to the double check of the init reference.

        Please not that i'm not at all attached to using DCL here. Christopher's suggestions are the best in the long run anyway. Of course, someone needs to be willing to do that work and test it. As things are, Lei has tested that this solution solves his problem, and i think it should be safe from the usual DCL pitfalls. Until someone comes up with the superior patch, i'm inclined to take this one.

        Show
        Nathan Bubna added a comment - "The only potential problem is that the VM or JIT could re-order instructions within the synchronized block and end up setting init=true before the constructor has completed and/or the reference has been set. So, although 32-bit primitives will work with DCL, the fact that we are using a 32-bit primitive to protect a reference complicates things." But the synchronization should only allow one thread at a time into the block in which setup() is called and init is set to true. Even though those instructions may be re-ordered, they should both be completed before another thread enters the block. So their order doesn't matter. The reference being checked (the init boolean) will be set properly before a second thread comes to the double check of the init reference. Please not that i'm not at all attached to using DCL here. Christopher's suggestions are the best in the long run anyway. Of course, someone needs to be willing to do that work and test it. As things are, Lei has tested that this solution solves his problem, and i think it should be safe from the usual DCL pitfalls. Until someone comes up with the superior patch, i'm inclined to take this one.
        Hide
        Christopher Schultz added a comment -

        "Even though those instructions may be re-ordered, they should both be completed before another thread enters the block. So their order doesn't matter."

        Since "init" is being checked before the synchronized block, the ordering of the "init=true" is certainly relevant. If the VM or JIT sets init=true before constructing the object, the other thread is hosed because it skips the synchronized block entirely and gets a reference that is potentially invalid.

        JSR-133 has addressed re-orderings with respect to volatile members, and I'm not sure I fully understand why this fixes DCL, but, as Nathan points out, we cannot assume Java 1.5 availability, so we should avoid DCL at least for now.

        Show
        Christopher Schultz added a comment - "Even though those instructions may be re-ordered, they should both be completed before another thread enters the block. So their order doesn't matter." Since "init" is being checked before the synchronized block, the ordering of the "init=true" is certainly relevant. If the VM or JIT sets init=true before constructing the object, the other thread is hosed because it skips the synchronized block entirely and gets a reference that is potentially invalid. JSR-133 has addressed re-orderings with respect to volatile members, and I'm not sure I fully understand why this fixes DCL, but, as Nathan points out, we cannot assume Java 1.5 availability, so we should avoid DCL at least for now.
        Hide
        Nathan Bubna added a comment -

        But the DCL'ed block only needs to happen once per instance. Isn't the whole point to make the second thread skip the block completely? We only need one thread per instance to go through, period. Once one thread has started going through the block, it will finish and keep other threads out of there for that instance, order notwithstanding.

        And again, this is not a constructor nor a completely abstract use of DCL that we're talking about. This is a very specific use.

        Show
        Nathan Bubna added a comment - But the DCL'ed block only needs to happen once per instance. Isn't the whole point to make the second thread skip the block completely? We only need one thread per instance to go through, period. Once one thread has started going through the block, it will finish and keep other threads out of there for that instance, order notwithstanding. And again, this is not a constructor nor a completely abstract use of DCL that we're talking about. This is a very specific use.
        Hide
        Nathan Bubna added a comment -

        Looking more carefully at this, rather than trusting my first look...

        Ok, i see that there is potential for a second thread to jump ahead of the first one which is busy performing the meat of the init. This second thread then might run into trouble if it tries to do anything with the instance before the first thread finishes the setup/initialization.

        So, Christopher, you were right, there is an issue here, just not the original one. Sorry, and thanks for pressing me to keep thinking about it.

        So, if the user is running things on pre-1.5, then JIT/VM can re-order the setting of init=true within the synchronized block. Then, if the VM decides to do that and another thread comes along and finds init=true, thus skipping the synchronization, it could pass up the original thread which is still busy performing the initialization. If it then gets on to where it tries to use the incompletely initialized (but fully instantiated instance, it could get into trouble.

        So, while Lei's patch completely eliminates the chances of having the initialization code called multiple times, it does introduce the chance of even less-frequent (and thus much harder to debug) synchronization problems for those running on older JVMs. Apply Murphy's Law, and we can be sure it will happen. We'd better just synchronize the block/method, without trying to skip it via double checking. If anyone's interested, we could leave a note that once we require 1.5, we can use DCL with the volatile keyword happily.

        Show
        Nathan Bubna added a comment - Looking more carefully at this, rather than trusting my first look... Ok, i see that there is potential for a second thread to jump ahead of the first one which is busy performing the meat of the init. This second thread then might run into trouble if it tries to do anything with the instance before the first thread finishes the setup/initialization. So, Christopher, you were right, there is an issue here, just not the original one. Sorry, and thanks for pressing me to keep thinking about it. So, if the user is running things on pre-1.5, then JIT/VM can re-order the setting of init=true within the synchronized block. Then, if the VM decides to do that and another thread comes along and finds init=true, thus skipping the synchronization, it could pass up the original thread which is still busy performing the initialization. If it then gets on to where it tries to use the incompletely initialized (but fully instantiated instance, it could get into trouble. So, while Lei's patch completely eliminates the chances of having the initialization code called multiple times, it does introduce the chance of even less-frequent (and thus much harder to debug) synchronization problems for those running on older JVMs. Apply Murphy's Law, and we can be sure it will happen. We'd better just synchronize the block/method, without trying to skip it via double checking. If anyone's interested, we could leave a note that once we require 1.5, we can use DCL with the volatile keyword happily.
        Hide
        Henning Schmiedehausen added a comment -

        Hm, I'm late to that show. I did not read up all the thread, but please consider http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html which is the bible on why DCL is broken. Even though there is mentioned that it works for 32 bit integers, the summary is "this is not worth the trouble".

        I'm completely with Christopher here. DCL code should be actively removed from a code base, not added. Even if it is the corner case for "32 bit primitives".

        Show
        Henning Schmiedehausen added a comment - Hm, I'm late to that show. I did not read up all the thread, but please consider http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html which is the bible on why DCL is broken. Even though there is mentioned that it works for 32 bit integers, the summary is "this is not worth the trouble". I'm completely with Christopher here. DCL code should be actively removed from a code base, not added. Even if it is the corner case for "32 bit primitives".
        Hide
        Will Glass-Husain added a comment -

        Hi Lei,

        Went back and looked at this issue. I'd like to commit, but I think the double-checked locking won't work.

        I think if we change code like

        if(!init)
        {
        synchronized(this)
        {
        if ( !init )

        { nodeTree.init( context, rsvc); init = true; }
        }

        to:

        synchronized(this)
        {
        if ( !init )
        { nodeTree.init( context, rsvc); init = true; }

        it'd be fine. I'm really not worried about the performance hit of the synchronization. most things I've read says this isn't significant in modern JVM's.

        Seem reasonable?

        Show
        Will Glass-Husain added a comment - Hi Lei, Went back and looked at this issue. I'd like to commit, but I think the double-checked locking won't work. I think if we change code like if(!init) { synchronized(this) { if ( !init ) { nodeTree.init( context, rsvc); init = true; } } to: synchronized(this) { if ( !init ) { nodeTree.init( context, rsvc); init = true; } it'd be fine. I'm really not worried about the performance hit of the synchronization. most things I've read says this isn't significant in modern JVM's. Seem reasonable?
        Hide
        Lei Gu added a comment -

        Hi Will,
        Yes, that looks very reasonable to me. Thanks for taking care of this issue.
        – Lei

        Show
        Lei Gu added a comment - Hi Will, Yes, that looks very reasonable to me. Thanks for taking care of this issue. – Lei
        Hide
        Will Glass-Husain added a comment -

        patch applied - thanks again!

        Show
        Will Glass-Husain added a comment - patch applied - thanks again!

          People

          • Assignee:
            Unassigned
            Reporter:
            Lei Gu
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development