Commons JXPath
  1. Commons JXPath
  2. JXPATH-152

Concurrent access on hashmap of JXPathIntrospector

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.3
    • Fix Version/s: 1.4
    • Labels:
      None
    • Environment:

      Java5, Windows/AIX

      Description

      JXPathIntrospector.registerDynamicClass method can be called in static part of classes.
      If two classes A & B try to registerDynamicClass in the same time a concurrent access exception can append on hashmap of JXPathIntrospector.

      Replace hashmap by concurrent hashmap or synchronized access to these hashmaps.

        Activity

        Hide
        Naozumi Taromaru added a comment -

        I think that this issue is a critical bug.

        For example,
        if the following code(an ordinary code) is executed in a multithread environment,
        all threads hang-up(infinity loop) occurs in the worst case.


        JXPathContext context = JXPathContext.newContext(target);
        Iterator ite = context.iteratePointers(requestXpath);
        while (ite.hasNext())

        { Pointer p = (Pointer) ite.next(); ... }

        Thread-1 use JXPathContext instance-1.
        Thread-2 use JXPathContext instance-2.

        However,
        Thread-1 and Thread-2 may execute
        JXPathIntrospector#getBeanInfo (static method)
        concurrently.

        JXPathIntrospector#getBeanInfo execute
        "byClass.put(beanClass, beanInfo);".
        ("byClass" is HashMap in static field)

        That is,
        Thread-1 and Thread-2 may execute
        HashMap#put concurrently.

        Thread-1 and Thread-2 use same HashMap instance (in static field).

        In the worst case,
        Thread-1 and Thread-2 expand HashMap capacity concurrently.
        Then, the structure of HashMap is broken.

        If a thread use broken HashMap's method(get, put, etc...),
        the thread execute infinity loop in HashMap.

        If HashMap(byClass) is broken,
        Thread-1 use JXPathContext ... infinity loop
        Thread-2 use JXPathContext ... infinity loop
        Thread-3 use JXPathContext ... infinity loop
        ...
        As a result, all threads hang-up(infinity loop).

        Show
        Naozumi Taromaru added a comment - I think that this issue is a critical bug. For example, if the following code(an ordinary code) is executed in a multithread environment, all threads hang-up(infinity loop) occurs in the worst case. JXPathContext context = JXPathContext.newContext(target); Iterator ite = context.iteratePointers(requestXpath); while (ite.hasNext()) { Pointer p = (Pointer) ite.next(); ... } Thread-1 use JXPathContext instance-1. Thread-2 use JXPathContext instance-2. However, Thread-1 and Thread-2 may execute JXPathIntrospector#getBeanInfo (static method) concurrently. JXPathIntrospector#getBeanInfo execute "byClass.put(beanClass, beanInfo);". ("byClass" is HashMap in static field) That is, Thread-1 and Thread-2 may execute HashMap#put concurrently. Thread-1 and Thread-2 use same HashMap instance (in static field). In the worst case, Thread-1 and Thread-2 expand HashMap capacity concurrently. Then, the structure of HashMap is broken. If a thread use broken HashMap's method(get, put, etc...), the thread execute infinity loop in HashMap. If HashMap(byClass) is broken, Thread-1 use JXPathContext ... infinity loop Thread-2 use JXPathContext ... infinity loop Thread-3 use JXPathContext ... infinity loop ... As a result, all threads hang-up(infinity loop).
        Hide
        Matt Benson added a comment - - edited

        It would seem that synchronizing each HashMap#put() call against the target map would defend against the worst-case scenarios you suggest. Thanks for the report!

        Committed revision 1293412.

        Show
        Matt Benson added a comment - - edited It would seem that synchronizing each HashMap#put() call against the target map would defend against the worst-case scenarios you suggest. Thanks for the report! Committed revision 1293412.
        Hide
        Naozumi Taromaru added a comment -

        Collections.synchronizedMap is thread safe method.

        However, since these HashMap are referred to frequently,
        it becomes a bottleneck by the method of synchronizedMap's lock
        in a multi-core CPU (or multi CPU) environment.

        I recommend correcting by one of the following methods.
        These methods can perform Map#get concurrently.

        • Since byInterface resembles byClass, byInterface omits.

        Before correction: (Before #1293412 modification.)


        private static HashMap byClass = new HashMap();
        ...
        byClass.put(...);
        ...
        beanInfo = (JXPathBeanInfo) byClass.get(...);


        correction method 1: (Easy)


        private static Map byClass = ConcurrentHashMap();
        ...
        byClass.put(...);
        ...
        beanInfo = (JXPathBeanInfo) byClass.get(...);


        • ConcurrentHashMap : java.util.concurrent.ConcurrentHashMap (JDK5 or later)

        The feature of the method 1:

        • Unlike JXPath-1.3, required JDK5 or later.
        • Unlike JXPath-1.3, if key or value is null, NullPointerException occur.
          (If you maintain the compatibility when key or value is null value, please see method 2.)

        correction method 2: (Specification is full compatible)


        private static HashMap byClass = new HashMap();
        private static final ReentrantReadWriteLock byClassReadWriteLock = new ReentrantReadWriteLock();
        private static final Lock byClassReadLock = byClassReadWriteLock.readLock();
        private static final Lock byClassWriteLock = byClassReadWriteLock.writeLock();
        ...
        byClassWriteLock.lock();
        try

        { byClass.put(...); } finally { byClassWriteLock.unlock(); }
        ...
        byClassReadLock.lock();
        try { beanInfo = (JXPathBeanInfo) byClass.get(...); } finally { byClassReadLock.unlock(); }
        ----
        * ReentrantReadWriteLock : java.util.concurrent.locks.ReentrantReadWriteLock (JDK5 or later)
        * Lock : java.util.concurrent.locks.Lock (JDK5 or later)
        please see
        http://docs.oracle.com/javase/1.5.0/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html
        "RWDictionary" example.

        The feature of the method 2:
        * Unlike JXPath-1.3, required JDK5 or later.
        * Like JXPath-1.3, null can be used for key or value.


        correction method 3: (JDK1.3 compatible. but contrary to usage.)
        ----
        private static HashMap byClass = new HashMap();
        ...
        synchronized (byClass) { byClass.put(...); }

        ...
        beanInfo = (JXPathBeanInfo) byClass.get(...);
        if (beanInfo == null) { // not mapping, null mapping, or when byClass is being expanded.
        synchronized (byClass)

        { beanInfo = (JXPathBeanInfo) byClass.get(...); }

        }


        • Although it is contrary to usage,
          this method can be used if it is java.util.HashMap,
          using method are "put" "get" only.

        (At least in the case of Sun(Oracle) JDK.)

        The feature of the method 3:

        • Like JXPath-1.3, JDK1.3 compatible.
        • Like JXPath-1.3, null can be used for key or value.
        • It is contrary to the following description of HashMap's API document.
          "If multiple threads access this map concurrently,
          and at least one of the threads modifies the map structurally,
          it must be synchronized externally."
          (http://docs.oracle.com/javase/1.5.0/docs/api/java/util/HashMap.html)
        Show
        Naozumi Taromaru added a comment - Collections.synchronizedMap is thread safe method. However, since these HashMap are referred to frequently, it becomes a bottleneck by the method of synchronizedMap's lock in a multi-core CPU (or multi CPU) environment. I recommend correcting by one of the following methods. These methods can perform Map#get concurrently. Since byInterface resembles byClass, byInterface omits. Before correction: (Before #1293412 modification.) private static HashMap byClass = new HashMap(); ... byClass.put(...); ... beanInfo = (JXPathBeanInfo) byClass.get(...); correction method 1: (Easy) private static Map byClass = ConcurrentHashMap(); ... byClass.put(...); ... beanInfo = (JXPathBeanInfo) byClass.get(...); ConcurrentHashMap : java.util.concurrent.ConcurrentHashMap (JDK5 or later) The feature of the method 1: Unlike JXPath-1.3, required JDK5 or later. Unlike JXPath-1.3, if key or value is null, NullPointerException occur. (If you maintain the compatibility when key or value is null value, please see method 2.) correction method 2: (Specification is full compatible) private static HashMap byClass = new HashMap(); private static final ReentrantReadWriteLock byClassReadWriteLock = new ReentrantReadWriteLock(); private static final Lock byClassReadLock = byClassReadWriteLock.readLock(); private static final Lock byClassWriteLock = byClassReadWriteLock.writeLock(); ... byClassWriteLock.lock(); try { byClass.put(...); } finally { byClassWriteLock.unlock(); } ... byClassReadLock.lock(); try { beanInfo = (JXPathBeanInfo) byClass.get(...); } finally { byClassReadLock.unlock(); } ---- * ReentrantReadWriteLock : java.util.concurrent.locks.ReentrantReadWriteLock (JDK5 or later) * Lock : java.util.concurrent.locks.Lock (JDK5 or later) please see http://docs.oracle.com/javase/1.5.0/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html "RWDictionary" example. The feature of the method 2: * Unlike JXPath-1.3, required JDK5 or later. * Like JXPath-1.3, null can be used for key or value. correction method 3: (JDK1.3 compatible. but contrary to usage.) ---- private static HashMap byClass = new HashMap(); ... synchronized (byClass) { byClass.put(...); } ... beanInfo = (JXPathBeanInfo) byClass.get(...); if (beanInfo == null) { // not mapping, null mapping, or when byClass is being expanded. synchronized (byClass) { beanInfo = (JXPathBeanInfo) byClass.get(...); } } Although it is contrary to usage, this method can be used if it is java.util.HashMap, using method are "put" "get" only. (At least in the case of Sun(Oracle) JDK.) The feature of the method 3: Like JXPath-1.3, JDK1.3 compatible. Like JXPath-1.3, null can be used for key or value. It is contrary to the following description of HashMap's API document. "If multiple threads access this map concurrently, and at least one of the threads modifies the map structurally, it must be synchronized externally." ( http://docs.oracle.com/javase/1.5.0/docs/api/java/util/HashMap.html )
        Hide
        Naozumi Taromaru added a comment -

        > correction method 3: (JDK1.3 compatible. but contrary to usage.)
        Sorry, it has problem.
        ArrayIndexOutOfBoundsException may occur.

        The problem did not occur in the case of Sun(Oracle) JDK5.
        result of get method:

        • return null
        • return value object

        But, the problem occured in the case of Sun(Oracle) JDK6.
        result of get method:

        • return null
        • return value object
        • throw ArrayIndexOutOfBoundsException
        Show
        Naozumi Taromaru added a comment - > correction method 3: (JDK1.3 compatible. but contrary to usage.) Sorry, it has problem. ArrayIndexOutOfBoundsException may occur. The problem did not occur in the case of Sun(Oracle) JDK5. result of get method: return null return value object But, the problem occured in the case of Sun(Oracle) JDK6. result of get method: return null return value object throw ArrayIndexOutOfBoundsException

          People

          • Assignee:
            Matt Benson
            Reporter:
            pleutre
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 24h
              24h
              Remaining:
              Remaining Estimate - 24h
              24h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development