[
https://issues.apache.org/jira/browse/OPENEJB-1880?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13425603#comment-13425603
]
Romain Manni-Bucau commented on OPENEJB-1880:
---------------------------------------------
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.
> atomicity violation bugs because of misusing concurrent collections
> -------------------------------------------------------------------
>
> Key: OPENEJB-1880
> URL: https://issues.apache.org/jira/browse/OPENEJB-1880
> Project: OpenEJB
> Issue Type: Bug
> Affects Versions: 4.0.0
> Reporter: Yu Lin
> Labels: patch
> Attachments: openejb-4.0.0.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
