Skip to main content
summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorJohn Cortell2010-11-10 11:47:55 -0500
committerJohn Cortell2010-11-10 11:47:55 -0500
commit5fa5ff3a8138a7f90645a606e35e7a6695fad5c6 (patch)
tree6cb941a3288f5bfc152e1af83526d0f32a0da28e
parent85b40d1d99aa62fde8355b2368bd1a5986c6c55c (diff)
downloadorg.eclipse.cdt-5fa5ff3a8138a7f90645a606e35e7a6695fad5c6.tar.gz
org.eclipse.cdt-5fa5ff3a8138a7f90645a606e35e7a6695fad5c6.tar.xz
org.eclipse.cdt-5fa5ff3a8138a7f90645a606e35e7a6695fad5c6.zip
Bug 329481: Inconsistency in DSF ACPM cancel behavior.
-rw-r--r--dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/AbstractCache.java2
-rw-r--r--dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/RequestCache.java22
-rw-r--r--dsf/org.eclipse.cdt.tests.dsf/src/org/eclipse/cdt/tests/dsf/concurrent/CacheTests.java114
3 files changed, 104 insertions, 34 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 e6985ea6bf..e4ad222a07 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
@@ -37,7 +37,7 @@ public abstract class AbstractCache<V> implements ICache<V> {
private class RequestCanceledListener implements RequestMonitor.ICanceledListener {
public void requestCanceled(final RequestMonitor canceledRm) {
- fExecutor.execute(new Runnable() {
+ fExecutor.getDsfExecutor().execute(new Runnable() {
public void run() {
handleCanceledRm(canceledRm);
}
diff --git a/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/RequestCache.java b/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/RequestCache.java
index dbf8f669b3..76b94d6e14 100644
--- a/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/RequestCache.java
+++ b/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/RequestCache.java
@@ -12,7 +12,6 @@ package org.eclipse.cdt.dsf.concurrent;
*******************************************************************************/
import org.eclipse.core.runtime.IStatus;
-import org.eclipse.core.runtime.Status;
/**
* A general purpose cache, which caches the result of a single request.
@@ -46,26 +45,25 @@ public abstract class RequestCache<V> extends AbstractCache<V> {
fRm = new DataRequestMonitor<V>(getImmediateInDsfExecutor(), null) {
- private IStatus fRawStatus = Status.OK_STATUS;
-
@Override
protected void handleCompleted() {
if (this == fRm) {
fRm = null;
- IStatus status;
- synchronized (this) {
- status = fRawStatus;
+ // If the requestor canceled the request, then leave the
+ // cache as is, regardless of how the retrieval completes.
+ // We want the cache to stay in the invalid state so that
+ // it remains functional. The request may have completed
+ // successfully, and it may be tempting to use the result in
+ // that case, but that opens up a can of worms. We'll follow
+ // the behavior in RequestMonitor: when an RM is canceled,
+ // it's canceled; period.
+ if (!isCanceled()) {
+ set(getData(), getStatus());
}
- set(getData(), status);
}
}
@Override
- public synchronized void setStatus(IStatus status) {
- fRawStatus = status;
- };
-
- @Override
public boolean isCanceled() {
return super.isCanceled() || RequestCache.this.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 67c0cc6bb8..c9cd8021ab 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
@@ -156,7 +156,7 @@ public class CacheTests {
Assert.assertFalse(fRetrieveRm.isCanceled());
}
- private void assertCacheCanceled() {
+ private void assertCacheInvalidAndWithCanceledRM() {
Assert.assertFalse(fTestCache.isValid());
try {
fTestCache.getData();
@@ -435,9 +435,11 @@ public class CacheTests {
Assert.fail("Expected a cancellation exception");
} catch (CancellationException e) {} // Expected exception;
- assertCacheCanceled();
+ assertCacheInvalidAndWithCanceledRM();
- // Completed the retrieve RM
+ // Simulate the retrieval completing successfully despite the cancel
+ // request. Perhaps the retrieval logic isn't checking the RM status.
+ // Even if it is checking, it may have gotten passed its last checkpoint
fExecutor.submit(new DsfRunnable() {
public void run() {
fRetrieveRm.setData(1);
@@ -445,11 +447,79 @@ public class CacheTests {
}
}).get();
- // Validate that cache accepts the canceled request data
- assertCacheValidWithData(1);
+ // Validate that cache didn't accept the result after its RM was canceled
+ assertCacheInvalidAndWithCanceledRM();
+ }
+
+ @Test
+ public void cancelWhilePendingTest2() throws InterruptedException, ExecutionException {
+ // Request data from cache
+ Query<Integer> q = new TestQuery();
+ fExecutor.execute(q);
+
+ // Wait until the cache starts data retrieval.
+ waitForRetrieveRm();
+
+ // Cancel the client request
+ q.cancel(true);
+ try {
+ q.get();
+ Assert.fail("Expected a cancellation exception");
+ } catch (CancellationException e) {} // Expected exception;
+
+ assertCacheInvalidAndWithCanceledRM();
+
+ // Simulate retrieval logic that is regularly checking the RM's cancel
+ // status and has discovered that the request has been canceled. It
+ // technically does not need to explicitly set a cancel status object in
+ // the RM, thanks to RequestMonitor.getStatus() automatically returning
+ // Status.CANCEL_STATUS when its in the cancel state. So here we
+ // simulate the retrieval logic just aborting its operations and
+ // completing the RM. Note that it hasn't provided the data to the
+ // cache.
+ fExecutor.submit(new DsfRunnable() {
+ public void run() {
+ fRetrieveRm.done();
+ }
+ }).get();
+
+ assertCacheInvalidAndWithCanceledRM();
}
@Test
+ public void cancelWhilePendingTest3() throws InterruptedException, ExecutionException {
+ // Request data from cache
+ Query<Integer> q = new TestQuery();
+ fExecutor.execute(q);
+
+ // Wait until the cache starts data retrieval.
+ waitForRetrieveRm();
+
+ // Cancel the client request
+ q.cancel(true);
+ try {
+ q.get();
+ Assert.fail("Expected a cancellation exception");
+ } catch (CancellationException e) {} // Expected exception;
+
+ assertCacheInvalidAndWithCanceledRM();
+
+ // Simulate retrieval logic that is regularly checking the RM's cancel
+ // status and has discovered that the request has been canceled. It
+ // aborts its processing, sets STATUS.CANCEL_STATUS in the RM and
+ // completes it.
+ fExecutor.submit(new DsfRunnable() {
+ public void run() {
+ fRetrieveRm.setStatus(Status.CANCEL_STATUS);
+ fRetrieveRm.done();
+ }
+ }).get();
+
+ // Validate that cache didn't accept the result after its RM was canceled
+ assertCacheInvalidAndWithCanceledRM();
+ }
+
+ @Test
public void cancelWhilePendingWithoutClientNotificationTest() throws InterruptedException, ExecutionException {
// Request data from cache
Query<Integer> q = new Query<Integer>() {
@@ -479,7 +549,7 @@ public class CacheTests {
// Cancel the client request
q.cancel(true);
- assertCacheCanceled();
+ assertCacheInvalidAndWithCanceledRM();
try {
q.get();
@@ -494,8 +564,8 @@ public class CacheTests {
}
}).get();
- // Validate that cache accepts the canceled request data
- assertCacheValidWithData(1);
+ // Validate that cache didn't accept the result after its RM was canceled
+ assertCacheInvalidAndWithCanceledRM();
}
/**
@@ -508,7 +578,6 @@ public class CacheTests {
public void cancelAfterCompletedRaceCondition() throws InterruptedException, ExecutionException {
// Create a client request with a badly behaved cancel implementation.
- @SuppressWarnings("unchecked")
final RequestMonitor[] rmBad = new RequestMonitor[1] ;
final boolean qBadCanceled[] = new boolean[] { false };
Query<Integer> qBad = new Query<Integer>() {
@@ -538,7 +607,8 @@ public class CacheTests {
// Avoid clearing cancel listeners list
};
- protected void handleSuccess() {
+ @Override
+ protected void handleSuccess() {
rm.setData(fTestCache.getData());
rm.done();
};
@@ -587,13 +657,15 @@ public class CacheTests {
@Test
public void cancelWhilePendingWithTwoClientsTest() throws InterruptedException, ExecutionException {
- // Request data from cache
+
+ // Request data from cache. Use submit/get to ensure both update
+ // requests are initiated before we wait for retrieval to start
Query<Integer> q1 = new TestQuery();
- fExecutor.execute(q1);
+ fExecutor.submit(q1).get();
// Request data from cache again
Query<Integer> q2 = new TestQuery();
- fExecutor.execute(q2);
+ fExecutor.submit(q2).get();
// Wait until the cache starts data retrieval.
@@ -614,7 +686,7 @@ public class CacheTests {
Assert.fail("Expected a cancellation exception");
} catch (CancellationException e) {} // Expected exception;
- assertCacheCanceled();
+ assertCacheInvalidAndWithCanceledRM();
// Completed the retrieve RM
fExecutor.submit(new DsfRunnable() {
@@ -624,8 +696,8 @@ public class CacheTests {
}
}).get();
- // Validate that cache accepts the canceled request data
- assertCacheValidWithData(1);
+ // Validate that cache didn't accept the result after its RM was canceled
+ assertCacheInvalidAndWithCanceledRM();
}
@Test
@@ -634,7 +706,7 @@ public class CacheTests {
List<Query<Integer>> qList = new ArrayList<Query<Integer>>();
for (int i = 0; i < 10; i++) {
Query<Integer> q = new TestQuery();
- fExecutor.execute(q);
+ fExecutor.submit(q).get();
qList.add(q);
}
@@ -660,7 +732,7 @@ public class CacheTests {
// Replace canceled requests with new ones
for (int i = 0; i < toCancel.length; i++) {
Query<Integer> q = new TestQuery();
- fExecutor.execute(q);
+ fExecutor.submit(q).get();
qList.set(toCancel[i], q);
assertCacheWaiting();
}
@@ -672,7 +744,7 @@ public class CacheTests {
qList.get(i).cancel(true);
}
qList.get(qList.size() - 1).cancel(true);
- assertCacheCanceled();
+ assertCacheInvalidAndWithCanceledRM();
// Completed the retrieve RM
fExecutor.submit(new DsfRunnable() {
@@ -682,8 +754,8 @@ public class CacheTests {
}
}).get();
- // Validate that cache accepts the canceled request data
- assertCacheValidWithData(1);
+ // Validate that cache didn't accept the result after its RM was canceled
+ assertCacheInvalidAndWithCanceledRM();
}
@Test

Back to the top