From 281781d091b76bd93433c3c678090a7142f1ab10 Mon Sep 17 00:00:00 2001 From: Jan Lahoda Date: Mon, 5 Apr 2021 23:33:35 +0200 Subject: [PATCH] When LSP Server fails to start too many times, don't try start it again. --- .../editor/lsp/LanguageServerImpl.java | 12 +- .../modules/lsp/client/LSPBindings.java | 71 +++++++-- .../modules/lsp/client/resources/error_16.png | Bin 0 -> 730 bytes .../modules/lsp/client/LSPBindingsTest.java | 140 ++++++++++++++++-- 4 files changed, 196 insertions(+), 27 deletions(-) create mode 100644 ide/lsp.client/src/org/netbeans/modules/lsp/client/resources/error_16.png diff --git a/cpplite/cpplite.editor/src/org/netbeans/modules/cpplite/editor/lsp/LanguageServerImpl.java b/cpplite/cpplite.editor/src/org/netbeans/modules/cpplite/editor/lsp/LanguageServerImpl.java index 27a39710a86f..877c6199d260 100644 --- a/cpplite/cpplite.editor/src/org/netbeans/modules/cpplite/editor/lsp/LanguageServerImpl.java +++ b/cpplite/cpplite.editor/src/org/netbeans/modules/cpplite/editor/lsp/LanguageServerImpl.java @@ -47,6 +47,7 @@ import org.netbeans.modules.cpplite.editor.spi.CProjectConfigurationProvider.ProjectConfiguration; import org.openide.filesystems.FileUtil; import org.openide.modules.Places; +import org.openide.util.Pair; /** * @@ -66,7 +67,7 @@ public class LanguageServerImpl implements LanguageServerProvider { private static final Logger LOG = Logger.getLogger(LanguageServerImpl.class.getName()); - private static final Map prj2Server = new HashMap<>(); + private static final Map> prj2Server = new HashMap<>(); @Override public LanguageServerDescription startServer(Lookup lookup) { @@ -88,7 +89,10 @@ public void preferenceChange(PreferenceChangeEvent evt) { String ccls = Utils.getCCLSPath(); String clangd = Utils.getCLANGDPath(); if (ccls != null || clangd != null) { - return prj2Server.computeIfAbsent(prj, (Project p) -> { + return prj2Server.compute(prj, (p, pair) -> { + if (pair != null && pair.first().isAlive()) { + return pair; + } try { List command = new ArrayList<>(); @@ -128,14 +132,14 @@ public void stateChanged(ChangeEvent e) { in = new CopyInput(in, System.err); out = new CopyOutput(out, System.err); } - return LanguageServerDescription.create(in, out, process); + return Pair.of(process, LanguageServerDescription.create(in, out, process)); } return null; } catch (IOException ex) { LOG.log(Level.FINE, null, ex); return null; } - }); + }).second(); } return null; } diff --git a/ide/lsp.client/src/org/netbeans/modules/lsp/client/LSPBindings.java b/ide/lsp.client/src/org/netbeans/modules/lsp/client/LSPBindings.java index 6744c6dbd6bf..06ce53400e46 100644 --- a/ide/lsp.client/src/org/netbeans/modules/lsp/client/LSPBindings.java +++ b/ide/lsp.client/src/org/netbeans/modules/lsp/client/LSPBindings.java @@ -22,6 +22,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.lang.ref.Reference; import java.lang.ref.ReferenceQueue; import java.lang.ref.WeakReference; import java.net.InetAddress; @@ -74,11 +75,13 @@ import org.netbeans.modules.lsp.client.spi.ServerRestarter; import org.netbeans.modules.lsp.client.spi.LanguageServerProvider; import org.netbeans.modules.lsp.client.spi.LanguageServerProvider.LanguageServerDescription; +import org.openide.awt.NotificationDisplayer; import org.openide.filesystems.FileObject; import org.openide.filesystems.FileUtil; import org.openide.modules.OnStop; import org.openide.util.ChangeSupport; import org.openide.util.Exceptions; +import org.openide.util.ImageUtilities; import org.openide.util.Lookup; import org.openide.util.NbBundle.Messages; import org.openide.util.RequestProcessor; @@ -95,10 +98,12 @@ public class LSPBindings { private static final int DELAY = 500; private static final int LSP_KEEP_ALIVE_MINUTES = 10; + private static final int INVALID_START_TIME = 1 * 60 * 1000; + private static final int INVALID_START_MAX_COUNT = 5; private static final RequestProcessor WORKER = new RequestProcessor(LanguageClientImpl.class.getName(), 1, false, false); private static final ChangeSupport cs = new ChangeSupport(LSPBindings.class); private static final Map lspKeepAlive = new IdentityHashMap<>(); - private static final Map>> project2MimeType2Server = new HashMap<>(); + private static final Map> project2MimeType2Server = new HashMap<>(); private static final Map> workspace2Extension2Server = new HashMap<>(); static { @@ -168,23 +173,28 @@ public static synchronized LSPBindings getBindingsImpl(Project prj, FileObject f URI uri = dir.toURI(); LSPBindings bindings = null; - WeakReference bindingsReference = + ServerDescription description = project2MimeType2Server.computeIfAbsent(uri, p -> new HashMap<>()) - .get(mimeType); + .computeIfAbsent(mimeType, m -> new ServerDescription()); - if(bindingsReference != null) { - bindings = bindingsReference.get(); + if (description.bindings != null) { + bindings = description.bindings.get(); } if (bindings != null && bindings.process != null && !bindings.process.isAlive()) { + startFailed(description, mimeType); bindings = null; } + if (description.failedCount >= INVALID_START_MAX_COUNT) { + return null; + } + if (bindings == null) { - bindings = buildBindings(prj, mimeType, dir, uri); + bindings = buildBindings(description, prj, mimeType, dir, uri); if (bindings != null) { - project2MimeType2Server.computeIfAbsent(uri, p -> new HashMap<>()) - .put(mimeType, new WeakReference<>(bindings)); + description.bindings = new WeakReference<>(bindings); + description.lastStartTimeStamp = System.currentTimeMillis(); WORKER.post(() -> cs.fireChange()); } } @@ -196,12 +206,34 @@ public static synchronized LSPBindings getBindingsImpl(Project prj, FileObject f return bindings != null ? bindings : null; } + @Messages({ + "# {0} - the mime type for which the LSP server failed to start", + "TITLE_FailedToStart=LSP Server for {0} failed to start too many times.", + "DETAIL_FailedToStart=The LSP Server failed to start too many times in a short time, and will not be restarted anymore." + }) + private static void startFailed(ServerDescription description, String mimeType) { + long timeStamp = System.currentTimeMillis(); + if (timeStamp - description.lastStartTimeStamp < INVALID_START_TIME) { + description.failedCount++; + if (description.failedCount == INVALID_START_MAX_COUNT) { + NotificationDisplayer.getDefault().notify(Bundle.TITLE_FailedToStart(mimeType), + ImageUtilities.loadImageIcon("/org/netbeans/modules/lsp/client/resources/error_16.png", false), + Bundle.DETAIL_FailedToStart(), + null); + } + } else { + description.failedCount = 0; + } + description.lastStartTimeStamp = timeStamp; + } + @SuppressWarnings({"AccessingNonPublicFieldOfAnotherObject", "ResultOfObjectAllocationIgnored"}) - private static LSPBindings buildBindings(Project prj, String mt, FileObject dir, URI baseUri) { + private static LSPBindings buildBindings(ServerDescription inDescription, Project prj, String mt, FileObject dir, URI baseUri) { MimeTypeInfo mimeTypeInfo = new MimeTypeInfo(mt); ServerRestarter restarter = () -> { synchronized (LSPBindings.class) { - WeakReference bRef = project2MimeType2Server.getOrDefault(baseUri, Collections.emptyMap()).remove(mt); + ServerDescription description = project2MimeType2Server.getOrDefault(baseUri, Collections.emptyMap()).remove(mt); + Reference bRef = description != null ? description.bindings : null; LSPBindings b = bRef != null ? bRef.get() : null; if (b != null) { @@ -219,6 +251,8 @@ private static LSPBindings buildBindings(Project prj, String mt, FileObject dir, } }; + boolean foundServer = false; + for (LanguageServerProvider provider : MimeLookup.getLookup(mt).lookupAll(LanguageServerProvider.class)) { final Lookup lkp = prj != null ? Lookups.fixed(prj, mimeTypeInfo, restarter) : Lookups.fixed(mimeTypeInfo, restarter); LanguageServerDescription desc = provider.startServer(lkp); @@ -228,6 +262,7 @@ private static LSPBindings buildBindings(Project prj, String mt, FileObject dir, if (b != null) { return b; } + foundServer = true; try { LanguageClientImpl lci = new LanguageClientImpl(); InputStream in = LanguageServerProviderAccessor.getINSTANCE().getInputStream(desc); @@ -249,6 +284,9 @@ private static LSPBindings buildBindings(Project prj, String mt, FileObject dir, } } } + if (foundServer) { + startFailed(inDescription, mt); + } return null; } @@ -328,7 +366,7 @@ public static synchronized Set getAllBindings() { project2MimeType2Server.values() .stream() .flatMap(n -> n.values().stream()) - .map(bindingRef -> bindingRef.get()) + .map(description -> description.bindings.get()) .filter(binding -> binding != null) .forEach(allBindings::add); workspace2Extension2Server.values() @@ -427,9 +465,9 @@ public static class Cleanup implements Runnable { @Override @SuppressWarnings("AccessingNonPublicFieldOfAnotherObject") public void run() { - for (Map> mime2Bindings : project2MimeType2Server.values()) { - for (WeakReference bRef : mime2Bindings.values()) { - LSPBindings b = bRef != null ? bRef.get() : null; + for (Map mime2Bindings : project2MimeType2Server.values()) { + for (ServerDescription description : mime2Bindings.values()) { + LSPBindings b = description.bindings != null ? description.bindings.get() : null; if (b != null && b.process != null) { b.process.destroy(); } @@ -488,4 +526,9 @@ public void run() { } } + private static class ServerDescription { + public long lastStartTimeStamp; + public int failedCount; + public Reference bindings; + } } diff --git a/ide/lsp.client/src/org/netbeans/modules/lsp/client/resources/error_16.png b/ide/lsp.client/src/org/netbeans/modules/lsp/client/resources/error_16.png new file mode 100644 index 0000000000000000000000000000000000000000..7d0ea445ad52de447e5ab7d483938a637ea84255 GIT binary patch literal 730 zcmV<00ww*4P)L* zh%lT1ssS0|2{Z&CfS5q6&4Pk4fByepY*<*x@bT6yhF|aAG5mh}mVrfBm;qz~hz9XN z;vjjDI*@vhK7arM8}JusvvFb~!}}{&7=FEe&F~wDf4+Lf@a6VxumK<%#0QCkOuMd0*D1pA3Kh{)Nl`0~!F*2M|DTFJKq|#J_+6cP%G}fejReJUk44frg+M0MrK% zKuj<%{0I6E8GizXxPZMqgXQ7FU>d|nR|C=q5I{^Y7qG%OPyh;jrKTnZb{QEkjhx^> z>R6%r00IcCXV0%+u{(f?haZ^I*jQK?Sb&(7m6hR6V`J3n;qz10bX9P&D=otnuiY=4goc9 z{`qqqKmf5oo%;=F$U2awzYiWPPSMf=hp;Fx;E(~x*>kR4V>tZ#ckEW6W{^IB0AfM% zGuRN2Pyb%KhBG%F1!~>~)ePbT1P~*7b_RI|NNM89&evg@K>#4Y0CcrqFDiIQj{pDw M07*qoM6N<$f_}v}MF0Q* literal 0 HcmV?d00001 diff --git a/ide/lsp.client/test/unit/src/org/netbeans/modules/lsp/client/LSPBindingsTest.java b/ide/lsp.client/test/unit/src/org/netbeans/modules/lsp/client/LSPBindingsTest.java index a23f42ba1da3..619dfa73b115 100644 --- a/ide/lsp.client/test/unit/src/org/netbeans/modules/lsp/client/LSPBindingsTest.java +++ b/ide/lsp.client/test/unit/src/org/netbeans/modules/lsp/client/LSPBindingsTest.java @@ -18,34 +18,44 @@ */ package org.netbeans.modules.lsp.client; +import java.awt.event.ActionListener; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; +import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.util.concurrent.atomic.AtomicInteger; +import javax.swing.Icon; +import javax.swing.JComponent; import static junit.framework.TestCase.assertNotNull; -import org.eclipse.lsp4j.ServerCapabilities; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; import org.junit.Test; import org.netbeans.api.editor.mimelookup.MimePath; -import org.netbeans.api.editor.mimelookup.MimeRegistration; import org.netbeans.api.project.FileOwnerQuery; -import org.netbeans.api.project.Project; import org.netbeans.junit.MockServices; import org.netbeans.modules.editor.mimelookup.MimeLookupCacheSPI; import org.netbeans.modules.lsp.client.spi.LanguageServerProvider; +import org.openide.awt.Notification; +import org.openide.awt.NotificationDisplayer; import org.openide.filesystems.FileObject; import org.openide.filesystems.FileUtil; import org.openide.filesystems.MIMEResolver; import org.openide.util.Lookup; import org.openide.util.lookup.Lookups; +import org.openide.util.lookup.ProxyLookup; +import org.openide.util.lookup.ServiceProvider; public class LSPBindingsTest { + private static final SettableProxyLookup MIME_LOOKUP = new SettableProxyLookup(); + @Test public void testGetBindingsOnNonProjectFolder() throws Exception { MockServices.setServices(MockMimeLookupCacheSPI.class, MockMimeResolver.class); + MIME_LOOKUP.setLookup(Lookups.fixed(new MockLSP())); + FileObject folder = FileUtil.createMemoryFileSystem().getRoot().createFolder("myfolder"); FileObject file = folder.createData("data.mock-txt"); assertEquals("application/mock-txt", file.getMIMEType()); @@ -55,11 +65,17 @@ public void testGetBindingsOnNonProjectFolder() throws Exception { assertNotNull("Bindings for the projectless file found", bindings); } + public static final class SettableProxyLookup extends ProxyLookup { + public void setLookup(Lookup l) { + setLookups(l); + } + } + public static final class MockMimeLookupCacheSPI extends MimeLookupCacheSPI { @Override public Lookup getLookup(MimePath mp) { assertEquals("application/mock-txt", mp.getPath()); - return Lookups.fixed(new MockLSP()); + return MIME_LOOKUP; } } @@ -82,11 +98,11 @@ public String findMIMEType(FileObject fo) { } } - static final class MockProcess extends Process { + static class BaseMockProcess extends Process { final ByteArrayInputStream in; final ByteArrayOutputStream out; - public MockProcess() { + public BaseMockProcess() { this.in = new ByteArrayInputStream(new byte[0]); this.out = new ByteArrayOutputStream(); } @@ -111,6 +127,26 @@ public int waitFor() throws InterruptedException { throw new InterruptedException(); } + @Override + public boolean isAlive() { + return true; + } + + @Override + public int exitValue() { + return 0; + } + + @Override + public void destroy() { + } + } + + static final class MockProcess extends BaseMockProcess { + + public MockProcess() { + } + @Override public boolean isAlive() { StackTraceElement[] stack = new Exception().getStackTrace(); @@ -120,13 +156,99 @@ public boolean isAlive() { return true; } + } + + @Test + public void testNoContinuousRestarts1() throws Exception { + class MockLSP implements LanguageServerProvider { + @Override + public LanguageServerDescription startServer(Lookup lookup) { + final BaseMockProcess process = new BaseMockProcess() { + @Override + public boolean isAlive() { + return false; + } + }; + return LanguageServerDescription.create(process.in, process.out, process); + } + } + + MockServices.setServices(MockMimeLookupCacheSPI.class, MockMimeResolver.class); + + MIME_LOOKUP.setLookup(Lookups.fixed(new MockLSP())); + + FileObject folder = FileUtil.createMemoryFileSystem().getRoot().createFolder("myfolder"); + FileObject file = folder.createData("data.mock-txt"); + assertEquals("application/mock-txt", file.getMIMEType()); + assertNull("No project owner", FileOwnerQuery.getOwner(file)); + + for (int i = 0; i < 5; i++) { + LSPBindings.getBindings(file); + } + + NotifyDisplayerImpl.called.set(0); + + LSPBindings bindings = LSPBindings.getBindings(file); + assertNull("No bindings after many failures", bindings); + assertEquals(1, NotifyDisplayerImpl.called.get()); + } + + @Test + public void testNoContinuousRestarts2() throws Exception { + class MockLSP implements LanguageServerProvider { + @Override + public LanguageServerDescription startServer(Lookup lookup) { + final BaseMockProcess process = new BaseMockProcess() { + @Override + public boolean isAlive() { + return false; + } + }; + OutputStream closed = new OutputStream() { + @Override + public void write(int b) throws IOException { + throw new IOException(); + } + }; + return LanguageServerDescription.create(process.in, closed, process); + } + } + + MockServices.setServices(MockMimeLookupCacheSPI.class, MockMimeResolver.class); + + MIME_LOOKUP.setLookup(Lookups.fixed(new MockLSP())); + + FileObject folder = FileUtil.createMemoryFileSystem().getRoot().createFolder("myfolder"); + FileObject file = folder.createData("data.mock-txt"); + assertEquals("application/mock-txt", file.getMIMEType()); + assertNull("No project owner", FileOwnerQuery.getOwner(file)); + + for (int i = 0; i < 5; i++) { + LSPBindings.getBindings(file); + } + + NotifyDisplayerImpl.called.set(0); + + LSPBindings bindings = LSPBindings.getBindings(file); + assertNull("No bindings after many failures", bindings); + assertEquals(1, NotifyDisplayerImpl.called.get()); + } + + @ServiceProvider(service=NotificationDisplayer.class, position=100) + public static class NotifyDisplayerImpl extends NotificationDisplayer { + + private static final AtomicInteger called = new AtomicInteger(); + @Override - public int exitValue() { - return 0; + public Notification notify(String title, Icon icon, String detailsText, ActionListener detailsAction, Priority priority) { + called.incrementAndGet(); + return null; } @Override - public void destroy() { + public Notification notify(String title, Icon icon, JComponent balloonDetails, JComponent popupDetails, Priority priority) { + throw new UnsupportedOperationException("Not supported yet."); } + } }