Uploaded image for project: 'OpenEJB'
  1. OpenEJB
  2. OPENEJB-1880

atomicity violation bugs because of misusing concurrent collections

    Details

    • Type: Bug
    • Status: Open
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: 4.0.0
    • Fix Version/s: None
    • Component/s: None
    • Labels:

      Description

      My name is Yu Lin. I'm a Ph.D. student in the CS department at
      UIUC. I'm currently doing research on mining Java concurrent library
      misusages. I found some misusages of ConcurrentHashMap in OpenEJB
      4.0.0, which may result in potential atomicity violation bugs or harm
      the performance.

      The code below is a snapshot of the code in file
      container/openejb-core/src/main/java/org/apache/openejb/core/managed/ManagedContainer.java
      from line 615 to 635

      L615 Instance instance = checkedOutInstances.get(primaryKey);
      L616 if (instance == null) {
      L617 try

      { L618 instance = cache.checkOut(primaryKey); L619 }


      ...

      L630 // remember instance until it is returned to the cache
      L631 checkedOutInstances.put(primaryKey, instance);
      L632 }

      L635 synchronized (instance) {

      In the code above, suppose a thread T1 executes line 615 and finds out
      the concurrent hashmap "checkedOutInstances" does not contain the key
      "primaryKey". Before it gets to execute line 631, another thread T2
      puts a pair <primaryKey, v> in the concurrent hashmap
      "checkedOutInstances". Now thread T1 resumes execution and it will
      overwrite the value written by thread T2. Thus, the code no longer
      preserves the "put-if-absent" semantics. Because "instance" object is
      used in the subsequent code, the code may use an "instance" that is
      not in the map, which is incorrect (I prepare a patch to fix the
      problem).

      I found some similar misusages in other files:

      In QueryProxy.java at line 203 and 366. In
      SocketConnectionFactory.java at line 166. In Client.java at line
      423. In Tracker.java at line 148. In StatefulContainer.java at line
      691, can we remove the "synchronized" at line 674 if we use
      "putIfAbsent" method at line 691? In StatsInterceptor.java at line
      197, if we use "putIfAbsent" method, can we remove the synchronization
      at line 193? In DynamicMBeanHandler.java at line 149 if using
      "putIfAbsent", the synchronization may also be removed. In
      DelegatePermissionCollection.java at line 81 and 97, we may also
      remove the synchronization if we use "putIfAbsent" (but we should use
      CopyOnWriteArrayList for list "permissions" since it will be accessed
      concurrently).

      There is another kind of misusage. Here is a piece of code in file
      examples/dynamic-datasource-routing/src/main/java/org/superbiz/dynamicdatasourcerouting/DeterminedRouter.java
      from line 90 to 94:

      L90 if (!dataSources.containsKey(datasourceName))

      { L91 throw new IllegalArgumentException("data source called " + datasourceName + " can't be found."); L92 }

      L93 DataSource ds = dataSources.get(datasourceName);
      L94 currentDataSource.set(ds);

      In the code above, suppose a thread T1 executes line 90 and finds out
      the concurrent hashmap "dataSources" contains the key
      "datasourceName". Before it gets to execute line 93, another thread T2
      removes the key "datasourceName" from the concurrent hashmap
      "dataSources". Now thread T1 resumes execution and it will
      get a null value, which is incorrect.

      Similar bugs can be found in DynamicDataSourceTest.java at line 283,
      QueryProxy.java at line 353 and 200, DelegatePermissionCollection.java
      at line 76.

        Activity

        Hide
        jacklondongood Yu Lin added a comment -

        The patch that may solve the problem.

        Show
        jacklondongood Yu Lin added a comment - The patch that may solve the problem.
        Hide
        romain.manni-bucau Romain Manni-Bucau added a comment -

        Hi,

        first thanks to have a look to it.

        Some note about the issue:
        1) about test: they simply show how to use the API, the impl is a detail so i'm not sure it is important (moreover some call to put are single threaded and the concurrenthashmap can be used for some other reasons.
        2) in some usage we don't care about putting twice the same value and it is often faster after than using any synch solution
        3) about containers if the instance is not in the map it can globally still be used and cleared, that's not the perfect solution but the issue is still here with your patch since you got 2 instances and manage a single one. Probably some more work is needed but the performance contraint is very important here if you want to fix it. Another note: for some container we'll do the put in createEJBObject and not in obtainInstance method where the put is simply a fallback so no issue.

        Globally the issue is it is hard to get a unit test reproducing the issue you described.

        Another note regarding your description: we don't always use a CHM for the putIfAbsent semantic.

        Show
        romain.manni-bucau Romain Manni-Bucau added a comment - Hi, first thanks to have a look to it. Some note about the issue: 1) about test: they simply show how to use the API, the impl is a detail so i'm not sure it is important (moreover some call to put are single threaded and the concurrenthashmap can be used for some other reasons. 2) in some usage we don't care about putting twice the same value and it is often faster after than using any synch solution 3) about containers if the instance is not in the map it can globally still be used and cleared, that's not the perfect solution but the issue is still here with your patch since you got 2 instances and manage a single one. Probably some more work is needed but the performance contraint is very important here if you want to fix it. Another note: for some container we'll do the put in createEJBObject and not in obtainInstance method where the put is simply a fallback so no issue. Globally the issue is it is hard to get a unit test reproducing the issue you described. Another note regarding your description: we don't always use a CHM for the putIfAbsent semantic.

          People

          • Assignee:
            Unassigned
            Reporter:
            jacklondongood Yu Lin
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development