eclipse / lsp4e

Language Server Protocol support in Eclipse IDE
Eclipse Public License 2.0
66 stars 54 forks source link

Resource exhaustion with many files with annotations #392

Open ahmedneilhussain opened 1 year ago

ahmedneilhussain commented 1 year ago

We've just experienced a problem (or more likely managed to reproduce reliably a sporadic bug) where we experience a weird UI sluggishness and then a load of out-of-OS-handle errors on Windows.

We think the culprit is https://github.com/eclipse/lsp4e/blob/5a75b544c083ab6b89ef11851dc07d2fcbe556ed/org.eclipse.lsp4e/src/org/eclipse/lsp4e/ConnectDocumentToLanguageServerSetupParticipant.java#L68

When the LS starts up for a project, it can report project-wide diagnostics, even for files that are not open. To do so the file is temporarily opened at https://github.com/eclipse/lsp4e/blob/5a75b544c083ab6b89ef11851dc07d2fcbe556ed/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/diagnostics/LSPDiagnosticsToMarkers.java#L145

It seems that when a buffer opens, even temporarily, ConnectDocumentToLanguageServerSetupParticipant will get run and submit an Eclipse Job. If the user is running without 'always run in background' set, then these jobs each pop up a progress dialog. Often these dialogs are never seen because the job completes so fast.

However, in the circumstances we have witnessed, with many many 'offline' files getting temporarily opened and progress dialogs created in a short space of time, that Eclipse can exhaust the OS handles for open shells.

ahmedneilhussain commented 1 year ago
org.eclipse.swt.SWTError: No more handles
      at org.eclipse.swt.SWT.error(SWT.java:4944)
      at org.eclipse.swt.SWT.error(SWT.java:4833)
      at org.eclipse.swt.SWT.error(SWT.java:4804)
      at org.eclipse.swt.widgets.Widget.error(Widget.java:447)
      at org.eclipse.swt.widgets.Control.createHandle(Control.java:718)
      at org.eclipse.swt.widgets.Scrollable.createHandle(Scrollable.java:145)
      at org.eclipse.swt.widgets.Composite.createHandle(Composite.java:294)
      at org.eclipse.swt.widgets.Decorations.createHandle(Decorations.java:362)
      at org.eclipse.swt.widgets.Shell.createHandle(Shell.java:613)
      at org.eclipse.swt.widgets.Control.createWidget(Control.java:746)
      at org.eclipse.swt.widgets.Scrollable.createWidget(Scrollable.java:160)
      at org.eclipse.swt.widgets.Decorations.createWidget(Decorations.java:371)
      at org.eclipse.swt.widgets.Shell.<init>(Shell.java:302)
      at org.eclipse.swt.widgets.Shell.<init>(Shell.java:381)
      at org.eclipse.jface.window.Window.createShell(Window.java:487)
      at org.eclipse.jface.window.Window.create(Window.java:430)
      at org.eclipse.jface.dialogs.Dialog.create(Dialog.java:1094)
      at org.eclipse.jface.dialogs.ProgressMonitorDialog.aboutToRun(ProgressMonitorDialog.java:544)
      at org.eclipse.ui.internal.progress.ProgressMonitorFocusJobDialog.show(ProgressMonitorFocusJobDialog.java:162)
      at org.eclipse.ui.internal.progress.ProgressManager.showInDialog(ProgressManager.java:975)
      at org.eclipse.ui.internal.progress.ProgressManager$1$1.runInUIThread(ProgressManager.java:480)
      at org.eclipse.ui.progress.UIJob.lambda$0(UIJob.java:95)
      at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40)
      at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:185)
      at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4035)
      at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3635)
      at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1155)
      at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338)
      at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1046)
      at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:155)
      at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:644)
      at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338)
      at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:551)
      at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:156)
      at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:152)
      at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:203)
      at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:136)
      at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
      at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:401)
      at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:255)
      at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
      at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
      at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
      at java.base/java.lang.reflect.Method.invoke(Method.java:566)
      at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:659)
      at org.eclipse.equinox.launcher.Main.basicRun(Main.java:596)
      at org.eclipse.equinox.launcher.Main.run(Main.java:1467)
      at org.eclipse.equinox.launcher.Main.main(Main.java:1440)
ahmedneilhussain commented 1 year ago

Example Sleak extract. at org.eclipse.jface.dialogs.ProgressMonitorDialog.aboutToRun(ProgressMonitorDialog.java:544) Is the common line to almost all Sleak entries. When we ran out of handles Sleak measured ~ 800 instances of Shell.

