Fix and unit test for #804: Race condition in LanguageServerWrapper (#…

…808)

* Test exception in ConnectionStreamProvider.start()

When ConnectionStreamProvider.start() throws an IOException, a race condition can occur that causes further attempts to start the language server to be ignored (issue 804)

* Fix race condition in LanguageServerWrapper

Fixes #804

org.eclipse.lsp4e.test/fragment.xml

@@ -105,6 +105,17 @@
             contentType="org.eclipse.lsp4e.test.singleton-ls-content-type"
             id="org.eclipse.lsp4e.test.singletonLS">
       </contentTypeMapping>
+       <server
+               class="org.eclipse.lsp4e.test.utils.MockConnectionProviderWithStartException"
+               id="org.eclipse.lsp4e.test.connection-provider-with-start-exception"
+               label="Connection Provider with start exception"
+               lastDocumentDisconnectedTimeout="0"
+               >
+       </server>
+       <contentTypeMapping
+               contentType="org.eclipse.lsp4e.test.stream-provider-start-exception-content-type"
+               id="org.eclipse.lsp4e.test.connection-provider-with-start-exception">
+       </contentTypeMapping>
    </extension>
    <extension
          point="org.eclipse.core.contenttype.contentTypes">
@@ -200,6 +211,12 @@
             name="Test Content-Type associated with Singleton LS"
             priority="normal">
       </content-type>
+       <content-type
+               file-extensions="lsptStartException"
+               id="org.eclipse.lsp4e.test.stream-provider-start-exception-content-type"
+               name="Test Content-Type associated with stream provider that causes an exception in start()"
+               priority="normal">
+       </content-type>
    </extension>
    <extension
          point="org.eclipse.ui.startup">

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

@@ -19,6 +19,7 @@
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ForkJoinPool;
+import java.util.concurrent.TimeoutException;
 
 import org.eclipse.core.resources.IFile;
 import org.eclipse.core.resources.IProject;
@@ -27,6 +28,7 @@
 import org.eclipse.lsp4e.LanguageServerWrapper;
 import org.eclipse.lsp4e.LanguageServiceAccessor;
 import org.eclipse.lsp4e.test.utils.AllCleanRule;
+import org.eclipse.lsp4e.test.utils.MockConnectionProviderWithStartException;
 import org.eclipse.lsp4e.test.utils.TestUtils;
 import org.eclipse.ui.IEditorPart;
 import org.junit.Before;
@@ -115,4 +117,32 @@ public void testStopAndActive() throws CoreException, IOException, AssertionErro
 			TestUtils.closeEditor(editor1, false);
 		}
 	}
+
+
+	@Test
+	public void testStartExceptionRace() throws Exception {
+		IFile testFile1 = TestUtils.createFile(project1, "shouldUseExtension.lsptStartException", "");
+
+		IEditorPart editor1 = TestUtils.openEditor(testFile1);
+
+		MockConnectionProviderWithStartException.resetCounters();
+		final int RUNS = 10;
+
+		for (int i = 0; i < RUNS; i++) {
+			MockConnectionProviderWithStartException.resetStartFuture();
+			@NonNull Collection<LanguageServerWrapper> wrappers = LanguageServiceAccessor.getLSWrappers(testFile1, request -> true);
+			try {
+				MockConnectionProviderWithStartException.waitForStart();
+			} catch (TimeoutException e) {
+				throw new RuntimeException("Start #" + i + " was not called", e);
+			}
+			assertEquals(1, wrappers.size());
+			LanguageServerWrapper wrapper = wrappers.iterator().next();
+			assertTrue(!wrapper.isActive());
+			assertTrue(MockConnectionProviderWithStartException.getStartCounter() >= i);
+		}
+		waitForAndAssertCondition(2_000, () -> MockConnectionProviderWithStartException.getStopCounter() >= RUNS);
+
+		TestUtils.closeEditor(editor1, false);
+	}
 }

org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/utils/MockConnectionProviderWithStartException.java

@@ -0,0 +1,56 @@
+/*******************************************************************************
+ * Copyright (c) 2017 Red Hat Inc. and others.
+ * This program and the accompanying materials are made
+ * available under the terms of the Eclipse Public License 2.0
+ * which is available at https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *
+ * Contributors:
+ *  Lucia Jelinkova (Red Hat Inc.)  - initial implementation
+ *******************************************************************************/
+package org.eclipse.lsp4e.test.utils;
+
+import java.io.IOException;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class MockConnectionProviderWithStartException extends MockConnectionProvider {
+	private static volatile CompletableFuture<Void> startFuture = new CompletableFuture<>();
+	private static volatile int startCounter = 0;
+	private static volatile int stopCounter = 0;
+	public static void resetStartFuture() {
+		startFuture = new CompletableFuture<>();
+	}
+
+	public static void waitForStart() throws ExecutionException, InterruptedException, TimeoutException {
+		startFuture.get(2, TimeUnit.SECONDS);
+	}
+
+	public static void resetCounters() {
+		startCounter = 0;
+		stopCounter = 0;
+	}
+
+	public static int getStartCounter() {
+		return startCounter;
+	}
+
+	public static int getStopCounter() {
+		return stopCounter;
+	}
+
+	@Override
+	public void start() throws IOException {
+		startCounter++;
+		startFuture.complete(null);
+		throw new IOException("Start failed");
+	}
+
+	@Override
+	public void stop() {
+		stopCounter++;
+	}
+}

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

@@ -322,10 +322,13 @@ public synchronized void start() throws IOException {
 				FileBuffers.getTextFileBufferManager().addFileBufferListener(fileBufferListener);
 			}).exceptionally(e -> {
 				LanguageServerPlugin.logError(e);
-				initializeFuture.completeExceptionally(e);
 				stop();
-				return null;
+				throw new RuntimeException(e);
 			});
+			if (this.initializeFuture.isCompletedExceptionally()) {
+				// This might happen if an exception occurred and stop() was called before this.initializeFuture was assigned
+				this.initializeFuture = null;
+			}
 		}
 	}