New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible misuses of concurrent collections in hazelcast #221

Closed
yulin2 opened this Issue Jul 31, 2012 · 2 comments

Comments

Projects
None yet
2 participants
@yulin2

yulin2 commented Jul 31, 2012

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

The code below is a snapshot of the code in file
hazelcast-client/src/main/java/com/hazelcast/client/HazelcastClient.java
from line 166 to 205

L166 public <K, V, E> Object getClientProxy(Object o) {
L167 Object proxy = mapProxies.get(o);
L168 if (proxy == null) {
L169 synchronized (mapProxies) {
L170 proxy = mapProxies.get(o);
L171 if (proxy == null) {
...
L200 mapProxies.put(o, proxy);
L201 }
L202 }
L203 }
L204 return mapProxies.get(o);
L205 }

In the code above, we may remove the "synchronized" keyword by using
"putIfAbsent" method provided by ConcurrentHashMap rather than "put"
at line 200. If we remove the synchronization without changing the
code, there will be an atomicity violation: suppose a thread T1
executes line 170 and finds out the concurrent hashmap "mapProxies"
does not contain the key "o". Before it gets to execute line 200,
another thread T2 puts a pair <o, v> in the map "mapProxies". 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, if we use "putIfAbsent", this semantics can be
preserved without synchronization. ("putIfAbsent" has better
performance than "synchronized"). Moreover, another problem is if T2
removes the key "o" before T1 executes line 204, the method will
return a null value. I attach a patch to fix this problem.

I found some other places that have "put-if-absent" semantics and
should use "putIfAbsent" method. I just simply list them:

hazelcast/src/main/java/com/hazelcast/config/Config.java at line
416 and 566. Note that I don't fix this file in the patch because it
seems that the map used here is not always concurrent hashmap, but
might be a regular hashmap. Thus, do we need synchronization at these
two places?

hazelcast/src/main/java/com/hazelcast/impl/BlockingQueueManager.java at line 669.

hazelcast/src/main/java/com/hazelcast/impl/ClientHandlerService.java at line 257.

hazelcast/src/main/java/com/hazelcast/impl/ListenerManager.java line
248: if we use "putIfAbsent", we can remove the "synchronized" at line
244.

hazelcast/src/main/java/com/hazelcast/impl/ExecutorManager.java line 392.

hazelcast/src/main/java/com/hazelcast/impl/ClientEndpoint.java line 330 and 332.

hazelcast/src/main/java/com/hazelcast/impl/ThreadContext.java line 57.

hazelcast/src/main/java/com/hazelcast/impl/ConcurrentMapManager.java line 2330

hazelcast/src/main/java/com/hazelcast/impl/wan/WanReplicationService.java
line 73. If we use "putIfAbsent" at line 73, can we remove the synchronization at line 45?

hazelcast/src/main/java/com/hazelcast/nio/Packet.java line 110.

hazelcast/src/main/java/com/hazelcast/nio/ConnectionManager.java line 218.

hazelcast/src/main/java/com/hazelcast/query/SortedIndexStore.java line 73.

hazelcast/src/main/java/com/hazelcast/query/MapIndexService.java line 138.

hazelcast/src/main/java/com/hazelcast/query/UnsortedIndexStore.java line 64.

hazelcast-client/src/main/java/com/hazelcast/client/ClientThreadContext.java line 40.

@yulin2

This comment has been minimized.

Show comment
Hide comment
@yulin2

yulin2 Jul 31, 2012

Since there is no way to attach my patch, I just show how I fix the example code I provided above:

public <K, V, E> Object getClientProxy(Object o) {
Object proxy = mapProxies.get(o);
if (proxy == null) {
...
Object tmpProxy = mapProxies.putIfAbsent(o, proxy);
if(tmpProxy != null)
proxy = tmpProxy;
}
return proxy;
}

You can find I remove the unnecessary synchronization by using "putIfAbsent".
Other fixes are similar.

yulin2 commented Jul 31, 2012

Since there is no way to attach my patch, I just show how I fix the example code I provided above:

public <K, V, E> Object getClientProxy(Object o) {
Object proxy = mapProxies.get(o);
if (proxy == null) {
...
Object tmpProxy = mapProxies.putIfAbsent(o, proxy);
if(tmpProxy != null)
proxy = tmpProxy;
}
return proxy;
}

You can find I remove the unnecessary synchronization by using "putIfAbsent".
Other fixes are similar.

@mdogan

This comment has been minimized.

Show comment
Hide comment
@mdogan

mdogan Aug 28, 2013

Member

Code base changed dramatically in version 3.0.

Member

mdogan commented Aug 28, 2013

Code base changed dramatically in version 3.0.

@mdogan mdogan closed this Aug 28, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment