Skip to content

Commit

Permalink
fix: Prevent LanguageServers.forProject().anyMatching() from blocking
Browse files Browse the repository at this point in the history
  • Loading branch information
sebthom committed Jan 15, 2025
1 parent 871d73a commit fa4a00e
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,28 @@ public synchronized CompletableFuture<Hover> hover(HoverParams position) {
assertEquals("Should not have been any messages dispatched on UI thread", 0, uiDispatchCount.get());
}

@Test
public void testAnyMatchingIsNonBlocking() throws Exception {
// test with no LS available
long start = System.currentTimeMillis();
assertFalse(LanguageServers.forProject(project).anyMatching());
long duration = System.currentTimeMillis() - start;
assertTrue("LanguageServers.anyMatching() took too long: " + duration + "ms", duration < 100);

// test with one slow LS available
MockLanguageServer.INSTANCE.setTimeToProceedQueries(5_000);
var testFile1 = createUniqueTestFile(project, "");
var editor1 = openEditor(testFile1);
start = System.currentTimeMillis();
try {
assertTrue(LanguageServers.forProject(project).anyMatching());
duration = System.currentTimeMillis() - start;
assertTrue("LanguageServers.anyMatching() took too long: " + duration + "ms", duration < 100);
} finally {
editor1.getSite().getPage().closeEditor(editor1, false);
}
}

