[clebert.suconic]

I would change it to be:

 public int hashCode() {
 long tmp = time + id;
 return (int)(tmp ^ (tmp >> 32));
 }

[adrian.brock]

That's a co-incidence :-)
http://jira.jboss.com/jira/browse/JBAS-2564

[clebert.suconic]

I implemented the hashCode on UID as I said before, and this locking issue was solved... but:

It looks like another locking issue appeared. It was like opening a pipe and another leak appeared.

We (I and Phillip Thurmond) made some tests and we found a constraint at BasicMBeanRegistry.

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

domainMap is a ConcurrentReaderHashMap. So, does this operation needs to be atomic?

At least, I think this code should 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;
 }

Also, could I change add and remove methods to be:

 protected void add(MBeanEntry entry)
 throws InstanceAlreadyExistsException
 {
 // Determine the MBean's name and properties
 ObjectName name = entry.getObjectName();
 String domain = name.getDomain();
 String props = name.getCanonicalKeyPropertyListString();

 // Create a properties -> entry map if we don't have one
 Map mbeanMap = getMBeanMap(domain, true);

 synchronized (mbeanMap)
 {
 // Make sure we aren't already registered
 if (mbeanMap.get(props) != null)
 throw new InstanceAlreadyExistsException(name + " already registered.");

 // Ok, we are registered
 mbeanMap.put(props, entry);
 }
 }

 /**
 * Removes an MBean entry
 *
 * WARNING: The object name should be fully qualified.
 *
 * @param name the object name of the entry to remove
 * @exception InstanceNotFoundException when the object name is not
 * registered
 */
 protected void remove(ObjectName name)
 throws InstanceNotFoundException
 {
 // Determine the MBean's name and properties
 String domain = name.getDomain();
 String props = name.getCanonicalKeyPropertyListString();
 Map mbeanMap = getMBeanMap(domain, false);

 synchronized (mbeanMap)
 {
 // Remove the entry, raise an exception when it didn't exist
 if (null == mbeanMap || null == mbeanMap.remove(props))
 throw new InstanceNotFoundException(name + " not egistered.");
 }
 }

Notice that I'm delegating the locking to mbeanMap

[adrian.brock]

Yes these need to be atomic for the add/remove.

There is no need for atomicity in the other operations.

Whay you need to do is remove the synchronization from everything but
the add/remove whch need to be relatively atomic operations.

Map mbeanMap = (Map) domainMap.get(domain);
 if (mBeanMap==null && createIfMissing)
 {
 synchronized (domainMap)
 {
 mbeanMap = (Map) domainMap.get(domain);

This code is the bad form of the double check!

[adrian.brock]

This code was changed recently:
http://jira.jboss.com/jira/browse/JBAS-2238
discussion thread:
http://www.jboss.org/index.html?module=bb&op=viewtopic&t=69097
where I suggested that it uses concurrent hashmaps and just leave a couple
of synchronization blocks in add/remove to avoid the race between
register/unregisterMBean.