java.lang.Error: Created widget of type: Composite
    at org.eclipse.swt.internal.WidgetSpy$NonDisposedWidgetTracker.widgetCreated(WidgetSpy.java:92)
    at org.eclipse.swt.internal.WidgetSpy.widgetCreated(WidgetSpy.java:61)
    at org.eclipse.swt.widgets.Widget.notifyCreationTracker(Widget.java:2496)
    at org.eclipse.swt.widgets.Widget.<init>(Widget.java:166)
    at org.eclipse.swt.widgets.Control.<init>(Control.java:114)
    at org.eclipse.swt.widgets.Scrollable.<init>(Scrollable.java:85)
    at org.eclipse.swt.widgets.Composite.<init>(Composite.java:99)
    at org.eclipse.ui.internal.progress.ProgressMonitorJobsDialog.createButtonBar(ProgressMonitorJobsDialog.java:176)
    at org.eclipse.jface.dialogs.IconAndMessageDialog.createDialogAndButtonArea(IconAndMessageDialog.java:228)
    at org.eclipse.jface.dialogs.IconAndMessageDialog.createContents(IconAndMessageDialog.java:206)
    at org.eclipse.jface.window.Window.create(Window.java:431)
    at org.eclipse.jface.dialogs.Dialog.create(Dialog.java:1094)
    at org.eclipse.jface.dialogs.ProgressMonitorDialog.aboutToRun(ProgressMonitorDialog.java:544)
    at org.eclipse.ui.internal.progress.ProgressMonitorFocusJobDialog.show(ProgressMonitorFocusJobDialog.java:162)
    at org.eclipse.ui.internal.progress.ProgressManager.showInDialog(ProgressManager.java:975)
    at org.eclipse.ui.internal.progress.ProgressManager$1$1.runInUIThread(ProgressManager.java:480)
    at org.eclipse.ui.progress.UIJob.lambda$0(UIJob.java:95)
    at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40)
    at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:185)
    at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4035)
    at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3635)
    at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1155)
    at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338)
    at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1046)
    at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:155)
    at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:644)
    at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338)
    at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:551)
    at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:156)
    at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:152)
    at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:203)
    at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:136)
    at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
    at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:401)
    at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:255)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:566)
    at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:659)
    at org.eclipse.equinox.launcher.Main.basicRun(Main.java:596)
    at org.eclipse.equinox.launcher.Main.run(Main.java:1467)
    at org.eclipse.equinox.launcher.Main.main(Main.java:1440)
ahmedneilhussain commented 1 year ago

Can any Eclipse experts comment on this? Having Preferences > General > Always run in background prevents Eclipse from spawning separate dialogs for the progress bars, so that's a workaround.

Is there anything we can do in the relevant bit of the code to prevent this problem? Presumably we could use other background thread mechanisms for the async part, but I guess the UI Job is used specifically so the user receives feedback.

It feels maybe not quite right to have this UI progress bars generated for something that happens implicitly, in the background, as it does when diagnostics for unopened files are created.

mickaelistria commented 1 year ago

It seems like a dialog is closed by not disposed, so the Shell and all its children handles are still there. Which version of Eclipse Platform are you using? I suspect the issue may come from Platform rather than LSP4E.

rubenporras commented 1 year ago

I am curious, about the topic, since I expected I could have a similar problem and have not noticed it, a typical workspace for our DSLs has about 15000 markers. I am surprised because I put a couple of breakpoints in the class ConnectDocumentToLanguageServerSetupParticipant but they where never hit. I think that class is not triggered for our setup, even though I did nothing special to disable it, and I do not know why. Maybe it would be a solution for you, to disable the class (provided we would know how)?

mickaelistria commented 1 year ago

I think if you disable ConnectDocumentToLanguageServerSetupParticipant , then didChange and other document synchronization methods are not sent by default unless you explicitly bind the file to the language server later on.

Note that some plugins (eg WTP) tend to load many documents to perform validation; instead of working on raw text content. Loading many documents would cause many ConnectDocumentToLanguageServerSetupParticipant to be instantiated. This should be fine in theory.

ahmedneilhussain commented 1 year ago

I am curious, about the topic, since I expected I could have a similar problem and have not noticed it, a typical workspace for our DSLs has about 15000 markers. I am surprised because I put a couple of breakpoints in the class ConnectDocumentToLanguageServerSetupParticipant but they where never hit. I think that class is not triggered for our setup, even though I did nothing special to disable it, and I do not know why. Maybe it would be a solution for you, to disable the class (provided we would know how)?

We think the problem may be not so much the number of markers, as the number of files that have markers - i.e. 1 file with 15000 markers wouldn't be problematic, but 15000 files with 1 marker would be.

We're not sure, though. I'm trying to write a torture test (another one!) to reproduce the issue.

Also, do you have always run in the background enabled in the Eclipse preferences? Both Tom and I do, but the customer that experienced the issue does not. When they enabled that option, that prevented the problem, since Eclipse replaces all the top-level 'modal-ish' progress dialogs with inline progress bars in the view in the main shell.

