Skip to content

Commit

Permalink
fix: propagate LanguagServerWrapper startup exception to consumers (#…
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
ava-fred authored Sep 5, 2024
1 parent d17a02e commit 7ce9692
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 112 deletions.
17 changes: 0 additions & 17 deletions org.eclipse.lsp4e.test/fragment.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Expand Down Expand Up @@ -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">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
}

This file was deleted.

31 changes: 22 additions & 9 deletions org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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<>();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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();

Expand Down

0 comments on commit 7ce9692

Please sign in to comment.