Velocity
  1. Velocity
  2. VELOCITY-750

org.apache.velocity.runtime.RuntimeInstance initialization is not ThreadSafe

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6.2, 1.6.x
    • Fix Version/s: 1.6.x, 1.7, 2.x
    • Component/s: Engine
    • Labels:
      None
    • Environment:
      Vista, Java 1.6

      Description

      Two threads call Velocity.evaluate() method.
      Call RuntimeInstance.parse() and requireInitialization() method.
      One thread trying make initialization, set "initializing" to true and the other thread does not wait until initialization is done and go on.
      At next line Parser parser = (Parser) parserPool.get() is NullPointerException thrown.

        Issue Links

          Activity

          Hide
          Jarkko Viinamäki added a comment -

          If I'm not mistaken, code in RuntimeInstance class seems to have some sort of broken "Double Checked Locking" implementation. See http://jeremymanson.blogspot.com/2007/05/double-checked-locking-and-problem-with.html

          Proper synchronization will introduce a performance penalty for all page renderings.

          Show
          Jarkko Viinamäki added a comment - If I'm not mistaken, code in RuntimeInstance class seems to have some sort of broken "Double Checked Locking" implementation. See http://jeremymanson.blogspot.com/2007/05/double-checked-locking-and-problem-with.html Proper synchronization will introduce a performance penalty for all page renderings.
          Hide
          Nathan Bubna added a comment -

          The fundamental problem here is that anyone using a single RuntimeInstance in a multi-threaded environment should generally be manually init-ing the runtime. The lazy initialization was not really intended for such situations, and the double-checked locking was meant more as a convenience than a guarantee.

          With that said, i was under the impression this is NOT broken double checked locking in Java 1.5+. See:

          http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html (section about volatile at the bottom)

          Unless i've misunderstood that (or it's wrong), the init() method is synchronized and the initialized variable is marked volatile. The assignment of the volatile initialized variable should not be reordered to happen before the initialize[Foo]() calls. And the synchronized init() method shouldn't be executing simultaneously in multiple threads on the same instance. But perhaps i have mis-implemented the fixed pattern?

          Show
          Nathan Bubna added a comment - The fundamental problem here is that anyone using a single RuntimeInstance in a multi-threaded environment should generally be manually init-ing the runtime. The lazy initialization was not really intended for such situations, and the double-checked locking was meant more as a convenience than a guarantee. With that said, i was under the impression this is NOT broken double checked locking in Java 1.5+. See: http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html (section about volatile at the bottom) Unless i've misunderstood that (or it's wrong), the init() method is synchronized and the initialized variable is marked volatile. The assignment of the volatile initialized variable should not be reordered to happen before the initialize [Foo] () calls. And the synchronized init() method shouldn't be executing simultaneously in multiple threads on the same instance. But perhaps i have mis-implemented the fixed pattern?
          Hide
          Jarkko Viinamäki added a comment -

          I took a look at sources of 1.6.2 and 1.6.3 releases. They both have this (broken) code:

          /**

          • Indicate whether the Runtime is in the midst of initialization.
            */
            private boolean initializing = false;

          /**

          • Indicate whether the Runtime has been fully initialized.
            */
            private boolean initialized = false;

          which shows that the initialized variable is not marked as volatile. However, current SVN head has the volatile modification (since revision 737363 by Byron). I think with the volatile modifier it should work under Java 1.5+. However, I guess we still officially support also Java 1.3+ so it doesn't work there since the Java Memory Management Model is different. Then again, who still uses such old JREs (1.5 was released 2004)?

          Show
          Jarkko Viinamäki added a comment - I took a look at sources of 1.6.2 and 1.6.3 releases. They both have this (broken) code: /** Indicate whether the Runtime is in the midst of initialization. */ private boolean initializing = false; /** Indicate whether the Runtime has been fully initialized. */ private boolean initialized = false; which shows that the initialized variable is not marked as volatile. However, current SVN head has the volatile modification (since revision 737363 by Byron). I think with the volatile modifier it should work under Java 1.5+. However, I guess we still officially support also Java 1.3+ so it doesn't work there since the Java Memory Management Model is different. Then again, who still uses such old JREs (1.5 was released 2004)?
          Hide
          Nathan Bubna added a comment -

          Oops. I shouldn't just look at the trunk code when this was reported against 1.6. That would explain the dissonance between the user's experience and my understanding of the code.

          And i thought we'd agreed to drop support of Java 1.3 after Velocity 1.5? Of course, that still leaves Java 1.4 users with the problem. However, let's not forget that there is a very simple workaround here, which is actually the way older code always had to do it: manually call init and not rely on the new auto-init feature. I think we just need to add volatile to the initialized property in the 1.6 branch and get a 1.6.4 release out. Anyone still using an outdated JRE can do initialization the old way. Does Sun even still support Java 1.4?

          Oh, and for reference, Byron's adding of volatile was along with VELOCITY-673, and we've had the double-checked locking conversation about other code in VELOCITY-536.

          Show
          Nathan Bubna added a comment - Oops. I shouldn't just look at the trunk code when this was reported against 1.6. That would explain the dissonance between the user's experience and my understanding of the code. And i thought we'd agreed to drop support of Java 1.3 after Velocity 1.5? Of course, that still leaves Java 1.4 users with the problem. However, let's not forget that there is a very simple workaround here, which is actually the way older code always had to do it: manually call init and not rely on the new auto-init feature. I think we just need to add volatile to the initialized property in the 1.6 branch and get a 1.6.4 release out. Anyone still using an outdated JRE can do initialization the old way. Does Sun even still support Java 1.4? Oh, and for reference, Byron's adding of volatile was along with VELOCITY-673 , and we've had the double-checked locking conversation about other code in VELOCITY-536 .
          Hide
          Nathan Bubna added a comment -

          Fixed for Java 1.5+. Users of shared Velocity runtimes in multi-threaded environments on older JREs must use manual init() call.

          Show
          Nathan Bubna added a comment - Fixed for Java 1.5+. Users of shared Velocity runtimes in multi-threaded environments on older JREs must use manual init() call.
          Hide
          Cenek Rauscher added a comment - - edited

          I think that "Double Checked Locking" is not main problem in this bug (but anyway you should also fix it). Main problem is using "initialized" and "initializing" fields. When initialization is started (synchronized init() method), "initializing" is set to true and requireInitialization() method not wait when initialization is done. I think that using both fields is incorrect - at least in requireInitialization() method.

          Show
          Cenek Rauscher added a comment - - edited I think that "Double Checked Locking" is not main problem in this bug (but anyway you should also fix it). Main problem is using "initialized" and "initializing" fields. When initialization is started (synchronized init() method), "initializing" is set to true and requireInitialization() method not wait when initialization is done. I think that using both fields is incorrect - at least in requireInitialization() method.
          Hide
          Nathan Bubna added a comment -

          Yeah, and the initializing variable is unneeded, since init() is synchronized and initialized can't be re-ordered. Time for it to go.

          Show
          Nathan Bubna added a comment - Yeah, and the initializing variable is unneeded, since init() is synchronized and initialized can't be re-ordered. Time for it to go.
          Hide
          Cenek Rauscher added a comment -

          Thank you

          Show
          Cenek Rauscher added a comment - Thank you

            People

            • Assignee:
              Unassigned
              Reporter:
              Cenek Rauscher
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development