8000 Don't leak buffers on moved files by iloveeclipse · Pull Request #1266 · eclipse-lsp4e/lsp4e · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Don't leak buffers on moved files #1266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 27, 2025
Merged

Don't leak buffers on moved files #1266

merged 1 commit into from
May 27, 2025

Conversation

iloveeclipse
Copy link
Contributor

Fixes #1265

@iloveeclipse iloveeclipse mentioned this pull request May 22, 2025
Closed
@iloveeclipse iloveeclipse force-pushed the issue_1265 branch 3 times, most recently from de16a12 to 257a6ca Compare May 22, 2025 14:21
@iloveeclipse
Copy link
Contributor Author

These two tests were failing before, I've adopted expectations, but I honestly don't understand what the tests were checking and why they have expected something to "survive" closed project:

Test Result (2 failures )
org.eclipse.lsp4e.test.WorkspaceFoldersTest.testProjectReopen
org.eclipse.lsp4e.test.WorkspaceFoldersTest.testProjectClose

@iloveeclipse
Copy link
Contributor Author

@sebthom, @mickaelistria : could you please take a look? Note, I have almost zero knowledge in LSP area, so I did the change by "best guessing" how that all is supposed to work.

rubenporras
rubenporras reviewed May 23, 2025
View reviewed changes
8000
@iloveeclipse
Copy link
Contributor Author

Looks like Jenkins had issues?
https://ci.eclipse.org/lsp4e/job/lsp4e-github/job/PR-1266/5/console

[ERROR] Failed to execute goal org.eclipse.tycho:tycho-surefire-plugin:4.0.12:test (default-test) on project org.eclipse.lsp4e.test: An unexpected error occurred while launching the test runtime (process returned error code 143(SIGTERM received?)). The process logfile /home/jenkins/agent/workspace/lsp4e-github_PR-1266/org.eclipse.lsp4e.test/target/work/data/.metadata/.log might contain further details. Command-line used to launch the sub-process was /opt/tools/java/openjdk/jdk-17/latest/bin/java -Dosgi.noShutdown=false -Dosgi.os=linux -Dosgi.ws=gtk -Dosgi.arch=x86_64 -Dfile.encoding=UTF-8 -Xms1g -Xmx1g -Djava.util.logging.config.file=/home/jenkins/agent/workspace/lsp4e-github_PR-1266/org.eclipse.lsp4e.test/src/jul.properties -Dosgi.clean=true -jar /home/jenkins/.m2/repository/p2/osgi/bundle/org.eclipse.equinox.launcher/1.6.1000.v20250227-1734/org.eclipse.equinox.launcher-1.6.1000.v20250227-1734.jar -data /home/jenkins/agent/workspace/lsp4e-github_PR-1266/org.eclipse.lsp4e.test/target/work/data -install /home/jenkins/agent/workspace/lsp4e-github_PR-1266/org.eclipse.lsp4e.test/target/work -configuration /home/jenkins/agent/workspace/lsp4e-github_PR-1266/org.eclipse.lsp4e.test/target/work/configuration -application org.eclipse.tycho.surefire.osgibooter.uitest -testproperties /home/jenkins/agent/workspace/lsp4e-github_PR-1266/org.eclipse.lsp4e.test/target/surefire.properties in working directory /home/jenkins/agent/workspace/lsp4e-github_PR-1266/org.eclipse.lsp4e.test -> [Help 1]

@iloveeclipse
Copy link
Contributor Author

Looks like Jenkins had issues?

I guess it is more than that: https://www.eclipsestatus.io

Core Services, Secondary Services, and 5 other services are down

@sebthom
Copy link
Member
sebthom commented May 23, 2025

The testProjectClose test fails consistently with your patch applied because the WorkspaceFolderEvents are different.

MockLanguageServer.INSTANCE.getWorkspaceService().getWorkspaceFoldersEvents() now contains these events at the end of the test

[DidChangeWorkspaceFoldersParams [
  event = WorkspaceFoldersChangeEvent [
    added = ArrayList (
      WorkspaceFolder [
        uri = "file:///D:/workspaces/junit-workspace/WorkspaceFoldersTest_testProjectClose_1747991065251/"
        name = "WorkspaceFoldersTest_testProjectClose_1747991065251"
      ]
    )
    removed = ArrayList ()
  ]
], DidChangeWorkspaceFoldersParams [
  event = WorkspaceFoldersChangeEvent [
    added = ArrayList ()
    removed = ArrayList ()
  ]
]]

instead of

[DidChangeWorkspaceFoldersParams [
  event = WorkspaceFoldersChangeEvent [
    added = ArrayList (
      WorkspaceFolder [
        uri = "file:///D:/workspaces/junit-workspace/WorkspaceFoldersTest_testProjectClose_1747991209996/"
        name = "WorkspaceFoldersTest_testProjectClose_1747991209996"
      ]
    )
    removed = ArrayList ()
  ]
], DidChangeWorkspaceFoldersParams [
  event = WorkspaceFoldersChangeEvent [
    added = ArrayList ()
    removed = ArrayList (
      WorkspaceFolder [
        uri = "file:///D:/workspaces/junit-workspace/WorkspaceFoldersTest_testProjectClose_1747991209996/"
        name = "WorkspaceFoldersTest_testProjectClose_1747991209996"
      ]
    )
  ]
]]

before the patch.

@iloveeclipse
Copy link
Contributor Author

The testProjectClose test fails consistently with your patch applied because the WorkspaceFolderEvents are different.

Thanks.

I haven't yet tried to understand what this test is doing and why it is failing now, as I've found yet another buffer leak.

This time no file move is needed, it is enough for client to call org.eclipse.lsp4e.LanguageServerWrapper.connect(URI, IDocument) on editor opening and org.eclipse.lsp4e.LanguageServerWrapper.disconnect(URI) on editor close to leak the buffer.

The last patch fixes that too. I have to stop working now, my family needs me. If no one has an idea about how to fix failing test, I plan to continue investigating next week.

@iloveeclipse
Copy link
Contributor Author

Here are my updated tests that show the problem in presence of Copilot plugin and are green with the latest patch here:
TestBuffers_updated.zip

@iloveeclipse
Copy link
Contributor Author

Yet another fix was needed, so updated tests: TestBuffers_updated2.zip

@rubenporras
Copy link
Contributor

Thanks for the PR, it looks sensible. Thanks for working on it. You can just tell me when it is ready to review.

@mickaelistria
Copy link
Contributor

Yes, this PR looks quite good. The failing test needs a fix though.

@iloveeclipse
Copy link
Contributor Author

I'm on it, trying to setup an error free workspace to debug tests...

@iloveeclipse
Copy link
Contributor Author

OK, the org.eclipse.lsp4e.test.WorkspaceFoldersTest.testProjectClose() fails because it expects that LSWrapper still monitors the project life cycle even after disconnecting last listener related to that project. I would assume this is wrong expecation.

Stack at which we disconnect resource listener with the patch here:

	at org.eclipse.core.internal.resources.Workspace.removeResourceChangeListener(Workspace.java:2425)
	at org.eclipse.lsp4e.LanguageServerWrapper.shutdown(LanguageServerWrapper.java:652)
	at org.eclipse.lsp4e.LanguageServerWrapper.stop(LanguageServerWrapper.java:639)
	at org.eclipse.lsp4e.LanguageServerWrapper.disconnect(LanguageServerWrapper.java:806)
	at org.eclipse.lsp4e.LanguageServerWrapper$1.bufferDisposed(LanguageServerWrapper.java:133)
	at org.eclipse.core.internal.filebuffers.TextFileBufferManager$12.run(TextFileBufferManager.java:788)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:47)
	at org.eclipse.core.internal.filebuffers.TextFileBufferManager.fireBufferDisposed(TextFileBufferManager.java:785)
	at org.eclipse.core.internal.filebuffers.TextFileBufferManager.disconnect(TextFileBufferManager.java:207)
	at org.eclipse.lsp4e.LanguageServerWrapper.disconnectTextFileBuffer(LanguageServerWrapper.java:828)
	at org.eclipse.lsp4e.LanguageServerWrapper$1.underlyingFileDeleted(LanguageServerWrapper.java:195)
	at org.eclipse.core.internal.filebuffers.TextFileBufferManager$7.run(TextFileBufferManager.java:723)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:47)
	at org.eclipse.core.internal.filebuffers.TextFileBufferManager.fireUnderlyingFileDeleted(TextFileBufferManager.java:720)
	at org.eclipse.core.internal.filebuffers.ResourceFileBuffer.handleFileDeleted(ResourceFileBuffer.java:429)
	at org.eclipse.core.internal.filebuffers.ResourceFileBuffer$FileSynchronizer$4.execute(ResourceFileBuffer.java:171)
	at org.eclipse.core.internal.filebuffers.ResourceFileBuffer$SafeFileChange.run(ResourceFileBuffer.java:86)
	at org.eclipse.ui.internal.editors.text.UISynchronizationContext.run(UISynchronizationContext.java:35)
	at org.eclipse.core.internal.filebuffers.TextFileBufferManager.execute(TextFileBufferManager.java:593)
	at org.eclipse.core.internal.filebuffers.ResourceFileBuffer$FileSynchronizer.resourceChanged(ResourceFileBuffer.java:184)
	at org.eclipse.core.internal.events.NotificationManager$1.run(NotificationManager.java:321)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:47)
	at org.eclipse.core.internal.events.NotificationManager.notify(NotificationManager.java:311)
	at org.eclipse.core.internal.events.NotificationManager.broadcastChanges(NotificationManager.java:174)
	at org.eclipse.core.internal.resources.Workspace.broadcastPostChange(Workspace.java:465)
	at org.eclipse.core.internal.resources.Workspace.endOperation(Workspace.java:1593)
	at org.eclipse.core.internal.resources.Project.close(Project.java:245)
	at org.eclipse.lsp4e.test.WorkspaceFoldersTest.testProjectClose(WorkspaceFoldersTest.java:108)

