Skip to content

Commit

Permalink
Fix quick disconnect (eclipse#681)
Browse files Browse the repository at this point in the history 
Asynchronous nature of stop() helps with quick reaction to closed
editors. An attempt to make it asynchronous confused isActive() check.
This change reverts the attempt, and allows for monitoring of stopping
processing via a returned Future.

Note the use of Plafotm.getLog() - ILog can't be received from plugin
activator during platform shutdown.
  • Loading branch information
@basilevs
basilevs committed Jun 6, 2023
1 parent a8d184e commit b9c8949
Show file tree
Hide file tree
Showing 4 changed files with 98 additions and 70 deletions.
3 changes: 2 additions & 1 deletion org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/LanguageServerWrapperTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,8 @@ public void close() throws SecurityException {
waitForAndAssertCondition(2_000, () -> MockLanguageServerMultiRootFolders.INSTANCE.isRunning());
assertTrue(wrapper.isConnectedTo(testFile1.getLocationURI()));
logMessages.clear();
wrappers.forEach(LanguageServerWrapper::stopDispatcher);
LanguageServiceAccessor.clearStartedServers();
wrapper.stopDispatcher();
waitForAndAssertCondition(2_000, () -> !MockLanguageServerMultiRootFolders.INSTANCE.isRunning());
Assert.assertEquals(Collections.emptyList(), logMessages);
} finally {
Expand Down
2 changes: 1 addition & 1 deletion org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerPlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public void start(BundleContext context) throws Exception {
@Override
public void stop(BundleContext context) throws Exception {
plugin = null;
LanguageServiceAccessor.shutdownAllDispatchers();
LanguageServiceAccessor.clearStartedServers();
super.stop(context);
}

Expand Down
136 changes: 78 additions & 58 deletions org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Consumer;
import java.util.function.Function;
Expand Down Expand Up @@ -158,7 +157,7 @@ public void dirtyStateChanged(IFileBuffer buffer, boolean isDirty) {
private ServerCapabilities serverCapabilities;
private final Timer timer = new Timer("Stop Language Server Task Processor"); //$NON-NLS-1$
private TimerTask stopTimerTask;
private AtomicBoolean stopping = new AtomicBoolean(false);
private AtomicReference<CompletableFuture<Void>> stopping = new AtomicReference<CompletableFuture<Void>>(null);

private final ExecutorService dispatcher;

Expand Down Expand Up @@ -201,8 +200,16 @@ private LanguageServerWrapper(@Nullable IProject project, @NonNull LanguageServe
}

void stopDispatcher() {
if (dispatcher.isShutdown()) {
return;
}
try {
stop();
stop().get(6, TimeUnit.SECONDS);
} catch (InterruptedException e ) {
Thread.currentThread().interrupt();
LanguageServerPlugin.logError(e);
} catch (ExecutionException | TimeoutException e) {
Platform.getLog(LanguageServerWrapper.class).error(String.format("Failed to stop %s in 6 second", this), e); //$NON-NLS-1$
} finally {
this.dispatcher.shutdownNow();

Expand Down Expand Up @@ -253,7 +260,7 @@ public synchronized void start() throws IOException {
for (Entry<URI, DocumentContentSynchronizer> entry : this.connectedDocuments.entrySet()) {
filesToReconnect.put(entry.getKey(), entry.getValue().getDocument());
}
stopFuture = CompletableFuture.runAsync(this::stop, dispatcher);
stopFuture = stop();
}
} else {
stopFuture = CompletableFuture.completedFuture(null);
Expand Down Expand Up @@ -438,66 +445,76 @@ boolean isWrapperFor(LanguageServer server) {
return server == this.languageServer;
}

public void stop() {
final Future<?> serverFuture;
final StreamConnectionProvider provider;
final LanguageServer languageServerInstance;
synchronized(this) {
final boolean alreadyStopping = this.stopping.getAndSet(true);
if (alreadyStopping) {
return;
}
removeStopTimerTask();
if (this.initializeFuture != null) {
this.initializeFuture.cancel(true);
this.initializeFuture = null;
}

this.serverCapabilities = null;
this.dynamicRegistrations.clear();

serverFuture = this.launcherFuture;
provider = this.lspStreamProvider;
languageServerInstance = this.languageServer;
ResourcesPlugin.getWorkspace().removeResourceChangeListener(workspaceFolderUpdater);
public synchronized CompletableFuture<Void> stop() {
CompletableFuture<Void> result = new CompletableFuture<Void>();
CompletableFuture<Void> alreadyStopping = this.stopping.compareAndExchange(null, result);
if (alreadyStopping != null) {
return alreadyStopping;
}
try {
removeStopTimerTask();
if (this.initializeFuture != null) {
this.initializeFuture.cancel(true);
this.initializeFuture = null;
}

if (languageServerInstance != null) {
CompletableFuture<Object> shutdown = languageServerInstance.shutdown();
try {
shutdown.get(5, TimeUnit.SECONDS);
} catch (InterruptedException ex) {
Thread.currentThread().interrupt();
} catch (Exception ex) {
LanguageServerPlugin.logError(ex);
this.serverCapabilities = null;
this.dynamicRegistrations.clear();

final Future<?> serverFuture = this.launcherFuture;
final StreamConnectionProvider provider = this.lspStreamProvider;
final LanguageServer languageServerInstance = this.languageServer;
ResourcesPlugin.getWorkspace().removeResourceChangeListener(workspaceFolderUpdater);

Runnable shutdownKillAndStopFutureAndProvider = () -> {
if (languageServerInstance != null) {
CompletableFuture<Object> shutdown = languageServerInstance.shutdown();
try {
shutdown.get(5, TimeUnit.SECONDS);
} catch (InterruptedException ex) {
Thread.currentThread().interrupt();
} catch (Exception ex) {
LanguageServerPlugin.logError(ex);
}
}
}

if (serverFuture != null) {
serverFuture.cancel(true);
}
if (serverFuture != null) {
serverFuture.cancel(true);
}

if (languageServerInstance != null) {
languageServerInstance.exit();
}
if (languageServerInstance != null) {
languageServerInstance.exit();
}

if (provider != null) {
provider.stop();
}
if (provider != null) {
provider.stop();
}
};

synchronized(this) {
this.stopping.set(false);
CompletableFuture.runAsync(shutdownKillAndStopFutureAndProvider, dispatcher).whenComplete((ignored, error) -> {
if (error != null) {
result.completeExceptionally(error);
} else {
result.complete(ignored);
}
if (!this.stopping.compareAndSet(result, null)) {
LanguageServerPlugin.logError(new IllegalStateException("Unexpected concurrent stop")); //$NON-NLS-1$
}
});

this.launcherFuture = null;
this.lspStreamProvider = null;
this.launcherFuture = null;
this.lspStreamProvider = null;

while (!this.connectedDocuments.isEmpty()) {
disconnect(this.connectedDocuments.keySet().iterator().next());
}
this.languageServer = null;
while (!this.connectedDocuments.isEmpty()) {
disconnect(this.connectedDocuments.keySet().iterator().next());
}
this.languageServer = null;

FileBuffers.getTextFileBufferManager().removeFileBufferListener(fileBufferListener);
FileBuffers.getTextFileBufferManager().removeFileBufferListener(fileBufferListener);
} catch (Exception e) {
result.completeExceptionally(e);
}
return result;
}

public @Nullable CompletableFuture<@NonNull LanguageServerWrapper> connect(IDocument document, @NonNull IFile file)
Expand Down Expand Up @@ -615,8 +632,8 @@ private boolean supportsWorkspaceFolderCapability() {
* @return null if not disconnection has happened, a future tracking the disconnection state otherwise
*/
public CompletableFuture<Void> disconnect(URI uri) {
@Nullable DocumentContentSynchronizer documentListener = this.connectedDocuments.remove(uri);
CompletableFuture<Void> documentClosedFuture;
DocumentContentSynchronizer documentListener = this.connectedDocuments.remove(uri);
CompletableFuture<Void> documentClosedFuture = null;
if (documentListener != null) {
documentListener.getDocument().removeDocumentListener(documentListener);
documentClosedFuture = documentListener.documentClosed();
Expand All @@ -627,9 +644,7 @@ public CompletableFuture<Void> disconnect(URI uri) {
if (this.serverDefinition.lastDocumentDisconnectedTimeout != 0) {
startStopTimerTask();
} else {
documentClosedFuture = documentClosedFuture.thenRunAsync(() -> {
this.stop();
}, dispatcher);
CompletableFuture.allOf(documentClosedFuture, stop());
}
}
return documentClosedFuture;
Expand Down Expand Up @@ -1139,4 +1154,9 @@ private boolean isValid(WorkspaceFolder wsFolder) {

}

@Override
public String toString() {
return serverDefinition.id;
}

}
27 changes: 17 additions & 10 deletions org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServiceAccessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,10 @@
import org.eclipse.core.resources.IFile;
import org.eclipse.core.resources.IProject;
import org.eclipse.core.resources.IResource;
import org.eclipse.core.runtime.ILog;
import org.eclipse.core.runtime.IPath;
import org.eclipse.core.runtime.Path;
import org.eclipse.core.runtime.Platform;
import org.eclipse.core.runtime.content.IContentDescription;
import org.eclipse.core.runtime.content.IContentType;
import org.eclipse.jdt.annotation.NonNull;
Expand All @@ -63,19 +65,28 @@ private LanguageServiceAccessor() {
// this class shouldn't be instantiated
}

private static final String FAILED_TO_STOP_SERVER = "Failed to stop language server %s"; // //$NON-NLS-1$


private static final Set<@NonNull LanguageServerWrapper> startedServers = new CopyOnWriteArraySet<>();
private static final Map<StreamConnectionProvider, LanguageServerDefinition> providersToLSDefinitions = new HashMap<>();
private static final ILog LOG = Platform.getLog(LanguageServiceAccessor.class);


/**
* This is meant for test code to clear state that might have leaked from other
* tests. It isn't meant to be used in production code.
* tests. It isn't meant to be used in production code, except for Eclipse shutdown.
*/
public static void clearStartedServers() {
startedServers.removeIf(server -> {
server.stop();
server.stopDispatcher();
return true;
});
startedServers.removeIf((wrapper) -> {
try {
wrapper.stopDispatcher();
} catch (Exception e) {
LOG.error(String.format(FAILED_TO_STOP_SERVER, wrapper), e);
}
return true;
}
);
}

/**
Expand Down Expand Up @@ -600,9 +611,5 @@ protected static LanguageServerDefinition getLSDefinition(@NonNull StreamConnect
}
return CompletableFuture.completedFuture(Collections.emptyList());
}

static void shutdownAllDispatchers() {
startedServers.forEach(LanguageServerWrapper::stopDispatcher);
}
}

0 comments on commit b9c8949

Please sign in to comment.