Maintenance/mitigate flaky tests

org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/AllCleanRule.java

@@ -16,6 +16,7 @@
 import org.eclipse.core.resources.IProject;
 import org.eclipse.core.resources.ResourcesPlugin;
 import org.eclipse.core.runtime.CoreException;
+import org.eclipse.lsp4e.ConnectDocumentToLanguageServerSetupParticipant;
 import org.eclipse.lsp4e.LanguageServiceAccessor;
 import org.eclipse.lsp4e.tests.mock.MockLanguageServer;
 import org.eclipse.lsp4j.ServerCapabilities;
@@ -26,6 +27,8 @@
 
 public class AllCleanRule extends TestWatcher {
 
+	private static final boolean LOG_TEST_NAMES = Boolean.getBoolean("lsp4e.log.test.names");
+
 	private final Supplier<ServerCapabilities> serverConfigurer;
 
 	public AllCleanRule() {
@@ -43,6 +46,9 @@ protected void starting(Description description) {
 		if (intro != null) {
 			PlatformUI.getWorkbench().getIntroManager().closeIntro(intro);
 		}
+		if (LOG_TEST_NAMES) {
+			System.out.println("Starting: " + description);
+		}
 		clear();
 	}
 
@@ -53,14 +59,17 @@ protected void finished(Description description) {
 	}
 
 	private void clear() {
-		PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage().closeAllEditors(false);
+		// Give the platform three attempts to shut down windows
+		for (int i = 3; i > 0 && !PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage().closeAllEditors(false); i--) {}
+		ConnectDocumentToLanguageServerSetupParticipant.waitForAll();
 		for (IProject project : ResourcesPlugin.getWorkspace().getRoot().getProjects()) {
 			try {
 				project.delete(true, null);
 			} catch (CoreException e) {
 				e.printStackTrace();
 			}
 		}
+		MockLanguageServer.INSTANCE.waitBeforeTearDown();
 		LanguageServiceAccessor.clearStartedServers();
 		MockLanguageServer.reset(this.serverConfigurer);
 		TestUtils.tearDown();

org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/MockConnectionProvider.java

@@ -51,9 +51,10 @@ public void start() throws IOException {
 		clientOutputStream = Channels.newOutputStream(clientOutputToServerInput.sink());
 		listener = launcher.startListening();
 		MockLanguageServer.INSTANCE.addRemoteProxy(launcher.getRemoteProxy());
-		streams.add(clientInputStream);
+
+		// Store the output streams so we can close them to clean up. The corresponding input
+		// streams should automatically receive an EOF and close.
 		streams.add(clientOutputStream);
-		streams.add(serverInputStream);
 		streams.add(serverOutputStream);
 		streams.add(errorStream);
 	}

org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/TestUtils.java

@@ -11,7 +11,8 @@
  *******************************************************************************/
 package org.eclipse.lsp4e.test;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 import java.io.ByteArrayInputStream;
 import java.io.File;
@@ -59,7 +60,7 @@ public class TestUtils {
 	private TestUtils() {
 		// this class shouldn't be instantiated
 	}
-
+	
 	public static ITextViewer openTextViewer(IFile file) throws PartInitException {
 		IEditorPart editor = openEditor(file);
 		return LSPEclipseUtils.getTextViewer(editor);

org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/WorkspaceFoldersTest.java

@@ -25,6 +25,7 @@
 import org.eclipse.core.resources.IProject;
 import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.NullProgressMonitor;
+import org.eclipse.lsp4e.ConnectDocumentToLanguageServerSetupParticipant;
 import org.eclipse.lsp4e.LanguageServerWrapper;
 import org.eclipse.lsp4e.LanguageServiceAccessor;
 import org.eclipse.lsp4e.test.TestUtils.JobSynchronizer;
@@ -100,6 +101,7 @@ public void testPojectCreate() throws Exception {
 				.next();
 		new LSDisplayHelper(() -> MockLanguageServer.INSTANCE.isRunning()).waitForCondition(Display.getCurrent(), 5000,
 				300);
+		ConnectDocumentToLanguageServerSetupParticipant.waitForAll();
 
 		Collection<LanguageServerWrapper> wrappers = LanguageServiceAccessor.getLSWrappers(testFile1,
 				c -> Boolean.TRUE);
@@ -127,7 +129,7 @@ public void testProjectClose() throws Exception {
 				.next();
 		new LSDisplayHelper(() -> MockLanguageServer.INSTANCE.isRunning()).waitForCondition(Display.getCurrent(), 5000,
 				300);
-
+		ConnectDocumentToLanguageServerSetupParticipant.waitForAll();
 		final JobSynchronizer synchronizer = new JobSynchronizer();
 		project.close(synchronizer);
 		synchronizer.await();
@@ -151,6 +153,7 @@ public void testProjectDelete() throws Exception {
 				.next();
 		new LSDisplayHelper(() -> MockLanguageServer.INSTANCE.isRunning()).waitForCondition(Display.getCurrent(), 5000,
 				300);
+		ConnectDocumentToLanguageServerSetupParticipant.waitForAll();
 
 		Collection<LanguageServerWrapper> wrappers = LanguageServiceAccessor.getLSWrappers(testFile1,
 				c -> Boolean.TRUE);
@@ -180,6 +183,7 @@ public void projectReopenTest() throws Exception {
 				.next();
 		new LSDisplayHelper(() -> MockLanguageServer.INSTANCE.isRunning()).waitForCondition(Display.getCurrent(), 5000,
 				300);
+		ConnectDocumentToLanguageServerSetupParticipant.waitForAll();
 
 		final JobSynchronizer synchronizer = new JobSynchronizer();
 		project.close(synchronizer);

org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/rename/RenameTest.java

@@ -103,10 +103,12 @@ public void testAsyncRenameHandlerEnablement() throws Exception {
 		ICommandService commandService = PlatformUI.getWorkbench().getService(ICommandService.class);
 		Command command = commandService.getCommand(IWorkbenchCommandConstants.FILE_RENAME);
 		assertFalse(command.isEnabled() && command.isHandled());
-
-		Thread.sleep(2 * delay);
-		assertTrue(command.isEnabled());
-		assertTrue(command.isHandled());
+
+		Thread.sleep(delay * 3);
+
+		// Put back so shutdown doesn't time out
+		MockLanguageServer.INSTANCE.setTimeToProceedQueries(0);
+		assertTrue(command.isEnabled() && command.isHandled());
 	}
 
 	@Test

org.eclipse.lsp4e.tests.mock/META-INF/MANIFEST.MF

@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: Mock Language Server to test LSP4E
 Bundle-SymbolicName: org.eclipse.lsp4e.tests.mock
-Bundle-Version: 0.14.4.qualifier
+Bundle-Version: 0.14.5.qualifier
 Bundle-Vendor: Eclipse LSP4E
 Bundle-RequiredExecutionEnvironment: JavaSE-1.8
 Require-Bundle: org.eclipse.lsp4j;bundle-version="[0.14.0,0.15.0)",

org.eclipse.lsp4e.tests.mock/src/org/eclipse/lsp4e/tests/mock/MockLanguageServer.java

@@ -20,7 +20,9 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
+import java.util.concurrent.CancellationException;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
 import java.util.function.Supplier;
@@ -79,6 +81,8 @@ public final class MockLanguageServer implements LanguageServer {
 
 	private List<LanguageClient> remoteProxies = new ArrayList<>();
 
+	private List<CompletableFuture<?>> inFlight = new ArrayList<>();
+
 	public static void reset() {
 		INSTANCE = new MockLanguageServer(MockLanguageServer::defaultServerCapabilities);
 	}
@@ -104,6 +108,16 @@ public static void main(String[] args) throws InterruptedException, ExecutionExc
 		f.get();
 	}
 
+	public void waitBeforeTearDown() {
+		this.inFlight.forEach(future -> {
+			try {
+				future.join();
+			} catch (CancellationException | CompletionException e) {
+				System.err.println("Error waiting for in flight requests prior to teardown: " + e.getMessage());
+			}
+		});
+	}
+
 	public void addRemoteProxy(LanguageClient remoteProxy) {
 		this.textDocumentService.addRemoteProxy(remoteProxy);
 		this.remoteProxies.add(remoteProxy);
@@ -116,13 +130,15 @@ private void resetInitializeResult(final Supplier<ServerCapabilities> serverConf
 
 	<U> CompletableFuture<U> buildMaybeDelayedFuture(U value) {
 		if (delay > 0) {
-			return CompletableFuture.runAsync(() -> {
+			CompletableFuture<U> future = CompletableFuture.runAsync(() -> {
 				try {
 					Thread.sleep(delay);
 				} catch (InterruptedException e) {
 					throw new RuntimeException(e);
 				}
 			}).thenApply(v -> value);
+			inFlight.add(future);
+			return future;
 		}
 		return CompletableFuture.completedFuture(value);
 	}

org.eclipse.lsp4e/src/org/eclipse/lsp4e/ConnectDocumentToLanguageServerSetupParticipant.java

@@ -12,7 +12,9 @@
 package org.eclipse.lsp4e;
 
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
+import java.util.Set;
 
 import org.eclipse.core.filebuffers.IDocumentSetupParticipant;
 import org.eclipse.core.filebuffers.IDocumentSetupParticipantExtension;
@@ -23,6 +25,7 @@
 import org.eclipse.core.runtime.IPath;
 import org.eclipse.core.runtime.IProgressMonitor;
 import org.eclipse.core.runtime.IStatus;
+import org.eclipse.core.runtime.NullProgressMonitor;
 import org.eclipse.core.runtime.Status;
 import org.eclipse.core.runtime.jobs.Job;
 import org.eclipse.jface.text.IDocument;
@@ -33,6 +36,7 @@
  */
 public class ConnectDocumentToLanguageServerSetupParticipant implements IDocumentSetupParticipant, IDocumentSetupParticipantExtension {
 	private final Map<IPath, Job> locationMap = new HashMap<>();
+	private static final Set<Job> SUBMITTED_JOBS = new HashSet<>();
 
 	public ConnectDocumentToLanguageServerSetupParticipant() {
 
@@ -72,15 +76,34 @@ protected IStatus run(IProgressMonitor monitor) {
 					return Status.CANCEL_STATUS;
 				}
 				// connect to LS so they start receiving notifications and pushing diagnostics
-				LanguageServiceAccessor.getLanguageServers(document, capabilities -> true);
-				locationMap.remove(location);
+				LanguageServiceAccessor.getLanguageServers(document, capabilities -> true)
+					.thenRun(() -> {
+						locationMap.remove(location);
+						SUBMITTED_JOBS.remove(this);
+					});
 				return Status.OK_STATUS;
 			}
 		};
+		SUBMITTED_JOBS.add(job);
 		locationMap.put(location, job);
 		job.setUser(true);
 		job.setPriority(Job.INTERACTIVE);
 		job.schedule(100); // give some time to populate doc and associate it with the IFile
 	}
 
+	/**
+	 * Testing hook to ensure teardown doesn't remove documents while the LSP has in-flight async
+	 * jobs trying to attach to them
+	 */
+	public static void waitForAll() {
+		SUBMITTED_JOBS.forEach(job -> {
+			job.cancel();
+			try {
+				job.join(1000, new NullProgressMonitor());
+			} catch (InterruptedException e) {
+				LanguageServerPlugin.logInfo("Interrupted trying to cancel document setup"); //$NON-NLS-1$;
+			}
+		});
+	}
+
 }

org.eclipse.lsp4e/src/org/eclipse/lsp4e/LSPEclipseUtils.java

@@ -661,6 +661,11 @@ public static IDocument getDocument(ITextEditor editor) {
 	}
 
 	public static IDocument getDocument(IEditorInput editorInput) {
+		if (!editorInput.exists()) {
+			// Shouldn't happen too often, but happens rather a lot in testing when
+			// teardown runs when there are document setup actions still pending
+			return null;
+		}
 		if(editorInput instanceof IFileEditorInput) {
 			IFileEditorInput fileEditorInput = (IFileEditorInput)editorInput;
 				return getDocument(fileEditorInput.getFile());

org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java

@@ -37,6 +37,7 @@
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.UnaryOperator;
 import java.util.stream.Collectors;
 
@@ -179,6 +180,7 @@ public void dirtyStateChanged(IFileBuffer buffer, boolean isDirty) {
 	private LanguageServer languageServer;
 	private ServerCapabilities serverCapabilities;
 	private Timer timer;
+	private AtomicBoolean stopping = new AtomicBoolean(false);
 
 	/**
 	 * Map containing unregistration handlers for dynamic capability registrations.
@@ -437,7 +439,21 @@ public void run() {
 		}, TimeUnit.SECONDS.toMillis(this.serverDefinition.lastDocumentDisconnectedTimeout));
 	}
 
+	/**
+	 * Internal hook so that the unwrapped remote proxy can be matched to the corresponding
+	 * wrapper, which tracks things like whether it is still running or not
+	 * @param server LanguageServer to match on
+	 * @return True if this is the wrapper for the given server
+	 */
+	boolean isWrapperFor(LanguageServer server) {
+		return server == this.languageServer;
+	}
+
 	public synchronized void stop() {
+		final boolean alreadyStopping = this.stopping.getAndSet(true);
+		if (alreadyStopping) {
+			return;
+		}
 		removeStopTimer();
 		if (this.initializeFuture != null) {
 			this.initializeFuture.cancel(true);
@@ -475,6 +491,7 @@ public synchronized void stop() {
 			if (provider != null) {
 				provider.stop();
 			}
+			this.stopping.set(false);
 		};
 
 		CompletableFuture.runAsync(shutdownKillAndStopFutureAndProvider);

org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServiceAccessor.java

@@ -159,6 +159,10 @@ public static void disableLanguageServerContentType(
 		}
 	}
 
+	public static boolean isStillRunning(LanguageServer server) {
+		return startedServers.stream().anyMatch(wrapper -> wrapper.isWrapperFor(server) && wrapper.isActive());
+	}
+
 	public static void enableLanguageServerContentType(
 			@NonNull final ContentTypeToLanguageServerDefinition contentTypeToLSDefinition,
 			@NonNull final IEditorReference[] editors) {

org.eclipse.lsp4e/src/org/eclipse/lsp4e/outline/EditorToOutlineAdapterFactory.java

@@ -55,7 +55,7 @@ public <T> T getAdapter(Object adaptableObject, Class<T> adapterType) {
 
 				// first try to get / remove language server from cache from a previous call
 				LanguageServer server = LANG_SERVER_CACHE.remove(adaptableObject);
-				if (server != null) {
+				if (server != null && LanguageServiceAccessor.isStillRunning(server)) {
 					return adapterType.cast(createOutlinePage(editorPart, server));
 				}
 
@@ -79,7 +79,9 @@ public <T> T getAdapter(Object adaptableObject, Class<T> adapterType) {
 					if (!servers.isEmpty()) {
 						// TODO consider other strategies (select, merge...?)
 						LanguageServer languageServer = servers.get(0);
-						return adapterType.cast(createOutlinePage(editorPart, languageServer));
+						if (LanguageServiceAccessor.isStillRunning(languageServer)) {
+							return adapterType.cast(createOutlinePage(editorPart, languageServer));
+						}
 					}
 				}
 			}