@Test
public void testNoMatchingServers() throws Exception {
final var hoverResponse = new Hover(List.of(Either.forLeft("HoverContent")), new Range(new Position(0, 0), new Position(0, 10)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -913,7 +913,8 @@ public void sendNotification(Consumer<LanguageServer> fn) {
}

/**
* Warning: this is a long running operation
* <b>IMPORTANT:</b> If the server isn't yet initialized this method will be
* blocking for up to 10 seconds!
*
* @return the server capabilities, or null if initialization job didn't
* complete
Expand Down
32 changes: 26 additions & 6 deletions org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServers.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.concurrent.CancellationException;
Expand Down Expand Up @@ -328,19 +329,38 @@ public LanguageServerProjectExecutor excludeInactive() {
@Override
protected List<CompletableFuture<@Nullable LanguageServerWrapper>> getServers() {
// Compute list of servers from project & filter
Collection<LanguageServerWrapper> startedWrappers = order(LanguageServiceAccessor.getStartedWrappers(project, getFilter(), !restartStopped));
final var wrappers = new ArrayList<CompletableFuture<@Nullable LanguageServerWrapper>>(startedWrappers.size());
for (LanguageServerWrapper wrapper : startedWrappers) {
wrappers.add(wrapper.getInitializedServer().thenApply(ls -> wrapper));
}
return wrappers;
return order(LanguageServiceAccessor.getStartedWrappersAsync(project, getFilter(), !restartStopped));
}
}

private static boolean isEmptyCollection(final Object obj) {
return (obj instanceof Collection<?> c && c.isEmpty());
}

/**
* @return a list of futures, reordered to prioritize the one matching the specified server definition,
* or in their original order if no match is found
*/
protected List<CompletableFuture<@Nullable LanguageServerWrapper>> order(
final Map<LanguageServerDefinition, CompletableFuture<@Nullable LanguageServerWrapper>> wrappers) {
if (serverDefinition == null || wrappers.size() < 2) {
return new ArrayList<>(wrappers.values());
}
final var result = new ArrayList<CompletableFuture<@Nullable LanguageServerWrapper>>(wrappers.size());
for (final var entry : wrappers.entrySet()) {
if (Objects.equals(serverDefinition, entry.getKey())) {
result.add(0, entry.getValue());
} else {
result.add(entry.getValue());
}
}
return result;
}

/**
* @return a list of wrappers, reordered to prioritize the one matching the specified server definition,
* or in their original order if no match is found
*/
protected Collection<LanguageServerWrapper> order(Collection<LanguageServerWrapper> wrappers) {
if (serverDefinition != null && wrappers.size() > 1) {
final var temp = new ArrayList<LanguageServerWrapper>(wrappers);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -171,16 +172,17 @@ public static void enableLanguageServerContentType(
* {@code capabilitesPredicate} does not test positive for the server's
* capabilities, {@code null} is returned.
*/
private static @Nullable CompletableFuture<LanguageServer> getInitializedLanguageServer(IResource resource,
private static CompletableFuture<@Nullable LanguageServer> getInitializedLanguageServer(IResource resource,
LanguageServerDefinition lsDefinition, @Nullable Predicate<ServerCapabilities> capabilitiesPredicate) {
LanguageServerWrapper wrapper = getLSWrapper(resource.getProject(), lsDefinition, resource.getFullPath());
if (capabilitiesComply(wrapper, capabilitiesPredicate)) {
return wrapper.getInitializedServer();
}
return null;
return capabilitiesComplyAsync(wrapper, capabilitiesPredicate)
.thenCompose(complies -> complies ? wrapper.getInitializedServer() : null);
}

/**
* <b>IMPORTANT:</b> If the server isn't yet initialized this method will be
* blocking for up to 10 seconds!
*
* Checks if the given {@code wrapper}'s capabilities comply with the given
* {@code capabilitiesPredicate}.
*
Expand All @@ -190,7 +192,7 @@ public static void enableLanguageServerContentType(
* @param capabilitiesPredicate
* predicate testing the capabilities of {@code wrapper}.
* @return The result of applying the capabilities of {@code wrapper} to
* {@code capabilitiesPredicate}, or {@code false} if
* {@code capabilitiesPredicate}, or {@code true} if
* {@code capabilitiesPredicate == null} or
* {@code wrapper.getServerCapabilities() == null}
*/
Expand All @@ -204,6 +206,26 @@ private static boolean capabilitiesComply(LanguageServerWrapper wrapper,
|| capabilitiesPredicate.test(castNonNull(wrapper.getServerCapabilities()));
}

/**
* Asynchronously checks if the given {@code wrapper}'s capabilities comply with the given
* {@code capabilitiesPredicate}.
*
* @param wrapper
* the server that's capabilities are tested with
* {@code capabilitiesPredicate}
* @param capabilitiesPredicate
* predicate testing the capabilities of {@code wrapper}.
* @return The result of applying the capabilities of {@code wrapper} to
* {@code capabilitiesPredicate}, or {@code true} if
* {@code capabilitiesPredicate == null}
*/
private static CompletableFuture<Boolean> capabilitiesComplyAsync(final LanguageServerWrapper wrapper,
final @Nullable Predicate<ServerCapabilities> capabilitiesPredicate) {
return capabilitiesPredicate == null //
? CompletableFuture.completedFuture(true) //
: wrapper.getServerCapabilitiesAsync().thenApply(capabilitiesPredicate::test);
}

/**
* TODO we need a similar method for generic IDocument (enabling non-IFiles)
*
Expand Down Expand Up @@ -389,16 +411,28 @@ public static LanguageServerWrapper startLanguageServer(LanguageServerDefinition
}
}

/**
* <b>IMPORTANT:</b> If the server isn't yet initialized this method will be
* blocking for up to 10 seconds per server!
*/
public static List<LanguageServerWrapper> getStartedWrappers(@Nullable Predicate<ServerCapabilities> request,
boolean onlyActiveLS) {
return getStartedWrappers(w -> true, request, onlyActiveLS);
}

/**
* <b>IMPORTANT:</b> If the server isn't yet initialized this method will be
* blocking for up to 10 seconds per server!
*/
public static List<LanguageServerWrapper> getStartedWrappers(@Nullable IProject project,
@Nullable Predicate<ServerCapabilities> request, boolean onlyActiveLS) {
return getStartedWrappers(w -> w.canOperate(project), request, onlyActiveLS);
}

/**
* <b>IMPORTANT:</b> If the server isn't yet initialized this method will be
* blocking for up to 10 seconds per server!
*/
public static List<LanguageServerWrapper> getStartedWrappers(IDocument document,
Predicate<ServerCapabilities> request, boolean onlyActiveLS) {
return getStartedWrappers(w -> w.canOperate(document), request, onlyActiveLS);
Expand All @@ -416,6 +450,35 @@ && capabilitiesComply(wrapper, capabilitiesPredicate)) {
return result;
}

public static Map<LanguageServerDefinition, CompletableFuture<@Nullable LanguageServerWrapper>> getStartedWrappersAsync(
final @Nullable Predicate<ServerCapabilities> request, final boolean onlyActiveLS) {
return getStartedWrappersAsync(w -> true, request, onlyActiveLS);
}

public static Map<LanguageServerDefinition, CompletableFuture<@Nullable LanguageServerWrapper>> getStartedWrappersAsync(
final @Nullable IProject project, final @Nullable Predicate<ServerCapabilities> request,
final boolean onlyActiveLS) {
return getStartedWrappersAsync(w -> w.canOperate(project), request, onlyActiveLS);
}

public static Map<LanguageServerDefinition, CompletableFuture<@Nullable LanguageServerWrapper>> getStartedWrappersAsync(final IDocument document,
final Predicate<ServerCapabilities> request, final boolean onlyActiveLS) {
return getStartedWrappersAsync(w -> w.canOperate(document), request, onlyActiveLS);
}

private static Map<LanguageServerDefinition, CompletableFuture<@Nullable LanguageServerWrapper>> getStartedWrappersAsync(
final Predicate<LanguageServerWrapper> canOperatePredicate,
final @Nullable Predicate<ServerCapabilities> capabilitiesPredicate, final boolean onlyActiveLS) {
final Map<LanguageServerDefinition, CompletableFuture<@Nullable LanguageServerWrapper>> result = new LinkedHashMap<>();
for (final LanguageServerWrapper wrapper : startedServers) {
if ((!onlyActiveLS || wrapper.isActive()) && canOperatePredicate.test(wrapper)) {
result.put(wrapper.serverDefinition, capabilitiesComplyAsync(wrapper, capabilitiesPredicate)
.thenApply(complies -> complies ? wrapper : null));
}
}
return result;
}

/**
* Returns {@code true} if there are running language servers satisfying a
* capability predicate. This does not start any matching language servers.
Expand Down

0 comments on commit fa4a00e

Please sign in to comment.