You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@mina.apache.org by "David M. Lloyd (JIRA)" <ji...@apache.org> on 2007/12/14 02:54:43 UTC

[jira] Created: (DIRMINA-497) Thread safety issue: incorrect usage of Collections.synchronizedMap in DefaultIoSessionDataStructureFactory.DefaultIoSessionAttributeMap

Thread safety issue: incorrect usage of Collections.synchronizedMap in DefaultIoSessionDataStructureFactory.DefaultIoSessionAttributeMap
----------------------------------------------------------------------------------------------------------------------------------------

                 Key: DIRMINA-497
                 URL: https://issues.apache.org/jira/browse/DIRMINA-497
             Project: MINA
          Issue Type: Bug
          Components: Core
            Reporter: David M. Lloyd


In trunk, the DefaultIoSessionAttributeMap.setAttributeIfAbsent() synchronizes on the attributes map in order to synchronize access with the other get/setAttribute methods. However, Collections.synchronizedMap does NOT synchronize on the map itself, but rather an internal object!  So, setAttributeIfAbsent() still contains a race condition.

A good solution for 2.x would be to simply use a ConcurrentMap, which has putIfAbsent().  For 1.0.x, which has to run on JDK 1.4 (correct?), I'd recommend to just drop Collections.synchronizedMap() and synchronize directly on the Map reference itself.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

[jira] Commented: (DIRMINA-497) Thread safety issue: incorrect usage of Collections.synchronizedMap in DefaultIoSessionDataStructureFactory.DefaultIoSessionAttributeMap

Posted by "Trustin Lee (JIRA)" <ji...@apache.org>.

    [ https://issues.apache.org/jira/browse/DIRMINA-497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12552321 ] 

Trustin Lee commented on DIRMINA-497:
-------------------------------------

Are you sure that the Map implementation that Collections.synchronizedMap() returns uses the internal object for synchronization?  I looked into the JavaDoc and JDK source code, but it seems like it synchronizes with the returned Map instance itself.  Could you check that again?

BTW, we have reverted back from ConcurrentHashMap to synchronized HashMap due to excessive memory usage of ConcurrentHashMap.  It would be really nice if there's a compact ConcurrentHashMap implementation that doesn't consume too much heap.

> Thread safety issue: incorrect usage of Collections.synchronizedMap in DefaultIoSessionDataStructureFactory.DefaultIoSessionAttributeMap
> ----------------------------------------------------------------------------------------------------------------------------------------
>
>                 Key: DIRMINA-497
>                 URL: https://issues.apache.org/jira/browse/DIRMINA-497
>             Project: MINA
>          Issue Type: Bug
>          Components: Core
>            Reporter: David M. Lloyd
>         Attachments: DIRMINA-497-1.patch
>
>
> In trunk, the DefaultIoSessionAttributeMap.setAttributeIfAbsent() synchronizes on the attributes map in order to synchronize access with the other get/setAttribute methods. However, Collections.synchronizedMap does NOT synchronize on the map itself, but rather an internal object!  So, setAttributeIfAbsent() still contains a race condition.
> A good solution for 2.x would be to simply use a ConcurrentMap, which has putIfAbsent().  For 1.0.x, which has to run on JDK 1.4 (correct?), I'd recommend to just drop Collections.synchronizedMap() and synchronize directly on the Map reference itself.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

[jira] Updated: (DIRMINA-497) Thread safety issue: incorrect usage of Collections.synchronizedMap in DefaultIoSessionDataStructureFactory.DefaultIoSessionAttributeMap

Posted by "David M. Lloyd (JIRA)" <ji...@apache.org>.

     [ https://issues.apache.org/jira/browse/DIRMINA-497?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

David M. Lloyd updated DIRMINA-497:
-----------------------------------

    Attachment: DIRMINA-497-1.patch

A patch that uses regular synchronization instead of Collections.synchronizedMap().

> Thread safety issue: incorrect usage of Collections.synchronizedMap in DefaultIoSessionDataStructureFactory.DefaultIoSessionAttributeMap
> ----------------------------------------------------------------------------------------------------------------------------------------
>
>                 Key: DIRMINA-497
>                 URL: https://issues.apache.org/jira/browse/DIRMINA-497
>             Project: MINA
>          Issue Type: Bug
>          Components: Core
>            Reporter: David M. Lloyd
>         Attachments: DIRMINA-497-1.patch
>
>
> In trunk, the DefaultIoSessionAttributeMap.setAttributeIfAbsent() synchronizes on the attributes map in order to synchronize access with the other get/setAttribute methods. However, Collections.synchronizedMap does NOT synchronize on the map itself, but rather an internal object!  So, setAttributeIfAbsent() still contains a race condition.
> A good solution for 2.x would be to simply use a ConcurrentMap, which has putIfAbsent().  For 1.0.x, which has to run on JDK 1.4 (correct?), I'd recommend to just drop Collections.synchronizedMap() and synchronize directly on the Map reference itself.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

[jira] Closed: (DIRMINA-497) Thread safety issue: incorrect usage of Collections.synchronizedMap in DefaultIoSessionDataStructureFactory.DefaultIoSessionAttributeMap

Posted by "David M. Lloyd (JIRA)" <ji...@apache.org>.

     [ https://issues.apache.org/jira/browse/DIRMINA-497?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

David M. Lloyd closed DIRMINA-497.
----------------------------------

    Resolution: Invalid

Ah yes, you are correct - I looked a while back and noticed that there is a separate "mutex" object in the SynchronizedMap implementation, but I never noticed that it was initialized to the map itself...

Never mind then ;-)

> Thread safety issue: incorrect usage of Collections.synchronizedMap in DefaultIoSessionDataStructureFactory.DefaultIoSessionAttributeMap
> ----------------------------------------------------------------------------------------------------------------------------------------
>
>                 Key: DIRMINA-497
>                 URL: https://issues.apache.org/jira/browse/DIRMINA-497
>             Project: MINA
>          Issue Type: Bug
>          Components: Core
>            Reporter: David M. Lloyd
>         Attachments: DIRMINA-497-1.patch
>
>
> In trunk, the DefaultIoSessionAttributeMap.setAttributeIfAbsent() synchronizes on the attributes map in order to synchronize access with the other get/setAttribute methods. However, Collections.synchronizedMap does NOT synchronize on the map itself, but rather an internal object!  So, setAttributeIfAbsent() still contains a race condition.
> A good solution for 2.x would be to simply use a ConcurrentMap, which has putIfAbsent().  For 1.0.x, which has to run on JDK 1.4 (correct?), I'd recommend to just drop Collections.synchronizedMap() and synchronize directly on the Map reference itself.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.