Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: propagate LanguagServerWrapper startup exception correctly #1044

Merged
merged 2 commits into from
Sep 5, 2024
Merged

fix: propagate LanguagServerWrapper startup exception correctly #1044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
977f8fb
fix: propagate LanguagServerWrapper startup exception to consumers
ava-fred Jul 24, 2024
3473390
Merge branch 'eclipse:main' into master
ava-fred Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 0 additions & 17 deletions org.eclipse.lsp4e.test/fragment.xml
View file
Open in desktop
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
31 changes: 2 additions & 29 deletions org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/LanguageServerWrapperTest.java
View file
Open in desktop
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);
}
}
57 changes: 0 additions & 57 deletions ...lsp4e.test/src/org/eclipse/lsp4e/test/utils/MockConnectionProviderWithStartException.java
View file
Open in desktop

This file was deleted.

31 changes: 22 additions & 9 deletions org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java
View file
Open in desktop
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an exception has been occurred during start. The LS cannot be started again when the cause has been fixed (e.g. wrong path do LS binary).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could we worth explaining as well that "an Eclipse-base product can check startupFailed and call restart if the problem has been programmatically handled", and that otherwise "once the problem is fixed a restart of eclipse is needed". That would clarify as well the need for the startupFailed method for future readers of this class. My wording can most probably be improved :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

..but I am okay with it. This can/should be handled by the LS provider

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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this method called by anyone? What's the purpose of this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is to determine whether a startup attempt on this LanguageServerWrapper object failed and a call to restart() is needed. We will be using it in our project.

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
Loading