From 142fa28c98a878e35f45fd6a09f6a2e090877e58 Mon Sep 17 00:00:00 2001 From: Fredi Pfister Date: Wed, 24 Jul 2024 15:47:48 +0200 Subject: [PATCH] fix: propagate LanguagServerWrapper startup exception to consumers 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 | 28 ++++++--- 4 files changed, 21 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 d44c8fa28..c64c7b255 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,14 +14,14 @@ 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.io.IOException; 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; @@ -30,7 +30,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; @@ -110,30 +109,4 @@ public void testStopAndActive() throws CoreException, IOException, AssertionErro } } - @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 e9b7e61c1..45bf73cb3 100644 --- a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java +++ b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java @@ -241,8 +241,8 @@ 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. */ public synchronized void start() { start(false); @@ -278,7 +278,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<>(); @@ -356,7 +356,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 +486,13 @@ public synchronized boolean isActive() { return this.launcherFuture != null && !this.launcherFuture.isDone() && !this.launcherFuture.isCancelled(); } + /** + * @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 +528,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 +546,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();