Uploaded image for project: 'MINA'
  1. MINA
  2. DIRMINA-897

atomicity violation bugs because of misusing concurrent collections

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.4
    • Fix Version/s: 2.0.5
    • 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 Mina
      2.0.4, which may result in potential atomicity violation bugs or harm
      the performance.

      The code below is a snapshot of the code in file
      mina-core/src/main/java/org/apache/mina/core/polling/AbstractPollingIoProcessor.java
      from line 156 to 170

      L156 synchronized (threadIds) {
      L157 // Get the current ID associated to this class' name
      L158 AtomicInteger threadId = threadIds.get(cls);
      L159
      L160 if (threadId == null)

      { ... L165 threadIds.put(cls, new AtomicInteger(newThreadId)); L166 }

      else

      { ... L169 }

      L170 }

      In the code above, we may eliminate the synchronization on "threadIds"
      object (line 156) by using putIfAbsent(k, v) method provided by
      ConcurrentHashMap at line 165, which will improve the performance.

      If we remove the the synchronized keyword without using putIfAbsent,
      an atomicity violation may occur between lines <160 and 165>: Suppose
      thread T1 executes line 158 and finds that the concurrent hashmap
      "threadIds" doesn't contain the key "threadIds". Before thread T1
      executes line 165, another thread T2 puts a pair <cls, v> in the
      concurrent hashmap "threadIds". 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. However, by using
      putIfAbsent, this atomicity violation can be avoided, since if T2
      already puts a pair, T1 won't put again.

      Similar to the above code, in
      mina-core/src/main/java/org/apache/mina/core/session/DefaultIoSessionDataStructureFactory.java,
      we may eliminate the synchronization on "attributes" object (line 103,
      128, 144) by using putIfAbsent(k, v), remove(k, v) and replace(k, v1,
      v2) method provided by the ConcurrentHashMap.

      I found some similar misusages in other files:

      In
      mina-core/src/main/java/org/apache/mina/filter/codec/demux/DemuxingProtocolEncoder.java,
      there may be an atomicity violation bug at line 207. If thread T2 put
      a pair <type, value> to map "state.findEncoderCache" between lines
      <160 and 207>, this value will be overwrite at line 207 by thread T1
      and never be read. Again, the code no longer preserves the
      "put-if-absent" semantics.

      In
      mina-core/src/main/java/org/apache/mina/filter/executor/DefaultIoEventSizeEstimator.java,
      there may be an atomicity violation bug at line 135. If another thread
      T2 puts a pair <clazz, value> to map "class2size" between lines <105 and
      135>, the invocation of putIfAbsent method at line 135 by thread T1
      won't put the value "answer" anymore. In this case, we should return
      the value put by T2 rather than the "answer" calculated by T1.

        Activity

        Hide
        jacklondongood Yu Lin added a comment -

        This is a patch that may fix the atomicity violation problem I mentioned.

        Show
        jacklondongood Yu Lin added a comment - This is a patch that may fix the atomicity violation problem I mentioned.
        Hide
        elecharny Emmanuel Lecharny added a comment -

        Thanks for the patch !

        This part of the code has been modified a while ago, and we replaced the HashMap with ConcurrentHashMap without checking the context they were used.

        I'll apply the patche.

        Show
        elecharny Emmanuel Lecharny added a comment - Thanks for the patch ! This part of the code has been modified a while ago, and we replaced the HashMap with ConcurrentHashMap without checking the context they were used. I'll apply the patche.
        Hide
        elecharny Emmanuel Lecharny added a comment -
        Show
        elecharny Emmanuel Lecharny added a comment - Patch applied : http://svn.apache.org/viewvc?rev=1359085&view=rev Many thanks !

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

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

                Development