Activiti
  1. Activiti
  2. ACT-714

Multi-threaded usage of non thread safe java.util.HashMap in ClassNameUtil

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 5.3
    • Fix Version/s: 5.10
    • Component/s: Engine
    • Labels:
      None
    • Number of attachments :
      0

      Description

      org.activiti.engine.impl.util.ClassNameUtil uses java.util.HashMap to cache class names.

      The usage of the cachedNames hash map is NOT synchronized while this utility class might be used by more than one threads at the same time.

      This can cause severe issues: the engine might fail to initialize properly or unexpected exceptions might be raised randomly later at runtime.

        Activity

        Hide
        Tijs Rademakers added a comment -

        Do you have any test case to show this?

        Show
        Tijs Rademakers added a comment - Do you have any test case to show this?
        Hide
        Peter Horvath added a comment - - edited

        It is quite difficult to create a test case that consistently reproduces this issue. The references made to this class should confirm that multiple threads can access the HashMap - ClassNameUtils is used from the following classes (as of Activiti 5.8):

        org.activiti.engine.impl.db.DbSqlSession.toString(PersistentObject)
        org.activiti.engine.impl.db.DbSqlSessionFactory.getStatement(Class<?>, Map<Class<?>, String>, String)
        org.activiti.engine.impl.db.DbSqlSession.DeleteById.execute()
        org.activiti.engine.impl.db.DbSqlSession.DeleteById.toString()
        org.activiti.engine.impl.interceptor.LogInterceptor.execute(Command<T>)

        The JavaDoc of java.util.HashMap indicates that concurrent access is illegal and non-thread safe:
        Note that this implementation is not synchronized. If multiple threads access a hash map concurrently, and at least one of the threads modifies the map structurally, it must be synchronized externally. (A structural modification is any operation that adds or deletes one or more mappings; merely changing the value associated with a key that an instance already contains is not a structural modification.)

        For example if one thread puts a new entry to the hashmap while it is being resized for a previous put operation, it can cause unexpected exception being throw, similar to the following ones (example, not actual Activiti exception).

        java.util.ConcurrentModificationException: concurrent access to HashMap
        attempted by Thread[Worker-66,5,main]
        at java.util.HashMap.onEntry(HashMap.java:211)
        at java.util.HashMap.transfer(HashMap.java:554)
        at java.util.HashMap.resize(HashMap.java:541)
        at java.util.HashMap.addEntry(HashMap.java:901)
        at java.util.HashMap.put(HashMap.java:474)
        at java.util.HashSet.add(HashSet.java:206)

        or
        java.util.ConcurrentModificationException: concurrent access to HashMap attempted by Thread[WebContainer : 16,5,main]
        at java.util.HashMap.onExit(HashMap.java:225)
        at java.util.HashMap.transfer(HashMap.java:636)
        at java.util.HashMap.resize(HashMap.java:622)
        at java.util.HashMap.addEntry(HashMap.java:994)
        at java.util.HashMap.put(HashMap.java:510)

        The following hashmaps in DbSqlSessionFactory are used similarly, to provide a caching of statements - all of them are prepared for multi-threaded usage (properly synchronized)

        org.activiti.engine.impl.db.DbSqlSessionFactory.insertStatements
        org.activiti.engine.impl.db.DbSqlSessionFactory.updateStatements
        org.activiti.engine.impl.db.DbSqlSessionFactory.deleteStatements

        I suggest you replace java.util.HashMap with a java.util.concurrent.ConcurrentHashMap to avoid this issue.
        Using Collections.synchronizedMap here would be a performance killer and could easily become a bottleneck; I think that callling java.lang.Class.getSimpleName() every time would even sigificantly outperform a synchronized hashmap usage.

        Show
        Peter Horvath added a comment - - edited It is quite difficult to create a test case that consistently reproduces this issue. The references made to this class should confirm that multiple threads can access the HashMap - ClassNameUtils is used from the following classes (as of Activiti 5.8): org.activiti.engine.impl.db.DbSqlSession.toString(PersistentObject) org.activiti.engine.impl.db.DbSqlSessionFactory.getStatement(Class<?>, Map<Class<?>, String>, String) org.activiti.engine.impl.db.DbSqlSession.DeleteById.execute() org.activiti.engine.impl.db.DbSqlSession.DeleteById.toString() org.activiti.engine.impl.interceptor.LogInterceptor.execute(Command<T>) The JavaDoc of java.util.HashMap indicates that concurrent access is illegal and non-thread safe: Note that this implementation is not synchronized. If multiple threads access a hash map concurrently, and at least one of the threads modifies the map structurally, it must be synchronized externally. (A structural modification is any operation that adds or deletes one or more mappings; merely changing the value associated with a key that an instance already contains is not a structural modification.) For example if one thread puts a new entry to the hashmap while it is being resized for a previous put operation, it can cause unexpected exception being throw, similar to the following ones (example, not actual Activiti exception). java.util.ConcurrentModificationException: concurrent access to HashMap attempted by Thread [Worker-66,5,main] at java.util.HashMap.onEntry(HashMap.java:211) at java.util.HashMap.transfer(HashMap.java:554) at java.util.HashMap.resize(HashMap.java:541) at java.util.HashMap.addEntry(HashMap.java:901) at java.util.HashMap.put(HashMap.java:474) at java.util.HashSet.add(HashSet.java:206) or java.util.ConcurrentModificationException: concurrent access to HashMap attempted by Thread [WebContainer : 16,5,main] at java.util.HashMap.onExit(HashMap.java:225) at java.util.HashMap.transfer(HashMap.java:636) at java.util.HashMap.resize(HashMap.java:622) at java.util.HashMap.addEntry(HashMap.java:994) at java.util.HashMap.put(HashMap.java:510) The following hashmaps in DbSqlSessionFactory are used similarly, to provide a caching of statements - all of them are prepared for multi-threaded usage (properly synchronized) org.activiti.engine.impl.db.DbSqlSessionFactory.insertStatements org.activiti.engine.impl.db.DbSqlSessionFactory.updateStatements org.activiti.engine.impl.db.DbSqlSessionFactory.deleteStatements I suggest you replace java.util.HashMap with a java.util.concurrent.ConcurrentHashMap to avoid this issue. Using Collections.synchronizedMap here would be a performance killer and could easily become a bottleneck; I think that callling java.lang.Class.getSimpleName() every time would even sigificantly outperform a synchronized hashmap usage.
        Hide
        Frederik Heremans added a comment -

        Using concurrent map now to prevent this kind of issues. I get your point about using the getSimpleName() directly instead of using a cache (which uses the cached classname and substrings it every time). Since the simple-names are used a lot in logging and in handling persistence, we'll stick with the cache.

        Show
        Frederik Heremans added a comment - Using concurrent map now to prevent this kind of issues. I get your point about using the getSimpleName() directly instead of using a cache (which uses the cached classname and substrings it every time). Since the simple-names are used a lot in logging and in handling persistence, we'll stick with the cache.

          People

          • Assignee:
            Unassigned
            Reporter:
            Peter Horvath
          • Votes:
            2 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: