-
1. Re: Synchronization on StatefulContainer
clebert.suconic Jun 20, 2006 4:56 PM (in response to 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. -
2. Re: Synchronization on StatefulContainer
clebert.suconic Jun 20, 2006 5:10 PM (in response to 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 -
3. Re: Synchronization on StatefulContainer
clebert.suconic Jun 20, 2006 5:33 PM (in response to 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? -
4. Re: Synchronization on StatefulContainer
michael.yuan Jun 20, 2006 5:49 PM (in response to clebert.suconic)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. -
5. Re: Synchronization on StatefulContainer
clebert.suconic Jun 20, 2006 6:15 PM (in response to clebert.suconic)Just a FYI: I have renamed the subject of this thread.
I had an incomplete name before due to some typo. -
6. Re: Synchronization on StatefulContainer
starksm64 Jun 20, 2006 6:48 PM (in response to clebert.suconic)"clebert.suconic@jboss.com" wrote:
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. -
7. Re: Synchronization on StatefulContainer
clebert.suconic Jun 20, 2006 6:53 PM (in response to 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). -
8. Re: Synchronization on StatefulContainer
bill.burke Jun 20, 2006 8:24 PM (in response to clebert.suconic)Yikes! This seems to be used for the getInvokedBusinessInterface and getBusinessObject invocation. Horrible implementation. I'll fix it.
-
9. Re: Synchronization on StatefulContainer
bill.burke Jun 20, 2006 8:32 PM (in response to clebert.suconic)getBusinessObject isn't even implemented correctly.
-
10. Re: Synchronization on StatefulContainer
bill.burke Jun 20, 2006 8:40 PM (in response to clebert.suconic)I'll fix this as soon as I get the Tomcat6 changes in.
-
11. Re: Synchronization on StatefulContainer
michael.yuan Jun 22, 2006 1:09 AM (in response to clebert.suconic)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! -
12. Re: Synchronization on StatefulContainer
bill.burke Jun 22, 2006 7:55 PM (in response to clebert.suconic)Ok, this should be fixed
-
13. Re: Synchronization on StatefulContainer
bdecoste Jun 22, 2006 9:27 PM (in response to clebert.suconic)My brain fart ... Burke fixed this in head and I fixed 4.0.x by removing the implementation of getInvokedBusinessInterface() and getBusinessObject().