Commons Jelly
  1. Commons Jelly
  2. JELLY-277

XMLParser.configure is not threadsafe.

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.0
    • Fix Version/s: None
    • Component/s: core / taglib.core
    • Labels:
      None
    • Environment:

      Linux 64bits, JDK build 1.6.0_01-b06

      Description

      XMLParser will try to load jelly.properties from disk when it first being used. However, because XMLParser.configure method is not threadsafe, when I tried to run multiple jelly scripts/instance at the same time, XMLParser threw out the following error message

      java.lang.ClassNotFoundException: core
      at org.apache.tools.ant.AntClassLoader.findClassInComponents(AntClassLoader.java:1166)
      at org.apache.tools.ant.AntClassLoader.findClass(AntClassLoader.java:1107)
      at org.apache.tools.ant.AntClassLoader.loadClass(AntClassLoader.java:977)
      at java.lang.ClassLoader.loadClass(ClassLoader.java:251)
      at org.apache.commons.jelly.parser.XMLParser.createTag(XMLParser.java:985)

      java.lang.ClassNotFoundException: core
      at org.apache.commons.jelly.parser.XMLParser.createSAXException(XMLParser.java:1180)
      at org.apache.commons.jelly.parser.XMLParser.createTag(XMLParser.java:990)
      at org.apache.commons.jelly.parser.XMLParser.startElement(XMLParser.java:593)

      In order to fix this problem, we have to make ensureConfigured() and configure() synchronized methods.

        Activity

        Hide
        Sean Harp added a comment -

        The compileScript method in org/apache/commons/jelly/JellyContext.java needs to be synchronized as well. This solved the issue for us.

        Prior to fixing this, I would experience nearly 50% failures with the error above when running 12 jelly scripts simultaneously on an 8-core server. After synchronizing compileScript, repeated tests with 50 simultaneous jelly scripts were successful.

        Show
        Sean Harp added a comment - The compileScript method in org/apache/commons/jelly/JellyContext.java needs to be synchronized as well. This solved the issue for us. Prior to fixing this, I would experience nearly 50% failures with the error above when running 12 jelly scripts simultaneously on an 8-core server. After synchronizing compileScript, repeated tests with 50 simultaneous jelly scripts were successful.
        Hide
        Frank van der Kleij added a comment -

        Since the variable jellyProperties is static making the method synchronized is not enough; there will still be race conditions between different instances of XMLParser.

        First it should be clear what the desired behaviour is:

        • is it essential that the properties are loaded only once, or
        • is it just a cache to avoid processing each time

        In the first case it is probably best to load the properties in a static block, through a static synchronized method or in a block synchronized on a static instance.

        private static Properties jellyProperties;

        static

        { jellyProperties = xxx; }

        or

        protected synchronized Properties getJellyProperties()

        { return _getJellyProperties(); }

        protected static synchronized Properties _getJellyProperties() {
        if (jellyProperties == null)

        {...}
        ...
        }

        or

        protected synchronized Properties getJellyProperties() {

        synchronized (this.getClass()){
        if (jellyProperties == null) {...}

        ...
        }

        In the second case it is enough to first fill a local Properties object before assigning it to the static variable; and not first assign it and then fill it because that corrupts the test on the 'nullness' of the jellyProperties. This avoids any additional synchronization but might get the properties to be filled in a number of times in parallel. As long as the properties are never modified this should be enough.

        protected synchronized Properties getJellyProperties() {
        if (jellyProperties == null)

        { Properties tmpJellyProperties = new Properties(); ... jellyProperties = tmpJellyProperties; }

        ...

        Show
        Frank van der Kleij added a comment - Since the variable jellyProperties is static making the method synchronized is not enough; there will still be race conditions between different instances of XMLParser. First it should be clear what the desired behaviour is: is it essential that the properties are loaded only once, or is it just a cache to avoid processing each time In the first case it is probably best to load the properties in a static block, through a static synchronized method or in a block synchronized on a static instance. private static Properties jellyProperties; static { jellyProperties = xxx; } or protected synchronized Properties getJellyProperties() { return _getJellyProperties(); } protected static synchronized Properties _getJellyProperties() { if (jellyProperties == null) {...} ... } or protected synchronized Properties getJellyProperties() { synchronized (this.getClass()){ if (jellyProperties == null) {...} ... } In the second case it is enough to first fill a local Properties object before assigning it to the static variable; and not first assign it and then fill it because that corrupts the test on the 'nullness' of the jellyProperties. This avoids any additional synchronization but might get the properties to be filled in a number of times in parallel. As long as the properties are never modified this should be enough. protected synchronized Properties getJellyProperties() { if (jellyProperties == null) { Properties tmpJellyProperties = new Properties(); ... jellyProperties = tmpJellyProperties; } ...

          People

          • Assignee:
            Unassigned
            Reporter:
            Yung-Lin Ho
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:

              Development