Bug 575205: [Core, model indexer] Race condition workspace model indexer

- do not use JobManager::join to wait for idle state of indexing
- aggregate after-index computations onto a single job to avoid
  flooding the JobManager with tiny JobBasedFutures
- add new listener mechanism in IndexManager to improve testability
  of synchronization scenarios involving the JobWrangler job
- ensure isolation of WorkspaceModelIndex tests for consistent
  execution

Change-Id: Icb915bebb8a90e46e29aafe3b788c18ade42033d
Signed-off-by: Christian W. Damus <give.a.damus@gmail.com>

1 files changed, 237 insertions, 128 deletions

diff --git a/tests/junit/plugins/infra/emf/org.eclipse.papyrus.infra.emf.tests/tests/org/eclipse/papyrus/infra/emf/resource/index/WorkspaceModelIndexTest.java b/tests/junit/plugins/infra/emf/org.eclipse.papyrus.infra.emf.tests/tests/org/eclipse/papyrus/infra/emf/resource/index/WorkspaceModelIndexTest.java
index 75b6f1377db..e76eba65479 100644
--- a/tests/junit/plugins/infra/emf/org.eclipse.papyrus.infra.emf.tests/tests/org/eclipse/papyrus/infra/emf/resource/index/WorkspaceModelIndexTest.java
+++ b/tests/junit/plugins/infra/emf/org.eclipse.papyrus.infra.emf.tests/tests/org/eclipse/papyrus/infra/emf/resource/index/WorkspaceModelIndexTest.java
@@ -1,6 +1,6 @@
 /*****************************************************************************
- * Copyright (c) 2014, 2017 Christian W. Damus and others.
- * 
+ * Copyright (c) 2014, 2021 Christian W. Damus, CEA LIST, and others.
+ *
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License 2.0
  * which accompanies this distribution, and is available at
@@ -10,7 +10,7 @@
  *
  * Contributors:
  *   Christian W. Damus - Initial API and implementation
- *   
+ *
  *****************************************************************************/
 
 package org.eclipse.papyrus.infra.emf.resource.index;
@@ -31,14 +31,16 @@ import java.io.InputStream;
 import java.nio.charset.Charset;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.EnumMap;
+import java.util.EnumSet;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Future;
 import java.util.concurrent.Semaphore;
 import java.util.concurrent.TimeUnit;
-import java.util.concurrent.locks.Condition;
-import java.util.concurrent.locks.Lock;
-import java.util.concurrent.locks.ReentrantLock;
 
 import org.eclipse.core.resources.IFile;
 import org.eclipse.core.resources.IProject;
@@ -61,6 +63,7 @@ import org.eclipse.emf.ecore.resource.ResourceSet;
 import org.eclipse.emf.ecore.resource.impl.ResourceSetImpl;
 import org.eclipse.emf.ecore.util.EcoreUtil;
 import org.eclipse.papyrus.infra.emf.Activator;
+import org.eclipse.papyrus.infra.emf.internal.resource.index.IIndexManagerListener;
 import org.eclipse.papyrus.infra.emf.internal.resource.index.IndexManager;
 import org.eclipse.papyrus.infra.emf.internal.resource.index.InternalModelIndex;
 import org.eclipse.papyrus.infra.emf.utils.EMFHelper;
@@ -69,12 +72,15 @@ import org.eclipse.papyrus.junit.utils.LogTracker;
 import org.eclipse.papyrus.junit.utils.rules.HouseKeeper;
 import org.eclipse.uml2.uml.resource.UMLResource;
 import org.junit.After;
-import org.junit.AfterClass;
 import org.junit.Before;
-import org.junit.BeforeClass;
+import org.junit.FixMethodOrder;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.TestName;
+import org.junit.runners.MethodSorters;
 
+import com.google.common.base.Strings;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
 import com.google.common.io.Files;
@@ -82,17 +88,23 @@ import com.google.common.io.Files;
 /**
  * Test suite for the {@link WorkspaceModelIndex} class.
  */
+@FixMethodOrder(MethodSorters.JVM)
 public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 
-	private static final Sync sync = new Sync();
-	private static final CrossReferenceIndexer index = new CrossReferenceIndexer(sync);
+	static final boolean DEBUG = Platform.inDebugMode();
+
+	private final Sync sync = new Sync();
+	private final CrossReferenceIndexer index = new CrossReferenceIndexer(sync);
 
-	private static IndexManager manager;
-	private static WorkspaceModelIndex<CrossReferenceIndex> fixture;
+	private IndexManager manager;
+	private WorkspaceModelIndex<CrossReferenceIndex> fixture;
 
 	@Rule
 	public final HouseKeeper houseKeeper = new HouseKeeper();
 
