Uploaded image for project: 'MyFaces Trinidad'
  1. MyFaces Trinidad
  2. TRINIDAD-2287

atomicity violation bugs of misusing concurrent collections

    Details

    • Type: Bug
    • Status: Patch Available
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: 2.0.1-core
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      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 Trinidad
      2.0.1, which may result in potential atomicity violation bugs or harm
      the performance.

      The code below is a snapshot of the code in file
      trinidad-api/src/main/java/org/apache/myfaces/trinidad/bean/PropertyKey.java
      from line 102 to 112

      L102 PropertyKey cachedKey = _sDefaultKeyCache.get(name);

      L104 if (cachedKey == null)
      L105

      { L106 cachedKey = new PropertyKey(name); L108 // we don't need putIfAbsent because we don't care about identity L109 _sDefaultKeyCache.put(name, cachedKey); L110 }

      L112 return cachedKey;

      In the code above, an atomicity violation may occur between line 105
      and 109. Suppose a thread T1 executes line 102 and finds out the
      concurrent hashmap does not contain the key "name". Before it gets to
      execute line 109, another thread T2 puts a pair <name, v> in the
      concurrent hashmap "_sDefaultKeyCache". 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.

      I found some similar atomicity violations in other 17 places (I don't
      list them one by one. Please see the patch).

      Note that in
      trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/image/cache/Cache.java,
      if we use "putIfAbsent" at line 89 and "replace" at line 115, we may
      remove the synchronized keyword at line 82. Similarly, in
      trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/renderkit/RenderKitBase.java,
      we may remove the synchronized by using "putIfAbsent" at line 192.

        Activity

        Hide
        jacklondongood Yu Lin added a comment -

        This is patch that fixes the described atomicity violation bugs.

        Show
        jacklondongood Yu Lin added a comment - This is patch that fixes the described atomicity violation bugs.
        Hide
        btsulliv Blake Sullivan added a comment -

        I don't understand why you think that no preserving put-if-absent semantics is important. The code says that it explicitly doesn't care about these semantics:

        // we don't need putIfAbsent because we don't care about identity
        _sDefaultKeyCache.put(name, cachedKey);

        Since we don't care about those semantics, put will always perform better than putIfAbsent

        Show
        btsulliv Blake Sullivan added a comment - I don't understand why you think that no preserving put-if-absent semantics is important. The code says that it explicitly doesn't care about these semantics: // we don't need putIfAbsent because we don't care about identity _sDefaultKeyCache.put(name, cachedKey); Since we don't care about those semantics, put will always perform better than putIfAbsent
        Hide
        jacklondongood Yu Lin added a comment -

        Thanks for the confirmation. However, there are 17 such usages, and do you mean none of them care about put-if-absent semantics? (I'm not sure about this because I'm not an expert about this project).

        In Cache.java and RenderKitBase.java, there are synchronization which prevents concurrently write to the concurrent hash map. Can we remove those synchronizations by using "putIfAbsent" or "replace" operations?

        Show
        jacklondongood Yu Lin added a comment - Thanks for the confirmation. However, there are 17 such usages, and do you mean none of them care about put-if-absent semantics? (I'm not sure about this because I'm not an expert about this project). In Cache.java and RenderKitBase.java, there are synchronization which prevents concurrently write to the concurrent hash map. Can we remove those synchronizations by using "putIfAbsent" or "replace" operations?
        Hide
        btsulliv Blake Sullivan added a comment -

        I haven't looked at all of them yet. It is quite possible that some of these are over-synchronized.

        Show
        btsulliv Blake Sullivan added a comment - I haven't looked at all of them yet. It is quite possible that some of these are over-synchronized.

          People

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

            Dates

            • Created:
              Updated:

              Time Tracking

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

                Development