fix: propagate LanguagServerWrapper startup exception to consumers (#…

…1044)

If there is an exception in the startup of the LanguageServerWrapper,
consumers get a cancellation exception rather than the actual exception
that occurred.
With this commit, this behavior is changed. As a consequence of the
change, calls to LanguageServerWrapper.start() will do nothing after a
failure. To test for this case (and attempt a restart() if feasible),
the startupFailed() method is added.

org.eclipse.lsp4e.test/fragment.xml

@@ -109,17 +109,6 @@
             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">
@@ -222,12 +211,6 @@
             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

@@ -14,13 +14,13 @@
 import static org.eclipse.lsp4e.test.utils.TestUtils.waitForAndAssertCondition;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.matchesPattern;
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 import java.util.Collection;
 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;
@@ -29,7 +29,6 @@
 import org.eclipse.lsp4e.LanguageServerWrapper;
 import org.eclipse.lsp4e.LanguageServiceAccessor;
 import org.eclipse.lsp4e.test.utils.AbstractTestWithProject;
-import org.eclipse.lsp4e.test.utils.MockConnectionProviderWithStartException;
 import org.eclipse.lsp4e.test.utils.TestUtils;
 import org.eclipse.ui.IEditorPart;
 import org.junit.Before;
@@ -109,30 +108,4 @@ public void testStopAndActive() throws CoreException, AssertionError, Interrupte
 		}
 	}
 
-	@Test
-	public void testStartExceptionRace() throws Exception {
-		IFile testFile1 = TestUtils.createFile(project, "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/src/org/eclipse/lsp4e/LanguageServerWrapper.java

@@ -242,8 +242,11 @@ private List<WorkspaceFolder> getRelevantWorkspaceFolders() {
 	}
 
 	/**
-	 * Starts a language server and triggers initialization. If language server is
-	 * started and active, does nothing. If language server is inactive, restart it.
+	 * Starts a language server and triggers initialization. If language server has been started
+	 * before, does nothing.
+	 * If the initialization throws an exception (e.g. because the binary for the LS could not be found),
+	 * call {@link #restart()} to force another startup attempt (e.g. because the exception could be handled programmatically).
+	 * Use {@link #startupFailed()} to check if an exception has occurred.
 	 */
 	public synchronized void start() {
 		start(false);
@@ -279,7 +282,7 @@ private synchronized void start(boolean forceRestart) {
 				stop();
 			}
 		}
-		if (this.initializeFuture == null) {
+		if (this.initializeFuture == null || forceRestart) {
 			final URI rootURI = getRootURI();
 			final Job job = createInitializeLanguageServerJob();
 			this.launcherFuture = new CompletableFuture<>();
@@ -354,7 +357,7 @@ private synchronized void start(boolean forceRestart) {
 				FileBuffers.getTextFileBufferManager().addFileBufferListener(fileBufferListener);
 				advanceInitializeFutureMonitor();
 			}).exceptionally(e -> {
-				stop();
+				shutdown();
 				final Throwable cause = e.getCause();
 				if (cause instanceof CancellationException c) {
 					throw c;
@@ -486,6 +489,13 @@ public synchronized boolean isActive() {
 		return launcherFuture != null && !launcherFuture.isDone();
 	}
 
+	/**
+	 * @return whether the last startup attempt has failed
+	 */
+	public synchronized boolean startupFailed() {
+		return this.initializeFuture != null && this.initializeFuture.isCompletedExceptionally();
+	}
+
 	private void removeStopTimerTask() {
 		synchronized (timer) {
 			if (stopTimerTask != null) {
@@ -521,6 +531,14 @@ boolean isWrapperFor(LanguageServer server) {
 	}
 
 	public synchronized void stop() {
+		if (this.initializeFuture != null) {
+			initializeFuture.cancel(true);
+			this.initializeFuture= null;
+		}
+		shutdown();
+	}
+
+	private void shutdown() {
 		final boolean alreadyStopping = this.stopping.getAndSet(true);
 		if (alreadyStopping) {
 			return;
@@ -531,11 +549,6 @@ public synchronized void stop() {
 			this.languageClient.dispose();
 		}
 
-		if (this.initializeFuture != null) {
-			this.initializeFuture.cancel(true);
-			this.initializeFuture = null;
-		}
-
 		this.serverCapabilities = null;
 		this.dynamicRegistrations.clear();