+	@Rule
+	public final TestName testName = new TestName();
+
 	private IProject referencingProject;
 	private IFile referencingFile;
 	private URI referencingURI;
@@ -118,6 +130,10 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 
 		final String newFileName = "the_referencing_model.uml";
 
+		// Interlock with the indexing to ensure that we don't try the index before it hears
+		// about the file delta
+		sync.init(SyncMode.TEST, IndexMode.MANAGER);
+
 		RenameJob rename = new RenameJob(referencingFile, newFileName);
 
 		// Update the identity of the file
@@ -127,6 +143,8 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 		// Rename it
 		rename.go();
 
+		sync.syncFromTest(); // Wait for indexing to start
+
 		long requestIndex = System.currentTimeMillis();
 
 		// Check the index
@@ -144,11 +162,17 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 		// Initial build
 		Map<IFile, CrossReferenceIndex> index = fixture.getIndex().get();
 
+		// Interlock with the indexing to ensure that we don't try the index before it hears
+		// about the file delta
+		sync.init(SyncMode.TEST, IndexMode.MANAGER);
+
 		DeleteJob delete = new DeleteJob(referencingFile);
 
 		// Delete it
 		delete.go();
 
+		sync.syncFromTest(); // Wait for indexing to start
+
 		long requestIndex = System.currentTimeMillis();
 
 		// Check the index
@@ -166,8 +190,14 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 		// Initial build
 		Map<IFile, CrossReferenceIndex> index = fixture.getIndex().get();
 
+		// Interlock with the indexing to ensure that we don't try the index before it hears
+		// about the file delta
+		sync.init(SyncMode.TEST, IndexMode.UNINDEX);
+
 		referencingFile.getProject().close(null);
 
+		sync.syncFromTest(); // Wait for unindexing to start
+
 		// Check the index
 		index = fixture.getIndex().get();
 
@@ -184,8 +214,14 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 		// Initial build
 		Map<IFile, CrossReferenceIndex> index = fixture.getIndex().get();
 
+		// Interlock with the indexing to ensure that we don't try the index before it hears
+		// about the file delta
+		sync.init(SyncMode.TEST, IndexMode.UNINDEX);
+
 		referencingFile.getProject().delete(true, true, null);
 
+		sync.syncFromTest(); // Wait for unindexing to start
+
 		// Check the index
 		index = fixture.getIndex().get();
 
@@ -218,8 +254,14 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 
 		TestListener listener = attach(new TestListener());
 
+		// Interlock with the indexing to ensure that we don't try the index before it hears
+		// about the file delta
+		sync.init(SyncMode.TEST, IndexMode.UNINDEX);
+
 		referencingFile.delete(true, null);
 
+		sync.syncFromTest(); // Wait for unindexing to start
+
 		// Access the index again to synchronize
 		fixture.getIndex().get();
 
@@ -233,7 +275,7 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 		Map<IFile, CrossReferenceIndex> index = fixture.getIndex().get();
 
 		// Interlock with the indexing for timing purposes
-		sync.createIndexSync();
+		Future<Job> controlJob = sync.init(() -> getJobWrangler(manager), SyncMode.INDEX, IndexMode.MANAGER, IndexMode.INDEX);
 
 		final String newFileName = "the_referencing_model.uml";
 
@@ -244,28 +286,21 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 		referencingFile = referencingProject.getFile(new Path(newFileName));
 		referencingURI = uri(referencingFile);
 
-		// Let the indexing start
-		sync.syncFromTest();
+		// Let the index manager start
+		sync.syncFromTest(IndexMode.MANAGER);
 
 		// Cancel the index control job
-		Job[] family = Job.getJobManager().find(manager);
-		Job controlJob = null;
-		for (Job job : family) {
-			if (job.getClass().getSimpleName().contains("JobWrangler")) {
-				controlJob = job;
-				break;
-			}
-		}
-		assertThat("Control job not found", controlJob, notNullValue());
+		JobWaiter controlJobWaiter = waitForStart(controlJob.get(30L, TimeUnit.SECONDS));
+
+		// Let the indexing start
+		sync.syncFromTest(IndexMode.INDEX);
 
 		long cancellingAt = System.currentTimeMillis();
 
-		controlJob.cancel();
+		controlJob.get().cancel();
 
 		// Let the indexing finish
-		sync.syncFromTest();
-
-		JobWaiter controlJobWaiter = JobWaiter.waitForStart(controlJob);
+		sync.syncFromTest(IndexMode.INDEX);
 
 		controlJobWaiter.waitForJob();
 
@@ -283,7 +318,7 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 	/**
 	 * Verify that the index will refresh workspace resources as necessary in order to
 	 * properly determine the content-types of files, skip missing resources, etc.
-	 * 
+	 *
 	 * @see https://bugs.eclipse.org/bugs/show_bug.cgi?id=473154
 	 */
 	@Test
@@ -294,7 +329,7 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 
 		final File nativeFile = new File(referencingFile.getLocationURI());
 		final Charset charset = Charset.forName(referencedFile.getCharset());
-		final String contents = Files.toString(nativeFile, charset);
+		final String contents = Files.asCharSource(nativeFile, charset).read();
 
 		// De-synchronize the file (by deletion, as in the bugzilla scenario)
 		new FileManipulationJob("Sneakily delete file", referencingFile) {
@@ -314,14 +349,14 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 
 		// Interlock with the indexing to ensure that we don't try the index before it hears
 		// about the file delta
-		sync.createTestSync();
+		sync.init(SyncMode.TEST, IndexMode.INDEX);
 
 		// Put the file back and synchronize it
 		new FileManipulationJob("Restore file", referencingFile) {
 			@Override
 			protected void manipulateFile(IFile file, IProgressMonitor monitor) throws CoreException {
 				try {
-					Files.write(contents, nativeFile, charset);
+					Files.asCharSink(nativeFile, charset).write(contents);
 				} catch (IOException e) {
 					throw new CoreException(new Status(IStatus.ERROR, Activator.PLUGIN_ID, "Failed to restore test file.", e));
 				}
@@ -342,28 +377,14 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 	// Test framework
 	//
 
-	@BeforeClass
-	public static void createFixture() {
-		manager = new IndexManager() {
-			@Override
-			protected Map<QualifiedName, InternalModelIndex> loadIndices() {
-				fixture = new WorkspaceModelIndex<>("test", UMLResource.UML_CONTENT_TYPE_IDENTIFIER, index, 2);
-				return Collections.singletonMap(fixture.getIndexKey(), (InternalModelIndex) fixture);
-			}
-		};
-		manager.startManager();
-	}
+	@Before
+	public void createFixture() throws Exception {
+		if (DEBUG) {
+			System.out.printf("[%s] Starting test ...%n", testName.getMethodName());
+		}
 
-	@AfterClass
-	public static void destroyFixture() {
-		// This disposes the fixture, too
-		manager.dispose();
-		manager = null;
-		fixture = null;
-	}
+		IndexManager.getInstance().pause();
 
-	@Before
-	public void createProjects() throws Exception {
 		referencedProject = houseKeeper.createProject("referencedProject");
 		referencingProject = houseKeeper.createProject("referencingProject");
 
@@ -384,11 +405,33 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 			input.close();
 		}
 		referencingURI = uri(referencingFile);
+
+		manager = new IndexManager() {
+			@Override
+			protected Map<QualifiedName, InternalModelIndex> loadIndices() {
+				fixture = new WorkspaceModelIndex<>("test", UMLResource.UML_CONTENT_TYPE_IDENTIFIER, index, 2);
+				return Collections.singletonMap(fixture.getIndexKey(), (InternalModelIndex) fixture);
+			}
+		};
+		manager.startManager();
 	}
 
 	@After
-	public void reset() {
-		sync.clear();
+	public void destroyFixture() {
+		try {
+			sync.clear();
+
+			// This disposes the fixture, too
+			manager.dispose();
+			manager = null;
+			fixture = null;
+
+			if (DEBUG) {
+				System.out.printf("[%s] Finished test.%n", testName.getMethodName());
+			}
+		} finally {
+			IndexManager.getInstance().resume();
+		}
 	}
 
 	static URI uri(IFile file) {
@@ -421,50 +464,102 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 	}
 
 	<T extends IWorkspaceModelIndexListener> T attach(T listener) {
-		fixture.addListener(houseKeeper.cleanUpLater(listener, new HouseKeeper.Disposer<T>() {
-			@Override
-			public void dispose(T object) {
-				fixture.removeListener(object);
+		fixture.addListener(listener);
+		return listener;
+	}
+
+	Job getJobWrangler(IndexManager indexManager) {
+		Job[] family = Job.getJobManager().find(manager);
+		Job result = null;
+		for (Job job : family) {
+			if (job.getClass().getSimpleName().contains("JobWrangler")) { //$NON-NLS-1$
+				result = job;
+				break;
 			}
-		}));
+		}
+		assertThat("Control job not found", result, notNullValue());
 
-		return listener;
+		return result;
 	}
 
 	//
 	// Nested types
 	//
 
-	static class Sync {
-		private volatile Semaphore sync;
-		private volatile Mode mode = Mode.NONE;
+	class Sync {
+		private volatile SyncMode mode = SyncMode.NONE;
+		private volatile Map<IndexMode, Semaphore> semaphores = new EnumMap<>(IndexMode.class);
+		private volatile Runnable onSyncFromIndex;
 
 		/**
-		 * Create (in the test) a semaphore that gates the index on the test.
+		 * Create (in the test) a semaphore that gates the index on the test or the test on the index,
+		 * according to the mode specified.
+		 *
+		 * @param syncMode
+		 *            the direction of synchronization gating
+		 * @param indexMode,&nbsp;more
+		 *            which kind of index operations are synchronized with the test
 		 */
-		void createIndexSync() {
-			sync = new Semaphore(0);
-			mode = Mode.INDEX;
+		void init(SyncMode syncMode, IndexMode indexMode, IndexMode... more) {
+			clear();
+
+			this.mode = syncMode;
+			EnumSet.of(indexMode, more).forEach(im -> semaphores.put(im, new Semaphore(0)));
+
+			if (this.semaphores.containsKey(IndexMode.MANAGER)) {
+				IIndexManagerListener[] listener = { null };
+				listener[0] = IIndexManagerListener.startingAdapter(__ -> {
+					try {
+						syncFromIndex(IndexMode.MANAGER);
+					} finally {
+						manager.removeIndexManagerListener(listener[0]);
+					}
+				});
+				manager.addIndexManagerListener(listener[0]);
+			}
 		}
 
-		/**
-		 * Create (in the test) a semaphore that gates the test on the index.
-		 */
-		void createTestSync() {
-			sync = new Semaphore(0);
-			mode = Mode.TEST;
+		<V> Future<V> init(Callable<V> callable, SyncMode syncMode, IndexMode indexMode, IndexMode... more) {
+			CompletableFuture<V> result = new CompletableFuture<>();
+
+			init(syncMode, indexMode, more);
+			onSyncFromIndex = () -> {
+				try {
+					result.complete(callable == null ? null : callable.call());
+				} catch (ThreadDeath d) {
+					throw d;
+				} catch (Throwable e) {
+					result.completeExceptionally(e);
+				} finally {
+					onSyncFromIndex = null;
+				}
+			};
+
+			return result;
 		}
 
-		/** Called by the index when it needs to sync up with the test. */
-		void syncFromIndex() {
-			switch (mode) {
+		/** Called by the indexing job when it needs to sync up with the test. */
+		void syncFromIndex(IndexMode mode) {
+			if (!semaphores.containsKey(mode)) {
+				return;
+			}
+
+			if (DEBUG && this.mode != SyncMode.NONE) {
+				System.out.printf("[%s] Synching from indexer (%s): %s [%s]%n", testName.getMethodName(), mode, this.mode, Thread.currentThread().getName());
+			}
+
+			if (onSyncFromIndex != null) {
+				onSyncFromIndex.run();
+			}
+
+			switch (this.mode) {
 			case INDEX:
 				// Index is gated on the test
-				sync.acquireUninterruptibly();
+				semaphores.get(mode).acquireUninterruptibly();
 				break;
 			case TEST:
 				// Test is gated on the index
-				sync.release();
+				semaphores.get(mode).release();
 				break;
 			default:
 				// Pass
@@ -473,14 +568,28 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 
 		/** Called by the test when it needs to sync up with the index. */
 		void syncFromTest() throws Exception {
-			switch (mode) {
+			assertThat("Must specify index mode", semaphores.size(), is(1));
+			syncFromTest(Iterables.getOnlyElement(semaphores.keySet()));
+		}
+
+		/** Called by the test when it needs to sync up with the index. */
+		void syncFromTest(IndexMode mode) throws Exception {
+			if (!semaphores.containsKey(mode)) {
+				return;
+			}
+
+			if (DEBUG && this.mode != SyncMode.NONE) {
+				System.out.printf("[%s] Synching from test (%s): %s [%s]%n", testName.getMethodName(), mode, this.mode, Thread.currentThread().getName());
+			}
+
+			switch (this.mode) {
 			case INDEX:
 				// Index is gated on the test
-				sync.release();
+				semaphores.get(mode).release();
 				break;
 			case TEST:
 				// Test is gated on the index
-				if (!sync.tryAcquire(5L, TimeUnit.SECONDS)) {
+				if (!semaphores.get(mode).tryAcquire(5L, TimeUnit.SECONDS)) {
 					fail("Timed out waiting for indexing job");
 				}
 				break;
@@ -490,17 +599,23 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 		}
 
 		void clear() {
-			mode = Mode.NONE;
-			if (sync != null) {
-				// Make sure that any blocked threads are released to whatever fate
-				sync.release(100);
-				sync = null;
-			}
-		}
+			onSyncFromIndex = null;
 
-		private enum Mode {
-			NONE, INDEX, TEST,
+			this.mode = SyncMode.NONE;
+
+			// Make sure that any blocked threads are released to whatever fate
+			semaphores.values().forEach(sema -> sema.release(100));
+			semaphores.clear();
 		}
+
+	}
+
+	private enum SyncMode {
+		NONE, INDEX, TEST,
+	}
+
+	private enum IndexMode {
+		INDEX, UNINDEX, MANAGER;
 	}
 
 	static class CrossReferenceIndexer implements WorkspaceModelIndex.IndexHandler<CrossReferenceIndex> {
@@ -510,7 +625,7 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 
 		/**
 		 * Initializes me.
-		 * 
+		 *
 		 * @param sync
 		 *            a protocol for handshaking at start and end of indexing a file
 		 */
@@ -538,7 +653,7 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 		public CrossReferenceIndex index(IFile file) {
 			final CrossReferenceIndex result = get(file);
 
-			sync.syncFromIndex();
+			sync.syncFromIndex(IndexMode.INDEX);
 
 			try {
 				Set<URI> imports = result.imports;
@@ -571,7 +686,7 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 					EMFHelper.unload(resourceSet);
 				}
 			} finally {
-				sync.syncFromIndex();
+				sync.syncFromIndex(IndexMode.INDEX);
 			}
 
 			return result;
@@ -581,6 +696,8 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 		public void unindex(IFile file) {
 			URI uri = uri(file);
 
+			sync.syncFromIndex(IndexMode.UNINDEX);
+
 			synchronized (index) {
 				// Remove this file from the index
 				index.remove(file);
@@ -680,13 +797,20 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 		}
 	}
 
-	static final class JobWaiter extends JobChangeAdapter {
+	final JobWaiter waitForStart(Job job) {
+		return new JobWaiter(job, true);
+	}
+
+	final JobWaiter waitForEnd(Job job) {
+		return new JobWaiter(job, false);
+	}
+
+	final class JobWaiter extends JobChangeAdapter {
 		private final Job job;
 		private final boolean waitForStart;
+		private final long startedAt = System.currentTimeMillis();
 
-		private final Lock lock = new ReentrantLock();
-		private final Condition cond = lock.newCondition();
-		private volatile boolean gotIt;
+		private final CountDownLatch jobSignal = new CountDownLatch(1);
 
 		private JobWaiter(Job job, boolean waitForStart) {
 			super();
@@ -697,54 +821,39 @@ public class WorkspaceModelIndexTest extends AbstractPapyrusTest {
 			Job.getJobManager().addJobChangeListener(this);
 		}
 
-		public static JobWaiter waitForStart(Job job) {
-			return new JobWaiter(job, true);
-		}
-
-		public static JobWaiter waitForEnd(Job job) {
-			return new JobWaiter(job, false);
-		}
-
 		public void waitForJob() {
-			lock.lock();
 
 			try {
-				while (!gotIt) {
-					try {
-						cond.await();
-					} catch (InterruptedException e) {
-						fail("Test was interrupted");
-					}
-				}
+				jobSignal.await();
+			} catch (InterruptedException e) {
+				fail("Test was interrupted");
 			} finally {
-				lock.unlock();
 				Job.getJobManager().removeJobChangeListener(this);
 			}
 		}
 
 		@Override
 		public void aboutToRun(IJobChangeEvent event) {
+			log(event.getJob(), "about to run");
+
 			if (waitForStart && (event.getJob() == job)) {
-				lock.lock();
-				try {
-					gotIt = true;
-					cond.signalAll();
-				} finally {
-					lock.unlock();
-				}
+				jobSignal.countDown();
+			}
+		}
+
+		private void log(Job job, String action) {
+			if (DEBUG && Strings.nullToEmpty(job.getName()).toLowerCase().contains("index")) {
+				long when = System.currentTimeMillis() - startedAt;
+				System.out.printf("[%s:%d] Job %s: %s%n", testName.getMethodName(), when, action, job);
 			}
 		}
 
 		@Override
 		public void done(IJobChangeEvent event) {
+			log(event.getJob(), "done");
+
 			if (!waitForStart && (event.getJob() == job)) {
-				lock.lock();
-				try {
-					gotIt = true;
-					cond.signalAll();
-				} finally {
-					lock.unlock();
-				}
+				jobSignal.countDown();
 			}
 		}