Skip to content
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.

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

Doesn't revalidate project files after classpath changes if autobuild is off. #3155

Closed
ethan-vanderheijden opened this issue May 9, 2024 · 6 comments · Fixed by #3159
Closed
Assignees
Milestone

Comments

@ethan-vanderheijden
Copy link
Contributor

If you add a project to the workspace that references a new library (e.g. JavaFX), it will first validate your project files without that dependency and then add the dependency to the classpath. If you have autobuild (java.autobuild.enabled) turned on, it will rebuild your project with the new classpath. The result is a flash of errors when the project is first imported:

dependency_error_flash.mp4

However, if you have autobuild turned off, it will never revalidate your project with the new classpath and the errors will remain on screen. Of course, once you start typing, the files are revalidated anyways and the errors disappear.

Maybe the server should revalidate all open project files if autobuild is turned off?

@rgrunber
Copy link
Contributor

rgrunber commented May 9, 2024

I was able to reproduce this once, but I'm not able to do so anymore. Are there a set of consistent steps to do this, or did I just run into some kind of weird race condition. I did notice that the library was accessible and just the diagnostics were stale.

If I turn off the autobuild, open one (invisible) project, add another (invisible project), and quickly open a file that makes use of some referenced library (under lib/ folder) the references seem to be consistently resolved. If the unresolved references flickered I should be able to see them in the client/server communication but they aren't there.

@ethan-vanderheijden
Copy link
Contributor Author

I can reproduce it consistently, but it is a little tricky.

My directory structure looks like:

C:\USERS\ETHAN\DOWNLOADS\WORKSPACE_TEST
│   config.code-workspace
│
├───project_1
│       Main.java
│
└───project_2
        JavaFXTest.java

Main.java is a plain java file with no dependencies and JavaFXTest.java has dependencies already added to java.project.referencedLibraries. config.code-workspace looks like:

{
    "folders": [
        {
            "path": "project_1"
        }
    ]
}

Steps to reproduce:

  1. Start with VScode in a clean state (i.e. no files or folders open).
  2. File > "Open Workspace from File..." and select config.code-workspace. You must open project_1 as a workspace folder. If you open project_1 as a normal project and then click "Add Folder to Workspace...", it will shut down the language server and reboot it with both folders open.
  3. Open Main.java and wait ~15 secs for everything to boot up and settle down
  4. File > "Add Folder to Workspace..." and select project_2
  5. Wait ~10 secs for the workspace/didChangeWorkspaceFolders to fire. Do not open JavaFXTest.java yet.
  6. Open JavaFXTest.java and the diagnostics should go red. Do not click inside the editor at this time otherwise the Java extension will send a java/validateDocument notification.

Here's the LSP logs starting from step 5. I left out all the codeAction, executeCommand, etc.

Sending notification 'workspace/didChangeWorkspaceFolders'.

Received notification 'language/status'.
    "message": "Updating workspace folders: Adding 1 folder(s), removing 0 folders."

Received notification '$/progress'.
    "message": "Updating workspace folders"

Received notification '$/progress'.
    "message": "Register Watchers"

Sending notification 'textDocument/didOpen'.
    "uri": "file:///c%3A/Users/Ethan/Downloads/workspace_test/project_2/JavaFXTest.java"

Sending notification 'java/validateDocument'.
    "uri": "file:///c%3A/Users/Ethan/Downloads/workspace_test/project_2/JavaFXTest.java"

Received notification '$/progress'.
    "message": "Validate documents"

--- If autobuild is on ---
Received notification '$/progress'.
    "message": "Building"

Received notification '$/progress'.
    "message": "Validate documents"
--------------------------

Received notification '$/progress'.
    "message": "Publish Diagnostics"

Received notification 'textDocument/publishDiagnostics'.
    "uri": "file:///C:/Users/Ethan/Downloads/workspace_test/project_2/JavaFXTest.java",
    "diagnostics": Lots and lots and lots of errors

Received notification '$/progress'.
    "message": "Update classpath Job"

Received notification 'language/eventNotification'.
    "eventType": 100

--- If autobuild is on ---
Received notification '$/progress'.
    "message": "Building"

Received notification '$/progress'.
    "message": "Validate documents"

Received notification '$/progress'.
    "message": "Publish Diagnostics"

Received notification 'textDocument/publishDiagnostics'.
    "uri": "file:///C:/Users/Ethan/Downloads/workspace_test/project_2/JavaFXTest.java",
    "diagnostics": No errors
--------------------------

@rgrunber
Copy link
Contributor

rgrunber commented May 17, 2024

I'm definitely able to reproduce so thank-you for the detailed example. At first I thought maybe java.edit.validateAllOpenBuffersOnChanges set to true would fix this but it doesn't! See #2587 & https://github.com/redhat-developer/vscode-java/pull/3053/files for how/why we're careful about refreshing diagnostics.

I think the issue has more to do with the fact that the autobuild (being disabled) is respected by the code paths from didChangeWorkspaceFolders . Specifically, is it possible to have the re-validation be added somewhere at

