1 2 Previous Next 19 Replies Latest reply on Dec 16, 2005 1:07 PM by adrian.brock Go to original post
      • 15. Re:  Lock Contention on org.jboss.ejb.BeanLockManager
        clebert.suconic

        I would change it to be:

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


        • 16. Re:  Lock Contention on org.jboss.ejb.BeanLockManager
          • 17. Re:  Lock Contention on org.jboss.ejb.BeanLockManager and Ba
            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

            • 18. Re:  Lock Contention on org.jboss.ejb.BeanLockManager

              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!

              • 19. Re:  Lock Contention on org.jboss.ejb.BeanLockManager

                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.

                1 2 Previous Next