Non-thread-safe usage of map can result in infinite loop and 100% CPU usage

[5inGularity]

From https://groups.google.com/forum/#!searchin/gwt-platform/spring/gwt-platform/vX2eKouqe74/dXDTZzuLB74J
Recently we faced high CPU utilization by tomcat process running a GWTP and Spring based web-application. The thread dump shows several threads executing which stack-traces similar to following -

"http-443-2" daemon prio=10 tid=0x09d47000 nid=0x159e runnable [0x8cfd3000]
java.lang.Thread.State: RUNNABLE
at java.util.HashMap.get(Unknown Source)
at com.gwtplatform.dispatch.server.spring.actionhandlervalidator.LazyActionHandlerValidatorRegistryImpl.findActionHandlerValidator(LazyActionHandlerValidatorRegistryImpl.java:68)
at com.gwtplatform.dispatch.server.AbstractDispatchImpl.findHandler(AbstractDispatchImpl.java:196)
at com.gwtplatform.dispatch.server.AbstractDispatchImpl.doExecute(AbstractDispatchImpl.java:147)
at com.gwtplatform.dispatch.server.AbstractDispatchImpl.execute(AbstractDispatchImpl.java:111)
at com.gwtplatform.dispatch.server.AbstractDispatchServiceImpl.execute(AbstractDispatchServiceImpl.java:81)

It seems that concurrent modifications to the maps used in this class may result in corrupted indexes [http://javaeesupportpatterns.blogspot.in/2012/02/hashmapget-high-cpu-case-study.html] in hashmap.

[christiangoudreau]

I think this also apply to the Guice implementation, could you update it as well?

[branflake2267]

Nice job.

[5inGularity]

We've been using locally patched spring module in our production code for last few months and have not encountered the issue. Although the guice module has not been tested with the changes, I guess the change is pretty straightforward to not cause any issues.

[5inGularity]

Sorry, I have not used git before. Am I supposed to keep this open?

[olafleur]

Yes, if you want the pull request to be considered to be added in the GWTP source code it should be kept open until someone who has the authorisation merge it into the source code.

[christiangoudreau]

You can keep it open and as soon as you will commit and push new changes, this PR will get updated

[christiangoudreau]

Hop merged, I didn't saw that you already had done the changes for guice as well

Thanks for the contribution!

[jDramaix]

using ConcurrentHashMap is not sufficient to be thread-Safe. we should use putIfAbsent() method (from ConcurrentHashMap) instead of put() in the following pattern :

Object value = map.get(key);

if (value == null) {
    value = createValueFor(key);
    map.put(key, value);
}

return value;

This pattern should be replaced by :

Object value = concurentHashMap.get(key);

if (value == null) {
    value = createValueFor(key);
    return concurentHashMap.putIfAbsent(key, value);
}

return value;

[5inGularity]

Changing map.put to map.putIfAbsent changes the behavior a bit. When called with different values for the same key, changed version will not replace the value while the unchanged would.
I am not sure if this will have any undesired impact on the functionality. I am not intimate with all the code flows here.

[christiangoudreau]

In this case, handlers are only registered once and shouldn't replaced in the map, else there's something that will end up double bound in Guice and Guice won't like it.

[jDramaix]

So I will create an issue to track this.