Synchronization on StatefulContainer

[clebert.suconic]

Michael Yuan showed me a stack trace of some performance tests he is doing with EJB3 and Seam.

After the test is finished, it looks like there is an infinite loop at this point:

Several threads hanging on this point:

 at java.util.HashMap.put(HashMap.java:385)
 at org.jboss.ejb3.stateful.StatefulContainer.localInvoke
(StatefulContainer.java:178)
 at org.jboss.ejb3.stateful.StatefulLocalProxy.invoke
(StatefulLocalProxy.java:98)
 at $Proxy71.getSize(Unknown Source)
 at sun.reflect.GeneratedMethodAccessor191.invoke(Unknown
Source)
 at sun.reflect.DelegatingMethodAccessorImpl.invoke
(DelegatingMethodAccessorImpl.java:25)
 at java.lang.reflect.Method.invoke(Method.java:585)
 at org.apache.myfaces.el.PropertyResolverImpl.getProperty
(PropertyResolverImpl.java:400)
 at org.apache.myfaces.el.PropertyResolverImpl.getValue
(PropertyResolverImpl.java:71)
 at org.apache.myfaces.el.ELParserHelper
$MyPropertySuffix.evaluate(ELParserHelper.java:532)

Looking at StatefulContainer, I can see two possible mistakes:

 if (info.unadvisedMethod != null)
 {
 invokedMethod.put(id, info.unadvisedMethod);
 }

I- This is leaking. unadvisedMethos is always != null, hence invokedMethod is always getting a new put

II - This is non sycnrhonized, so I guess it makes sense the infinite loop.

[clebert.suconic]

Of course the leakage is not because of the HashMap.put. HashMaps won't accept dupplications and the value would be replaced.

But since the hashmap is not synchronized, this is generating some error.

[clebert.suconic]

I'm not really sure about the use of invokedMethod on this interceptor, but it looks like caching for methods and hashes could be improved here.

Also, I know Scott condemned double checks in some wiki, but would this be a legal fix on that case? I'm just trying to avoid synchronization here on the read only operation and only do on the write.

Class: org.jboss.ejb3.stateful.StatefulContainer
 public Object localInvoke(Object id, Method method, Object[] args,
 FutureHolder provider) throws Throwable
 {
 ClassLoader oldLoader = Thread.currentThread().getContextClassLoader();
 ThreadLocalENCFactory.push(enc);
 try
 {
 long hash = MethodHashing.calculateHash(method);
 MethodInfo info = (MethodInfo) methodInterceptors.get(hash);
 if (info == null)
 {
 throw new RuntimeException(
 "Could not resolve beanClass method from proxy call: "
 + method.toString());
 }

 Method unadvisedMethod = info.getUnadvisedMethod();
 if (unadvisedMethod != null)
 invokedMethod.put(this, unadvisedMethod);

 if (unadvisedMethod != null && isHomeMethod(unadvisedMethod))
 {
 return invokeLocalHomeMethod(info, args);
 }
 else if (unadvisedMethod != null
 && isEJBObjectMethod(unadvisedMethod))
 {
 return invokeEJBLocalObjectMethod(id, info, args);
 }

 if (unadvisedMethod != null && invokedMethod.get(id) == null) // changed this line after original post. Had a wrong clause here
 {
 synchronized (invokedMethod)
 {
 if (invokedMethod.get(id)==null)
 {
 invokedMethod.put(id, unadvisedMethod);
 }
 }
 }

 Interceptor[] aspects = info.getInterceptors();
 StatefulContainerInvocation nextInvocation = new StatefulContainerInvocation(
 info, aspects, id);
 nextInvocation.setAdvisor(this);
 nextInvocation.setArguments(args);

 ProxyUtils.addLocalAsynchronousInfo(nextInvocation, provider);
 return nextInvocation.invokeNext();
 }
 finally
 {
 Thread.currentThread().setContextClassLoader(oldLoader);
 ThreadLocalENCFactory.pop();
 }
 }

If this is not legal, we will have to use ConcurrentHashMap here

[clebert.suconic]

Can someone explain me why's that?

if (unadvisedMethod != null)
 invokedMethod.put(this, unadvisedMethod);

i didn't find any reference to invokedMethod.get(this). All the operations are using invokedMethod.get(id) (not this).

Or even better... why do we need invokedMethod at al?

[michael.yuan]

Just to provide some context here: I am seeing 100% CPU hang on multi-processor machines during stress tests. My single processor box seems to work fine. So, it is likely to be a sync issue. I need the fix to continue any meaningful stress testing with our partners at Dell. I am trying out Clebert's fix as I type.

Actually, there is a memory leak somewhere. The HashMap$Entry count in my JBoss profiler report goes up as times goes on. But each haspmap entry is only around 30 bytes. So, it would take a very long time before the leak can affact performance. But it is there nevertheless ... We need a tool like FindBugs to check for stuff like that.

[clebert.suconic]

Just a FYI: I have renamed the subject of this thread.
I had an incomplete name before due to some typo.

[starksm64]

Michael Yuan showed me a stack trace of some performance tests he is doing with EJB3 and Seam.

After the test is finished, it looks like there is an infinite loop at this point:

Several threads hanging on this point:

...

II - This is non sycnrhonized, so I guess it makes sense the infinite loop.

Unsychronized use of a map in multi-threaded code is a definite must not do because the implementation can get into an infinite loop. We have seen this several times now. Really, there should be no use of HashMap in any core code. Use the oswego concurrent hash map or the java.util.concurrent.ConcurrentHashMap if its java5 based code.

[clebert.suconic]

I have looked at the implementation.

I would need to know the intent of invokedMethod.

It seems to me that it's not even needed. I guess it would be possible to re-architect this in such way we don't even need the field.

I could easily provide a fix changing this to ConcurrentHashMap, but I guess we need Bill to validate if this is really needed.

BTW: by "I guess it makes sense the infinite loop" I meant, it makes sense the error is happening hence we need to add synchronization on that field. (or remove the field at once).

[bill.burke]

Yikes! This seems to be used for the getInvokedBusinessInterface and getBusinessObject invocation. Horrible implementation. I'll fix it.

[bill.burke]

getBusinessObject isn't even implemented correctly.

[bill.burke]

I'll fix this as soon as I get the Tomcat6 changes in.

[michael.yuan]

I created a JIRA issue here:

http://jira.jboss.com/jira/browse/EJBTHREE-634

I'd appreciate a quick fix since it is blocking my stress tests ... Thanks a lot!

[bill.burke]

Ok, this should be fixed

[bdecoste]

My brain fart ... Burke fixed this in head and I fixed 4.0.x by removing the implementation of getInvokedBusinessInterface() and getBusinessObject().