JBossAS-2238: BasicMBeanRegistry synchronization

[adrian.brock]

http://jira.jboss.com/jira/browse/JBAS-2238

I would guess that somebody has broken the synchronization. Although I can't
see an obvious recent change.

This has been confusing in this class, ever since the jmx-remoting team reworked the synchronization.
Sometimes it synchronizes on the BasicMBeanRegistry instance,
sometimes on DomainMap, sometimes on both.

I propose the best approach is to convert the domain map and its submaps
to use a ConcurrentReaderHashMap. This would remove most of the need for
synchronization, except for the add/remove critical sections.

[starksm64]

Agreed.

[clebert.suconic]

The current implementation of BasicMBeanRegistry is causing locking contention:

Example:

"http-172.16.133.197-8080-716" daemon prio=10 tid=0x01788e60 nid=0x4fe waiting for monitor entry [0x3e1fe000..0x3e1ffc90]
 at org.jboss.mx.server.registry.BasicMBeanRegistry.getMBeanMap(BasicMBeanRegistry.java:959)
 - waiting to lock <0x8f6867f0> (a EDU.oswego.cs.dl.util.concurrent.ConcurrentReaderHashMap)
 at org.jboss.mx.server.registry.BasicMBeanRegistry.get(BasicMBeanRegistry.java:518)
 at org.jboss.mx.server.MBeanServerImpl.invoke(MBeanServerImpl.java:653)
 at org.jboss.invocation.local.LocalInvoker$MBeanServerAction.invoke(LocalInvoker.java:169)
 at org.jboss.invocation.local.LocalInvoker.invoke(LocalInvoker.java:118)
 at org.jboss.invocation.InvokerInterceptor.invokeLocalMarshalled(InvokerInterceptor.java:296)
 at org.jboss.invocation.MarshallingInvokerInterceptor.invoke(MarshallingInvokerInterceptor.java:61)
 at org.jboss.proxy.TransactionInterceptor.invoke(TransactionInterceptor.java:61)
 at org.jboss.proxy.SecurityInterceptor.invoke(SecurityInterceptor.java:70)
 at org.jboss.proxy.ejb.StatefulSessionInterceptor.invoke(StatefulSessionInterceptor.java:121)
 at org.jboss.proxy.ClientContainer.invoke(ClientContainer.java:100)

I would like to change BasicMBeanRegistry to be:

 private Map getMBeanMap(String domain, boolean createIfMissing)
 {
 Map mbeanMap = (Map) domainMap.get(domain);
 if (mbeanMap == null && createIfMissing)
 {
 synchronized (domainMap)
 {
 mbeanMap = (Map) domainMap.get(domain);
 if (mbeanMap == null && createIfMissing)
 {
 mbeanMap = new ConcurrentReaderHashMap();
 domainMap.put(domain, mbeanMap);
 } // end of if ()
 }
 }
 return mbeanMap;
 }

If there is an issue with that change, what would be another suggestion for this?

[starksm64]

I don't see that the synchronization on the domainMap is needed at all. The add(MBeanEntry entry) and remove(ObjectName name) methods that modify the domainMap contents are already synchronized. The getMBeanMap comment needs to be updated and should state that calling this method with createIfMissing needs to be done from a synchronized method.

[clebert.suconic]

http://jira.jboss.com/jira/browse/JBAS-2585