@Override
public IStatus runInWorkspace(IProgressMonitor monitor) {
IStatus status = Status.OK_STATUS;
SubMonitor subMonitor = SubMonitor.convert(monitor, addedRootPaths.size() + removedRootPaths.size());
try {
long start = System.currentTimeMillis();
IProject[] projects = getWorkspaceRoot().getProjects();
for (IProject project : projects) {
if (ResourceUtils.isContainedIn(project.getLocation(), removedRootPaths)) {
try {
project.delete(false, true, subMonitor.split(1));
} catch (CoreException e) {
JavaLanguageServerPlugin.logException("Problems removing '" + project.getName() + "' from workspace.", e);
}
}
}
importProjects(addedRootPaths, subMonitor.split(addedRootPaths.size()));
registerWatchers(true);
long elapsed = System.currentTimeMillis() - start;
JavaLanguageServerPlugin.logInfo("Updated workspace folders in " + elapsed + " ms: Added " + addedRootPaths.size() + " folder(s), removed" + removedRootPaths.size() + " folders.");
JavaLanguageServerPlugin.logInfo(getWorkspaceInfo());
? This is triggered specifically when workspace folders are added/removed.

CC'ing @testforstephen for awareness.

@ethan-vanderheijden
Copy link
Contributor Author

See #2587 & https://github.com/redhat-developer/vscode-java/pull/3053/files for how/why we're careful about refreshing diagnostics.

Ahh, so that's why vscode sends "java/validateDocument" when the editor receives focus! Very helpful, thank you.

I guess this begs the question of whether we want to revalidate open files when the classpath changes. Since the classpath rarely changes, it wouldn't have too much performance cost in the grand scheme, and when it does change, diagnostics across many files are likely to be stale. Then again, these stale diagnostics aren't a severe problem because focusing those files will refresh them.

I think the issue has more to do with the fact that the autobuild (being disabled) is respected by the code paths from didChangeWorkspaceFolders . Specifically, is it possible to have the re-validation be added somewhere at

This might change your mind: you can reproduce this bug by modifying pom.xml to change classpath. Here are the steps to reproduce (this example is derived from the test case in org.eclipse.jdt.ls.tests/projects/maven/salut):

directory structure:

salut
│   pom.xml
│   
└───src
    └───main
        └───java
            └───java
                    Foo.java

pom.xml:

<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
	xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
	<modelVersion>4.0.0</modelVersion>
	<groupId>foo.bar</groupId>
	<artifactId>salut</artifactId>
	<version>0.0.1-SNAPSHOT</version>
	<build>
		<plugins>
			<plugin>
				<artifactId>maven-compiler-plugin</artifactId>
				<version>3.5.1</version>
				<configuration>
					<source>1.7</source>
					<target>1.7</target>
				</configuration>
			</plugin>
		</plugins>
	</build>
	<dependencies>
		<dependency>
			<groupId>org.apache.commons</groupId>
			<artifactId>commons-lang3</artifactId>
			<version>3.5</version>
		</dependency>
	</dependencies>
</project>

Foo.java:

package java;

import org.apache.commons.lang3.StringUtils;

/**
 * This is foo
 */
public class Foo {

	public static void main(String[] args) {
		System.out.print( StringUtils.capitalize("Hello world! from "+Foo.class));
	}

	public void linkedFromFoo2() {

	}
}

Steps:

  1. Open up Foo.java and pom.xml in side-by-side editors and wait for the language server to boot up + settle down
  2. delete the commons-lang3 dependency from pom.xml. Click yes when vscode prompts "do you want to synchronize the Java classpath/configuration?"
  3. No errors appear in Foo.java because diagnostics are stale. Once you give focus to the file, the errors appear.
  4. Re-add commons-lang3 and see stale diagnostics again

@ethan-vanderheijden
Copy link
Contributor Author

If it helps:

When autobuilding is enabled and the classpath changes, the WorkspaceDiagnosticsHandler gets a resource change event for all files inside the project. These IResourceDeltas have marker deltas, so the WorkspaceDiagnosticsHandler revalidates the file here:

When autobuilding is disabled, the WorkspaceDiagnosticsHandler still gets a resource change event for each file inside the project, but these IResourceDelta have no marker deltas so the files are never revalidated.

It looks like the Eclipse Core library is responsible for rebuilding the project when the classpath changes. Rebuilding the project updates the diagnostic markers, which WorkspaceDiagnosticsHandler happens to see and send to the client.

@rgrunber
Copy link
Contributor

This might change your mind: you can reproduce this bug by modifying pom.xml to change classpath. Here are the steps to reproduce (this example is derived from the test case in org.eclipse.jdt.ls.tests/projects/maven/salut):

Yeah that's convincing. My first reaction was to avoid validating on classpath change when the issue seems to be only about project import/folder addition. However, if the issue exists on a classpath change as well, then it makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants