From 7ce9692bfaec0651fd344ec185eb5b25d571e1d1 Mon Sep 17 00:00:00 2001
From: ava-fred <52921434+ava-fred@users.noreply.github.com>
Date: Thu, 5 Sep 2024 10:55:49 +0200
Subject: [PATCH] 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 | 17 ------
.../lsp4e/test/LanguageServerWrapperTest.java | 31 +---------
...kConnectionProviderWithStartException.java | 57 -------------------
.../eclipse/lsp4e/LanguageServerWrapper.java | 31 +++++++---
4 files changed, 24 insertions(+), 112 deletions(-)
delete mode 100644 org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/utils/MockConnectionProviderWithStartException.java
diff --git a/org.eclipse.lsp4e.test/fragment.xml b/org.eclipse.lsp4e.test/fragment.xml
index 2d6047a8f..a6275db8c 100644
--- a/org.eclipse.lsp4e.test/fragment.xml
+++ b/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">
-
-
-
-
@@ -222,12 +211,6 @@
name="Test Content-Type associated with Singleton LS"
priority="normal">
-
-
diff --git a/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/LanguageServerWrapperTest.java b/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/LanguageServerWrapperTest.java
index b8d58bddb..8337a30c4 100644
--- a/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/LanguageServerWrapperTest.java
+++ b/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 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);
- }
}
diff --git a/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/utils/MockConnectionProviderWithStartException.java b/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/utils/MockConnectionProviderWithStartException.java
deleted file mode 100644
index c92b13bfe..000000000
--- a/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/utils/MockConnectionProviderWithStartException.java
+++ /dev/null
@@ -1,57 +0,0 @@
-/*******************************************************************************
- * 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 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++;
- }
-}
diff --git a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java
index 9c692e31c..31b117c98 100644
--- a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java
+++ b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java
@@ -242,8 +242,11 @@ private List 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();