[jboss-cvs] JBossAS SVN: r78604 - projects/aop/trunk/aop/src/main/org/jboss/aop.

[jboss-cvs-commits at lists.jboss.org]

Author: flavia.rainone at jboss.com
Date: 2008-09-16 20:57:41 -0400 (Tue, 16 Sep 2008)
New Revision: 78604

Modified:
   projects/aop/trunk/aop/src/main/org/jboss/aop/AspectManager.java
Log:
[JBAOP-629] A write lock at the beginning and end of AspectManager.removeBinding(String) method was missing.
The lock was moved from internalRemoveBinding, which is used internally by both addBinding and
removeBinding methods.
Both are now acquiring the write lock, so there is no need to have the lock in internalRemoveBinding.
This change in the code resulted in a couple of deadlocks... which was fixed by synchronizing other
methods using the same approach used in addBinding and removeBinding methods.

Modified: projects/aop/trunk/aop/src/main/org/jboss/aop/AspectManager.java
===================================================================
--- projects/aop/trunk/aop/src/main/org/jboss/aop/AspectManager.java	2008-09-17 00:36:25 UTC (rev 78603)
+++ projects/aop/trunk/aop/src/main/org/jboss/aop/AspectManager.java	2008-09-17 00:57:41 UTC (rev 78604)
@@ -707,18 +707,30 @@
 
    public synchronized void initialiseClassAdvisor(Class<?> clazz, ClassAdvisor advisor)
    {
-      synchronized (advisors)
+      // avoiding deadlock. Other threads first get the bindignCollection lock
+      // and then the advisors
+      // as we know that the bindingCollection lock will be needed during the 
+      // Advisor.attachClass method execution, we get the lock at this point
+      // making sure we are avoiding the deadlock.
+      bindingCollection.lockRead();
+      try
       {
-         advisors.put(clazz, new WeakReference<Advisor>(advisor));
+         synchronized (advisors)
+         {
+            advisors.put(clazz, new WeakReference<Advisor>(advisor));
+            registerClass(clazz);
+            advisor.attachClass(clazz);
+            InterceptorChainObserver observer = dynamicStrategy.getInterceptorChainObserver(clazz);
+            advisor.setInterceptorChainObserver(observer);
+            if (notificationHandler != null)
+            {
+               notificationHandler.attachClass(clazz.getName());
+            }
+         }
       }
-
-      registerClass(clazz);
-      advisor.attachClass(clazz);
-      InterceptorChainObserver observer = dynamicStrategy.getInterceptorChainObserver(clazz);
-      advisor.setInterceptorChainObserver(observer);
-      if (notificationHandler != null)
+      finally
       {
-         notificationHandler.attachClass(clazz.getName());
+         bindingCollection.unlockRead(false);
       }
    }
 
@@ -1330,12 +1342,20 @@
     */
    public synchronized void removeBinding(String name)
    {
-      AdviceBinding binding = internalRemoveBinding(name);
-      if (binding != null)
+      bindingCollection.lockWrite();
+      try
       {
-         binding.clearAdvisors();
-         dynamicStrategy.interceptorChainsUpdated();
+         AdviceBinding binding = internalRemoveBinding(name);
+         if (binding != null)
+         {
+            binding.clearAdvisors();
+            dynamicStrategy.interceptorChainsUpdated();
+         }
       }
+      finally
+      {
+         bindingCollection.unlockWrite();
+      }
    }
 
    public synchronized void removeBindings(ArrayList<String> binds)
@@ -2039,22 +2059,14 @@
     */
    private synchronized AdviceBinding internalRemoveBinding(String name)
    {
-      bindingCollection.lockWrite();
-      try
+      AdviceBinding binding = bindingCollection.removeBinding(name);
+      if (binding == null)
       {
-         AdviceBinding binding = bindingCollection.removeBinding(name);
-         if (binding == null)
-         {
-            return null;
-         }
-         Pointcut pointcut = binding.getPointcut();
-         this.removePointcut(pointcut.getName());
-         return binding;
+         return null;
       }
-      finally
-      {
-         bindingCollection.unlockWrite();
-      }
+      Pointcut pointcut = binding.getPointcut();
+      this.removePointcut(pointcut.getName());
+      return binding;
    }
 
 //   public void setBindings(LinkedHashMap bindings)