From f2967cbe9d3ee12982f04a2297aa9a2d3052dcb1 Mon Sep 17 00:00:00 2001 From: Vasili Gulevich Date: Wed, 7 Jun 2023 21:47:33 +0400 Subject: [PATCH] Stop language server on shutdown (#681) Note the use of Plafotm.getLog() - ILog can't be received from plug-in activator during platform shutdown. --- .../{test => }/LanguageServerWrapperTest.java | 54 ++++++- .../src/org/eclipse/lsp4e/test/AllTests.java | 1 + .../MockLanguageServerMultiRootFolders.java | 5 +- .../eclipse/lsp4e/LanguageServerWrapper.java | 146 +++++++++++------- 4 files changed, 150 insertions(+), 56 deletions(-) rename org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/{test => }/LanguageServerWrapperTest.java (55%) diff --git a/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/LanguageServerWrapperTest.java b/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/LanguageServerWrapperTest.java similarity index 55% rename from org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/LanguageServerWrapperTest.java rename to org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/LanguageServerWrapperTest.java index 2a8a3e033..311d3d301 100644 --- a/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/LanguageServerWrapperTest.java +++ b/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/LanguageServerWrapperTest.java @@ -9,21 +9,30 @@ * Contributors: * Markus Ofterdinger (SAP SE) - initial implementation *******************************************************************************/ -package org.eclipse.lsp4e.test; +package org.eclipse.lsp4e; import static org.eclipse.lsp4e.test.TestUtils.waitForAndAssertCondition; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.logging.Handler; +import java.util.logging.LogRecord; +import java.util.logging.Logger; import org.eclipse.core.resources.IFile; import org.eclipse.core.resources.IProject; import org.eclipse.core.runtime.CoreException; import org.eclipse.jdt.annotation.NonNull; -import org.eclipse.lsp4e.LanguageServerWrapper; -import org.eclipse.lsp4e.LanguageServiceAccessor; +import org.eclipse.lsp4e.test.AllCleanRule; +import org.eclipse.lsp4e.test.TestUtils; +import org.eclipse.lsp4e.tests.mock.MockLanguageServerMultiRootFolders; +import org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer; import org.eclipse.ui.IEditorPart; +import org.junit.Assert; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -63,4 +72,43 @@ public void testConnect() throws Exception { TestUtils.closeEditor(editor1, false); TestUtils.closeEditor(editor2, false); } + + @Test + public void doNotStopBusyDispatchers() throws Exception { + final Logger LOG = Logger.getLogger(StreamMessageProducer.class.getName()); + List logMessages = Collections.synchronizedList(new ArrayList<>()); + LOG.addHandler(new Handler() { + + @Override + public void publish(LogRecord record) { + logMessages.add(record.getMessage()); + } + + @Override + public void flush() { + + } + + @Override + public void close() throws SecurityException { + } + }); + IFile testFile1 = TestUtils.createFile(project1, "shouldUseExtension.lsptWithMultiRoot", ""); + IEditorPart editor1 = TestUtils.openEditor(testFile1); + + try { + @NonNull Collection wrappers = LanguageServiceAccessor.getLSWrappers(testFile1, request -> true); + LanguageServerWrapper wrapper = wrappers.iterator().next(); + assertEquals(1, wrappers.size()); + waitForAndAssertCondition(2_000, () -> MockLanguageServerMultiRootFolders.INSTANCE.isRunning()); + assertTrue(wrapper.isConnectedTo(testFile1.getLocationURI())); + logMessages.clear(); + wrapper.stopDispatcher(); + waitForAndAssertCondition(2_000, () -> !MockLanguageServerMultiRootFolders.INSTANCE.isRunning()); + Assert.assertEquals(Collections.emptyList(), logMessages); + } finally { + TestUtils.closeEditor(editor1, false); + } + + } } diff --git a/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/AllTests.java b/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/AllTests.java index f282db4ad..9584fc842 100644 --- a/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/AllTests.java +++ b/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/AllTests.java @@ -12,6 +12,7 @@ *******************************************************************************/ package org.eclipse.lsp4e.test; +import org.eclipse.lsp4e.LanguageServerWrapperTest; import org.eclipse.lsp4e.test.codeactions.CodeActionTests; import org.eclipse.lsp4e.test.color.ColorTest; import org.eclipse.lsp4e.test.commands.DynamicRegistrationTest; diff --git a/org.eclipse.lsp4e.tests.mock/src/org/eclipse/lsp4e/tests/mock/MockLanguageServerMultiRootFolders.java b/org.eclipse.lsp4e.tests.mock/src/org/eclipse/lsp4e/tests/mock/MockLanguageServerMultiRootFolders.java index 298787d22..a609b18d8 100644 --- a/org.eclipse.lsp4e.tests.mock/src/org/eclipse/lsp4e/tests/mock/MockLanguageServerMultiRootFolders.java +++ b/org.eclipse.lsp4e.tests.mock/src/org/eclipse/lsp4e/tests/mock/MockLanguageServerMultiRootFolders.java @@ -137,7 +137,10 @@ public U apply(Void v) { @Override public CompletableFuture initialize(InitializeParams params) { - return buildMaybeDelayedFuture(initializeResult); + return buildMaybeDelayedFuture(initializeResult).thenApply(result -> { + started = true; + return result; + }); } @Override diff --git a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java index 75cdcc402..9fe5a2cb7 100644 --- a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java +++ b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java @@ -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; @@ -54,6 +53,7 @@ import org.eclipse.core.resources.WorkspaceJob; import org.eclipse.core.runtime.Assert; import org.eclipse.core.runtime.CoreException; +import org.eclipse.core.runtime.ILog; import org.eclipse.core.runtime.IPath; import org.eclipse.core.runtime.IProgressMonitor; import org.eclipse.core.runtime.IStatus; @@ -111,6 +111,8 @@ public class LanguageServerWrapper { + private static final ILog LOG = Platform.getLog(LanguageServerWrapper.class); + private final IFileBufferListener fileBufferListener = new FileBufferListenerAdapter() { @Override public void bufferDisposed(IFileBuffer buffer) { @@ -158,7 +160,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 final AtomicReference> stopping = new AtomicReference>(null); private final ExecutorService dispatcher; @@ -201,14 +203,26 @@ private LanguageServerWrapper(@Nullable IProject project, @NonNull LanguageServe } void stopDispatcher() { - this.dispatcher.shutdownNow(); - - // Only really needed for testing - the listener (an instance of ConcurrentMessageProcessor) should exit - // as soon as the input stream from the LS is closed, and a cached thread pool will recycle idle - // threads after a 60 second timeout - or immediately in response to JVM shutdown. - // If we don't do this then a full test run will generate a lot of threads because we create new - // instances of this class for each test - this.listener.shutdownNow(); + if (dispatcher.isShutdown()) { + return; + } + try { + stop().get(6, TimeUnit.SECONDS); + } catch (InterruptedException e ) { + Thread.currentThread().interrupt(); + LOG.error(e.getLocalizedMessage(), e); + } catch (ExecutionException | TimeoutException e) { + LOG.error(String.format("Failed to stop %s in 6 second", this), e); //$NON-NLS-1$ + } finally { + this.dispatcher.shutdownNow(); + + // Only really needed for testing - the listener (an instance of ConcurrentMessageProcessor) should exit + // as soon as the input stream from the LS is closed, and a cached thread pool will recycle idle + // threads after a 60 second timeout - or immediately in response to JVM shutdown. + // If we don't do this then a full test run will generate a lot of threads because we create new + // instances of this class for each test + this.listener.shutdownNow(); + } } /** @@ -241,6 +255,7 @@ private List getRelevantWorkspaceFolders() { */ public synchronized void start() throws IOException { final var filesToReconnect = new HashMap(); + final CompletableFuture stopFuture; if (this.languageServer != null) { if (isActive()) { return; @@ -248,13 +263,32 @@ public synchronized void start() throws IOException { for (Entry entry : this.connectedDocuments.entrySet()) { filesToReconnect.put(entry.getKey(), entry.getValue().getDocument()); } - stop(); + stopFuture = stop(); } + } else { + stopFuture = CompletableFuture.completedFuture(null); } if (this.initializeFuture == null) { - final URI rootURI = getRootURI(); this.launcherFuture = new CompletableFuture<>(); - this.initializeFuture = CompletableFuture.supplyAsync(() -> { + this.initializeFuture = stopFuture.thenCompose(ignored -> initialize()); + initializeFuture.thenRunAsync(() -> { + FileBuffers.getTextFileBufferManager().addFileBufferListener(fileBufferListener); + watchProjects(); + for (Entry fileToReconnect : filesToReconnect.entrySet()) { + try { + connect(fileToReconnect.getKey(), fileToReconnect.getValue()); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + }); + } + } + + private CompletableFuture initialize() { + final URI rootURI = getRootURI(); + return CompletableFuture.supplyAsync(() -> { + synchronized (this) { if (LoggingStreamConnectionProviderProxy.shouldLog(serverDefinition.id)) { this.lspStreamProvider = new LoggingStreamConnectionProviderProxy( serverDefinition.createConnectionProvider(), serverDefinition.id); @@ -262,13 +296,13 @@ public synchronized void start() throws IOException { this.lspStreamProvider = serverDefinition.createConnectionProvider(); } initParams.setInitializationOptions(this.lspStreamProvider.getInitializationOptions(rootURI)); - try { - lspStreamProvider.start(); - } catch (IOException e) { - throw new RuntimeException(e); - } - return null; - }).thenRun(() -> { + } + try { + lspStreamProvider.start(); + } catch (IOException e) { + throw new RuntimeException(e); + } + synchronized (this) { languageClient = serverDefinition.createLanguageClient(); initParams.setProcessId((int) ProcessHandle.current().pid()); @@ -298,33 +332,20 @@ public synchronized void start() throws IOException { this.languageServer = launcher.getRemoteProxy(); languageClient.connect(languageServer, this); this.launcherFuture = launcher.startListening(); - }) - .thenCompose(unused -> initServer(rootURI)) - .thenAccept(res -> { + } + return null; + }, dispatcher).thenCompose(unused -> initServer(rootURI)).thenAccept(res -> { + synchronized(this) { serverCapabilities = res.getCapabilities(); this.initiallySupportsWorkspaceFolders = supportsWorkspaceFolders(serverCapabilities); - }).thenRun(() -> { this.languageServer.initialized(new InitializedParams()); - }).thenRun(() -> { - final Map toReconnect = filesToReconnect; - initializeFuture.thenRunAsync(() -> { - watchProjects(); - for (Entry fileToReconnect : toReconnect.entrySet()) { - try { - connect(fileToReconnect.getKey(), fileToReconnect.getValue()); - } catch (IOException e) { - throw new RuntimeException(e); - } - } - }); - FileBuffers.getTextFileBufferManager().addFileBufferListener(fileBufferListener); - }).exceptionally(e -> { - LanguageServerPlugin.logError(e); - initializeFuture.completeExceptionally(e); - stop(); - return null; - }); - } + } + }).exceptionally(e -> { + LanguageServerPlugin.logError(e); + initializeFuture.completeExceptionally(e); + stop(); + return null; + }); } private CompletableFuture initServer(final URI rootURI) { @@ -427,11 +448,13 @@ boolean isWrapperFor(LanguageServer server) { return server == this.languageServer; } - public synchronized void stop() { - final boolean alreadyStopping = this.stopping.getAndSet(true); - if (alreadyStopping) { - return; + public synchronized CompletableFuture stop() { + CompletableFuture result = new CompletableFuture(); + CompletableFuture alreadyStopping = this.stopping.compareAndExchange(null, result); + if (alreadyStopping != null) { + return alreadyStopping; } + try { removeStopTimerTask(); if (this.initializeFuture != null) { this.initializeFuture.cancel(true); @@ -454,7 +477,7 @@ public synchronized void stop() { } catch (InterruptedException ex) { Thread.currentThread().interrupt(); } catch (Exception ex) { - LanguageServerPlugin.logError(ex); + LOG.error(ex.getLocalizedMessage(), ex); } } @@ -469,10 +492,18 @@ public synchronized void stop() { if (provider != null) { provider.stop(); } - this.stopping.set(false); }; - CompletableFuture.runAsync(shutdownKillAndStopFutureAndProvider); + CompletableFuture.runAsync(shutdownKillAndStopFutureAndProvider, dispatcher).whenComplete((ignored, error) -> { + if (error != null) { + result.completeExceptionally(error); + } else { + result.complete(ignored); + } + if (!this.stopping.compareAndSet(result, null)) { + LOG.error("Unexpected concurrent stop", new IllegalStateException()); //$NON-NLS-1$ + } + }); this.launcherFuture = null; this.lspStreamProvider = null; @@ -483,6 +514,10 @@ public synchronized void stop() { this.languageServer = null; FileBuffers.getTextFileBufferManager().removeFileBufferListener(fileBufferListener); + } catch (Exception e) { + result.completeExceptionally(e); + } + return result; } public @Nullable CompletableFuture<@NonNull LanguageServerWrapper> connect(IDocument document, @NonNull IFile file) @@ -605,12 +640,14 @@ public CompletableFuture disconnect(URI uri) { if (documentListener != null) { documentListener.getDocument().removeDocumentListener(documentListener); documentClosedFuture = documentListener.documentClosed(); + } else { + documentClosedFuture = CompletableFuture.completedFuture(null); } if (this.connectedDocuments.isEmpty()) { if (this.serverDefinition.lastDocumentDisconnectedTimeout != 0) { startStopTimerTask(); } else { - stop(); + documentClosedFuture = documentClosedFuture.thenCompose(ignored -> this.stop()); } } return documentClosedFuture; @@ -1123,4 +1160,9 @@ private boolean isValid(WorkspaceFolder wsFolder) { } + @Override + public String toString() { + return serverDefinition.id; + } + } \ No newline at end of file