Repository navigation
fix: remove index of deleted project - #3842
rubenporras wants to merge 1 commit into
Conversation
rubenporras
changed the title
| * | ||
| * @since 2.45 | ||
| */ | ||
| public void removeProjectIndex(String projectName) { |
There was a problem hiding this comment.
Since this method is called on a remaining project and removes another project's container from its resource set, removeProjectIndex reads as if it removed this project's own index. Could we name it removeProjectFromIndex(String) instead?
/**
* Removes the index of the given project from the resource set of this project.
*
* <p>This is a no-op if this project has not been built yet. It is expected to be
* called on the workspace write thread (see WorkspaceManager.refreshWorkspaceConfig).
*
* @since 2.45
*/
public void removeProjectFromIndex(String projectName) {The call site in WorkspaceManager.refreshWorkspaceConfig would of course need to be updated too. Documenting the no-op and the threading expectation would also help subclasses that override this to clean up their own additional resource sets.
There was a problem hiding this comment.
I have been thinking about it. What do you think about naming it notifyProjectRemoved? Then we would remove the paragraph "This is a no-op if this project has not been built yet...".
There was a problem hiding this comment.
notifyProjectRemoved works for me — it reads well at the call site (remainingProject.notifyProjectRemoved(deletedProject)) and fits the existing aboutToRemoveFromWorkspace notification style. I'd keep the note that it must be called on the workspace write thread; a shortened no-op note ("no-op if this project has no resource set yet") is fine to keep or to drop, no strong preference.
| languageServer.didChangeWorkspaceFolders(didChangeWorkspaceFoldersParams); | ||
| }); | ||
|
|
||
| Assert.assertNull(getIndex("root1").getContainer("root2")); |
There was a problem hiding this comment.
The test verifies the internal data structure, but not the user-visible consequence from #3841: after root2 is removed, root1 should no longer see the descriptions/types of the removed project. Could we assert that as well?
For example, keep the URI returned by writeFile(rootFolder2, ...) and add:
Assert.assertNull(getIndex("root1").getResourceDescription(URI.createURI(twoUri)));Alternatively, trigger a rebuild of root1 and assert that Bar in one.testlang is now reported as unresolved (i.e. diagnostics change from none to one).
That way the regression test guards the behaviour rather than only the container map, and it would also catch a future change that reintroduces the removed descriptions through a different path.
There was a problem hiding this comment.
I will look into it tomorrow
| ChunkedResourceDescriptions resDescs = ChunkedResourceDescriptions.findInEmfObject(resourceSet); | ||
| for (Map.Entry<String, ResourceDescriptionsData> entry : indexProvider.get().entrySet()) { | ||
| Map<String, ResourceDescriptionsData> index = indexProvider.get(); | ||
| resDescs.retainContainers(index.keySet()); |
There was a problem hiding this comment.
@rubenporras quick question about the second commit: in the default WorkspaceManager setup, indexProvider is () -> fullIndex, and the only place that removes an entry from fullIndex is refreshWorkspaceConfig, which this PR already handles by calling removeProjectFromIndex on the remaining projects. Containers only ever enter a project's ChunkedResourceDescriptions via createNewResourceSet/createFreshResourceSet from that same map. So with the first commit in place, is there a scenario where retainContainers here can still find a stale container?
I'm asking because this changes the contract of this protected method for subclasses that intentionally add extra containers to a reused resource set without registering them in indexProvider — those containers would now be dropped on the next build.
If there is a concrete scenario (e.g. a subclass passing a different index provider, or someone mutating the index directly), could you describe it and add a test that actually exercises the reconciliation: drop a container from the provider, rebuild, assert it is gone? If there isn't one, I'd suggest dropping this commit and keeping only the eager cleanup — that already restores the invariant and keeps the fix minimal. If you prefer to keep it as a safety net, a note on createFreshResourceSet that the chunked index is reconciled with indexProvider on every build would make the behavior explicit for subclass authors.
Also, the commit message currently reads as if this commit fixes the reported leak, but the eager removal is what fixes #3841 for the default setup — it would be worth rewording so the two commits don't look like duplicates.
There was a problem hiding this comment.
The second commit was only a "safety" net. I actually do not have a concrete scenario in mind and I am happy of dropping it from this PR. I will do this tomorrow.
67a8a74 to
cdadbb8
Compare
ProjectManager.createFreshResourceSet reuses the resource set of a project and only ever sets containers of its ChunkedResourceDescriptions. When a workspace folder is removed, refreshWorkspaceConfig removes the project from fullIndex, but the resource sets of the remaining projects still hold its index. It stays in memory for as long as any other project exists, and the remaining projects can still see it. Remove the container of a deleted project from the resource sets of the remaining projects through the new ProjectManager.notifyProjectRemoved, which subclasses that keep further resource sets can override.
cdadbb8 to
31f8ac4
Compare
for #3841