rubenporras commented 1 year ago

We have about 5000 files with markers, so I guess it would also be a problem for us if we would trigger this code, but lucky for us the code is somehow not triggered.

rubenporras commented 1 year ago

I think if you disable ConnectDocumentToLanguageServerSetupParticipant , then didChange and other document synchronization methods are not sent by default unless you explicitly bind the file to the language server later on.

And how is it disabled?

mickaelistria commented 1 year ago

And how is it disabled?

There is no option to disable that in LSP4E, and I don't think there should be one. This participant (and others) would get skipped if you use a different TextFileBufferManager for documents I believe, a factory that ignore participants.

ahmedneilhussain commented 1 year ago

I've added a torture test and can now reproduce this at will on Windows.

See https://github.com/eclipse/lsp4e/pull/420 Run the diagnostics test (for convenience I've Ignored the other test cases in this class): the test creates 1000 dummy files with a single diagnostic on each.

As it's easiest to play around with it manually (not least to change the preference) I've added a brute force SWT event loop at the bottom of the file (could be just a Thread.sleep() on Windows, except my primary dev platform is MacOS and the test runner is also the main/UI thread on MacOS...) so that the UI stays up and you can interact with it.

Every time you open one of the files the mock test server will send all 1000 diagnostics again.

That makes it easy to demonstrate interactively:

On windows:

  1. Start the test, launching inferior eclipse (with 'always run in background' enabled)
  2. As the test automatically opens 1 file, to start the language server, and send the 1000 diagnostics
  3. This will succeed (and reasonably quickly), as the jobs are running in the background and not creating top level shells.
  4. Uncheck 'always run in background' from Preferences.> General
  5. Open an editor for another of the dummy files, triggering the diagnostics to be resent, but this time with jobs running in the foreground.
  6. Eclipse blows up with the No More Handles problem.
ahmedneilhussain commented 1 year ago

I ran this with Windows 10 Pro and Eclipse 2022-06 (4.24), and played around with the number of files - 250 was OK, 500 blew up.

I also checked with Sleak - if you set the number of files to 250 and check with Sleak, leaving enough time for the diagnostics to be created and the jobs to all complete (you can tell this by the way the cursor stops flickering over the Project Explorer view!), then you can verify that there are no handles leaked, per se: they do eventually get cleared up. The problem seems to be that we just create too many of them. I think the dialogs/shells may stick around for a long time in a 'closed but not disposed' state.

ahmedneilhussain commented 1 year ago

sleakout_001.txt

See attached sleak output - this was captured midway through a run (since as noted above the shells do get cleaned up if you let it run through and you don't hit the exhaustion limit). You can see that there are a lot of shells with the text 'Initialise Language Servers', so I think Tom was right and this is coming from the ConnectDocumentToLanguageServerSetupParticipant.

ahmedneilhussain commented 1 year ago

I don't know why @rubenporras couldn't hit a breakpoint in ConnectDocumentToLanguageServerSetupParticipant because I definitely hit breakpoints in this class every time when opening documents in Eclipse.

What's interesting is that this class might not be needed at all any more - the IDocumentSetupParticipant and IDocumentSetupParticipantExtension hooks are part of the file buffer framework which we are using to ensure we register the document with the appropriate LS wrappers and send the LSP didOpen messages.

However, that's all this class does. There are lots of other things that get triggered when an editor is opened, for example the new semantic highlighting, outlining and so on. All of those will immediately call much the same connect code, because of the way LSP4e uses a lazy on-demand approach to make sure LS are active and documents connected before every operation.

As such, it might well be the case that there are always lots of specific operations that will trigger the connection, making this class (which just does the connect and nothing else) completely redundant.

I'll look in more detail into this. It may be that this class is effectively redundant when the user opens a file in an editor but is still needed for cases like the diagnostics, where we are mounting a file buffer and creating a document but not actually opening an editor: it might be that all the other operations that will trigger a connect implicitly only occur if you actually materialise an editor.

Just from observation, it looks like the semantic highlighting and the ConnectDocumentToLanguageServerSetupParticipant are always the first couple of things to fire when a document is mounted (so ConnectDocumentToLanguageServerSetupParticipant is hit early on in the lifecycle, but not definitively the earliest - I've certainly seen the semantic highlighting fire first).

Can anyone comment on the difference between opening a buffer vs actually opening an editor?

mickaelistria commented 1 year ago

This still looks like an issue in Platform. I don't believe Platform restricts having many UIJob, so those should be handled gracefully.

ahmedneilhussain commented 1 year ago

I've submitted a patch that just uses normal CompletableFuture async mechanism to do the connect (which for many LS happens implicitly anyway). Seems to work fine. See #471