Bug 329488 - DSF ACPM deadlocks

3 files changed, 70 insertions, 24 deletions

diff --git a/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/AbstractCache.java b/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/AbstractCache.java
index e4ad222a077..1f22902065a 100644
--- a/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/AbstractCache.java
+++ b/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/AbstractCache.java
@@ -195,45 +195,67 @@ public abstract class AbstractCache<V> implements ICache<V> {
         
     }
 
+    /**
+     * Returns true if there are no clients waiting for this cache or if the 
+     * clients that are waiting, have already canceled their requests.  
+     * <p>
+     * Note: Calling this method may cause the client request monitors that were
+     * canceled to be completed with a cancel status.  If all the client request
+     * monitors were canceled, this method will also cause the {@link #canceled()} 
+     * method to be called.  Both of these side effects will only happen 
+     * asynchronously after <code>isCanceled()</code> returns.
+     * </p> 
+     *  
+     * @return <code>true</code> if all clients waiting on this cache have been 
+     * canceled, or if there are no clients waiting at all. 
+     */    
     @ThreadSafe
     protected boolean isCanceled() {
         boolean canceled;
         List<RequestMonitor> canceledRms = null;
         synchronized (this) {
-            if (fWaitingList instanceof RequestMonitor && ((RequestMonitor)fWaitingList).isCanceled()) {
-                canceledRms = new ArrayList<RequestMonitor>(1);
-                canceledRms.add((RequestMonitor)fWaitingList); 
-                fWaitingList = null;
+            if (fWaitingList instanceof RequestMonitor) {
+                if ( ((RequestMonitor)fWaitingList).isCanceled() ) {
+                    canceledRms = new ArrayList<RequestMonitor>(1);
+                    canceledRms.add((RequestMonitor)fWaitingList); 
+                    canceled = true;
+                } else {
+                    canceled = false;
+                }
             } else if(fWaitingList instanceof RequestMonitor[]) {
-                boolean waiting = false;
+                canceled = true;
                 RequestMonitor[] waitingList = (RequestMonitor[])fWaitingList;
                 for (int i = 0; i < waitingList.length; i++) {
-                    if (waitingList[i] != null && waitingList[i].isCanceled()) {
-                        if (canceledRms == null) {
-                            canceledRms = new ArrayList<RequestMonitor>(1);
+                    if (waitingList[i] != null) {
+                        if (waitingList[i].isCanceled()) {
+                            if (canceledRms == null) {
+                                canceledRms = new ArrayList<RequestMonitor>(1);
+                            }
+                            canceledRms.add( waitingList[i] );
+                        } else {
+                            canceled = false;
                         }
-                        canceledRms.add( waitingList[i] );
-                        waitingList[i] = null;
                     }
-                    waiting = waiting || waitingList[i] != null;
-                }
-                if (!waiting) {
-                    fWaitingList = null;
                 }
-            }            
-            canceled = fWaitingList == null;
+            } else {
+                assert fWaitingList == null;
+                canceled = true;
+            }
         }
         if (canceledRms != null) {
-            for (RequestMonitor canceledRm : canceledRms) {
-                canceledRm.setStatus(Status.CANCEL_STATUS);
-                canceledRm.removeCancelListener(fRequestCanceledListener);
-                canceledRm.done();
-            }
+            final List<RequestMonitor> _canceledRms = canceledRms;
+            fExecutor.getDsfExecutor().execute(new DsfRunnable() {
+                public void run() {
+                    for (RequestMonitor canceledRm : _canceledRms) {
+                        handleCanceledRm(canceledRm);
+                    }
+                }
+            });
         }
         
         return canceled;
     }
-
+    
 	/**
 	 * Resets the cache, setting the data to null and the status to
 	 * INVALID_STATUS. When in the invalid state, neither the data nor the
diff --git a/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/RequestMonitor.java b/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/RequestMonitor.java
index 243713a074d..09f876934bf 100644
--- a/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/RequestMonitor.java
+++ b/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/RequestMonitor.java
@@ -233,8 +233,15 @@ public class RequestMonitor extends DsfExecutable {
      * A request monitor is considered canceled if either it or its parent was canceled.
      * </p> 
      */
-    public synchronized boolean isCanceled() { 
-        return fCanceled || (fParentRequestMonitor != null && fParentRequestMonitor.isCanceled());
+    public boolean isCanceled() {
+        boolean canceled = false;
+        
+        // Avoid holding onto this lock while calling parent RM, which may 
+        // acquire other locks (bug 329488).
+        synchronized(this) {
+            canceled = fCanceled;
+        }
+        return canceled || (fParentRequestMonitor != null && fParentRequestMonitor.isCanceled());
     }
     
     /**
diff --git a/dsf/org.eclipse.cdt.tests.dsf/src/org/eclipse/cdt/tests/dsf/concurrent/CacheTests.java b/dsf/org.eclipse.cdt.tests.dsf/src/org/eclipse/cdt/tests/dsf/concurrent/CacheTests.java
index c9cd8021ab8..b7092fbbabd 100644
--- a/dsf/org.eclipse.cdt.tests.dsf/src/org/eclipse/cdt/tests/dsf/concurrent/CacheTests.java
+++ b/dsf/org.eclipse.cdt.tests.dsf/src/org/eclipse/cdt/tests/dsf/concurrent/CacheTests.java
@@ -521,6 +521,14 @@ public class CacheTests {
 
 	@Test 
     public void cancelWhilePendingWithoutClientNotificationTest() throws InterruptedException, ExecutionException {
+	    final boolean canceledCalled[] = new boolean[] { false };
+	    
+	    fTestCache = new TestCache() {
+	        protected synchronized void canceled() {
+	            canceledCalled[0] = true;
+	        };
+	    };
+	    
         // Request data from cache 
         Query<Integer> q = new Query<Integer>() { 
             @Override
@@ -550,12 +558,21 @@ public class CacheTests {
         q.cancel(true);
         
         assertCacheInvalidAndWithCanceledRM();
+        
+        // AbstractCache.canceled() should be called after isCanceled() 
+        // discovers that the client has canceled its request.  The canceled() method is 
+        // called in a separate dispatch cycle, so we have to wait one cycle of the executor 
+        // after is canceled is called.
+        fRetrieveRm.isCanceled();
+        fExecutor.submit(new Runnable() { public void run() {} }).get(); 
+        Assert.assertTrue(canceledCalled[0]);        
 
         try {
             q.get();
             Assert.fail("Expected a cancellation exception");
         } catch (CancellationException e) {} // Expected exception;
 
+
         // Completed the retrieve RM
         fExecutor.submit(new DsfRunnable() {
             public void run() {