@iloveeclipse iloveeclipse marked this pull request as ready for review May 26, 2025 11:27
@iloveeclipse
Copy link
Contributor Author

I would assume this is wrong expecation.

To keep existing behavior I've fixed org.eclipse.lsp4e.LanguageServerWrapper.WorkspaceFolderListener to watch for PRE_CLOSE events on projects. This way the event is sent before we disconnect LanguageServerWrapper.

The org.eclipse.lsp4e.test.WorkspaceFoldersTest.testProjectClose()` test should be green now (locally it is) and with that, please review this PR.

@iloveeclipse
Copy link
Contributor Author

Note: the PR can't be built because of TP issues not caused by this PR:

[ERROR] Failed to resolve target definition file:/home/jenkins/agent/workspace/lsp4e-github_PR-1266/target-platforms/target-platform-latest/target-platform-latest.target: Could not find "com.google.guava/33.4.0.jre" in the repositories of the cur
8000
rent location

@iloveeclipse
Copy link
Contributor Author

Updated test that currently fails with GitHub Copilot 0.6.0.202505160326 (works except two tests still failing also with patched GitHub Copilot 0.6.0.202505230325 and lsp4e with current PR on top):
TestBuffers_updated3.zip.

So far remaining two tests failing seem to be a problem of Github Copilot plugin that doesn't use proper (updated) URI for disconnecting of renamed buffers for non-text editors. One could argue that if client connected document using ABC URI, it can expect to disconnect via same URI even if the underlined file is renamed. In that case one could add a map with old/new URI's. However there is no guarantee how many times a file (folder) would be renamed and so on, so the only reliable way to track that for clients is to listen on document changes.

Anyway, the patch that "workarounds" current Copilot troubles with rename of non-text editors would look like

diff --git a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java
index 86ae833..62dd03a 100644
--- a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java
+++ b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java
@@ -127,2 +127,5 @@ public class LanguageServerWrapper {
 
+	/** key is the new URI, value is the old one */
+	private final Map<URI, URI> movedFiles = new HashMap<>();
+	
 	private final IFileBufferListener fileBufferListener = new LSFileBufferListener();
@@ -178,2 +181,3 @@ public void underlyingFileMoved(IFileBuffer buffer, IPath newPath) {
 			if (!connectedDocuments.containsKey(newUri)) {
+				movedFiles.put(oldUri, newUri);
 				try {
@@ -800,2 +804,11 @@ private boolean supportsWorkspaceFolderCapability() {
 		DocumentContentSynchronizer documentListener = this.connectedDocuments.remove(uri);
+		if(documentListener == null) {
+			URI movedUri = movedFiles.remove(uri);
+			if(movedUri != null) {
+				documentListener = this.connectedDocuments.remove(movedUri);
+				if(documentListener != null) {
+					uri = movedUri;
+				}
+			}
+		}
 		CompletableFuture<@Nullable Void> documentClosedFuture = null;

but as said, it will only work for "simple" use cases with a one-time file rename. So my personal preference is to fix client code here.

@rubenporras
Copy link
Contributor

Note: the PR can't be built because of TP issues not caused by this PR:

[ERROR] Failed to resolve target definition file:/home/jenkins/agent/workspace/lsp4e-github_PR-1266/target-platforms/target-platform-latest/target-platform-latest.target: Could not find "com.google.guava/33.4.0.jre" in the repositories of the current location

I am fixing this in #1276

@rubenporras
Copy link
Contributor
rubenporras 8000 commented May 27, 2025

@iloveeclipse , if you rebase the target should be fine now.

@iloveeclipse
Don't leak buffers on moved or previously connected files
7bd0162 
- adds listener callbacks on buffer changes
- disconnects previously connected buffers from ITextFileBufferManager
on disconnect()
- properly watch on project close events

Fixes eclipse-lsp4e#1265
@mickaelistria mickaelistria merged commit 09b7a2a into eclipse-lsp4e:main May 27, 2025
2 checks passed
@mickaelistria
Copy link
Contributor

Thanks!

@iloveeclipse
Copy link
Contributor Author

Thanks for merge. Are there any nightly build p2 repo one can consume?

@mickaelistria
Copy link
Contributor

Yes, please find the links to snapshots build in https://github.com/eclipse-lsp4e/lsp4e/blob/main/README.md , or you may also use the link to the p2 repository archived directly on Jenkins (we may actually get rid of download.eclipse.org for snapshots one day to make things simpler). Note that when submitting a PR, the p2 repo is also generated on Jenkins and archived as long as the PR is open in order to allow more testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebthom sebthom sebthom left review comments

@rubenporras rubenporras rubenporras left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BufferManager is not disconnect after use
4 participants
@iloveeclipse @sebthom @rubenporras @mickaelistria
0