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

Use linked resource instead of filesystem #2658

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mickaelistria
Copy link
Contributor

For all metadata files, except the .project (which may be harder to move).
This also copies some logic from the filesystem to ProjectsManager. We cannot have the filesystem bundle requiring and referencing the main jdt.ls bundle as this would cause classloading error.

@rgrunber
Copy link
Contributor

A while ago we had a discussion where you mentioned that .classpath, .settings files can be set using linked resources, so doing this is likely the right approach. We should use that filesystem customization as little as possible. The tests likely need some investigation.

@mickaelistria
Copy link
Contributor Author

I'm looking at the tests; however, it looks like it's not allowed to put linked resources under <workspace>/.metadata. Is it OK if we move those to <workspace>/.projects-metadata ?

@fbricon
Copy link
Contributor

fbricon commented May 16, 2023

your code only handles the case of projects that are automatically created by jdt.ls (default and invisible). Buildship or m2e would still create the eclipse config files in the source folder, on import, without the FS trick. Unless there's a simple way to workaround that problem, I don't see linked resources as a viable solution.

@mickaelistria
Copy link
Contributor Author

Buildship or m2e would still create the eclipse config files in the source folder, on import, without the FS trick.

OK, I see. I guess the importer can be reworked to first create the Java project, using ProjectsManager.createJavaProject() and then configuring it instead of making them fully create the project.

@mickaelistria mickaelistria force-pushed the linked-resources branch 3 times, most recently from 98a4eb6 to 02b875a Compare June 9, 2023 20:59
@mickaelistria mickaelistria force-pushed the linked-resources branch 3 times, most recently from cb10a2b to f89ed19 Compare June 12, 2023 14:04
@mickaelistria mickaelistria force-pushed the linked-resources branch 2 times, most recently from 8b5604d to 0dd82bd Compare June 20, 2023 14:56
@mickaelistria mickaelistria marked this pull request as draft June 20, 2023 20:00
@mickaelistria mickaelistria force-pushed the linked-resources branch 4 times, most recently from 80e718d to 711168f Compare June 22, 2023 16:49
@mickaelistria
Copy link
Contributor Author

Related: eclipse-platform/eclipse.platform#517

@mickaelistria mickaelistria force-pushed the linked-resources branch 3 times, most recently from 480faf8 to fae8c0e Compare July 2, 2023 17:10
@mickaelistria mickaelistria marked this pull request as ready for review July 2, 2023 20:00
@mickaelistria
Copy link
Contributor Author

I will try to investigate how this works or not with Bazel support ( redhat-developer/vscode-java#909 (comment) )

@mickaelistria
Copy link
Contributor Author

It looks like vscode-java-bazel already puts eclipse metadata into a .eclipse folder inside project at import; so this patch doesn't seem to change that.

@mickaelistria
Copy link
Contributor Author

It would be nice if this could be revisited sometimes. Working without the URI hacks would close the doors to many potential bugs.

@rgrunber
Copy link
Contributor

@jdneo do you think you might have time to look here. I see this is completely removing the jdt.ls.filesystem bundle so definitely would be nice to thoroughly test this.

@jdneo
Copy link
Contributor

jdneo commented Oct 31, 2023

So if it's Windows-specific issue, CI wouldn't see it?

I guess it's a Windows issue. I tried using Files.move(metadataFile.getLocation().toFile().toPath(), targetFile.toPath(), StandardCopyOption.REPLACE_EXISTING); to replace .renameTo and the metadata files won't exist on project root then.

We may also want to track if it happens that the linked resources are failed to be established. Will all thrown exception be sent as telemetry? This is a question to @rgrunber and @testforstephen

@mickaelistria
Copy link
Contributor Author

I tried using Files.move(metadataFile.getLocation().toFile().toPath(), targetFile.toPath(), StandardCopyOption.REPLACE_EXISTING); to replace .renameTo and the metadata files won't exist on project root then.

Thanks for the tip, I've changed the code to use it.

@jdneo
Copy link
Contributor

jdneo commented Nov 2, 2023

There is an error in the log when import a workspace for the first time in the debug mode:

!ENTRY org.eclipse.jdt.core 4 4 2023-11-02 11:11:09.411
!MESSAGE Exception while reading /jdt.ls-java-project/.classpath
!STACK 0
java.io.IOException: Bad format
	at org.eclipse.jdt.internal.core.JavaProject.decodeClasspath(JavaProject.java:1323)
	at org.eclipse.jdt.internal.core.JavaProject.readFileEntriesWithException(JavaProject.java:3081)
	at org.eclipse.jdt.internal.core.JavaProject.readFileEntries(JavaProject.java:3091)
	at org.eclipse.jdt.internal.core.JavaProject.writeFileEntries(JavaProject.java:3475)
	at org.eclipse.jdt.internal.core.JavaModelManager$PerProjectInfo.writeAndCacheClasspath(JavaModelManager.java:1570)
	at org.eclipse.jdt.internal.core.JavaModelManager$PerProjectInfo.writeAndCacheClasspath(JavaModelManager.java:1582)
	at org.eclipse.jdt.internal.core.SetClasspathOperation.executeOperation(SetClasspathOperation.java:81)
	at org.eclipse.jdt.internal.core.JavaModelOperation.run(JavaModelOperation.java:740)
	at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2453)
	at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2478)
	at org.eclipse.jdt.internal.core.JavaModelOperation.runOperation(JavaModelOperation.java:811)
	at org.eclipse.jdt.internal.core.JavaProject.setRawClasspath(JavaProject.java:3685)
	at org.eclipse.jdt.internal.core.JavaProject.setRawClasspath(JavaProject.java:3645)
	at org.eclipse.jdt.internal.core.JavaProject.setRawClasspath(JavaProject.java:3701)
	at org.eclipse.jdt.ls.core.internal.managers.ProjectsManager.createJavaProject(ProjectsManager.java:439)
	at org.eclipse.jdt.ls.core.internal.managers.ProjectsManager.createJavaProject(ProjectsManager.java:366)
	at org.eclipse.jdt.ls.core.internal.managers.ProjectsManager.initializeProjects(ProjectsManager.java:114)
	at org.eclipse.jdt.ls.core.internal.handlers.InitHandler$1.runInWorkspace(InitHandler.java:260)
	at org.eclipse.core.internal.resources.InternalWorkspaceJob.run(InternalWorkspaceJob.java:43)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)
Caused by: org.xml.sax.SAXParseException; lineNumber: 1; columnNumber: 1; Premature end of file.
	at java.xml/com.sun.org.apache.xerces.internal.parsers.DOMParser.parse(DOMParser.java:262)
	at java.xml/com.sun.org.apache.xerces.internal.jaxp.DocumentBuilderImpl.parse(DocumentBuilderImpl.java:342)
	at org.eclipse.jdt.internal.core.JavaProject.decodeClasspath(JavaProject.java:1321)
	... 19 more

Not sure if this is related with my dev environment or it is an issue.

@mickaelistria
Copy link
Contributor Author

Can you please describe the exact steps to reproduce this issue? Do you think it can be captured in a unit test?

@jdneo
Copy link
Contributor

jdneo commented Nov 7, 2023

Sure.

  1. I'm using VS Code + PDE to launch this PR.
  2. Remove the workspace folder runtime-JDT-LS that is used at debug runtime entirely. To find that folder, you can
    1. Trigger the command Open Java Language server log file
    2. Reveal the opened log file in system file explorer
    3. you can find it going up: ../../runtime-JDT-LS
  3. Use jdt.ls.socket-stream.launch to launch the JDT.LS
  4. Use the vscode launch config Launch Extension - JDTLS Client to launch the client
  5. After import finished, open the server log: runtime-JDT-LS/.metadata/.log

@mickaelistria
Copy link
Contributor Author

Good news: I can reproduce it with a test, I'll just add a log listener to capture the issue so it may fail the test.

@mickaelistria
Copy link
Contributor Author

@jdneo I have added a check for this issue and fixed it.

@jdneo
Copy link
Contributor

jdneo commented Nov 8, 2023

@mickaelistria Found a new issue on a MacOS machine today when I tried this PR. I think the root cause is that the accuracy of attributes.creationTime().toInstant() and Instant.now() are different, which makes some of the metadata file fail to be linked.

Below are what I observed.

  1. Track the start time before calling Instant creationRequestInstant = Instant.now();
Screenshot 2023-11-08 at 15 56 38
  1. Track the end time and the duration after a project is created. From the screenshot, you can see that the duration is 68787125 nano seconds, which is ~ 68ms.
Screenshot 2023-11-08 at 15 56 21
  1. Because the duration is so short and the different accuracy when comparing two time instant, it fails to return the expected result:
Screenshot 2023-11-08 at 15 57 40

@mickaelistria
Copy link
Contributor Author

Thanks for the test and report analysis @jdneo !
I've update the patch so that isNewer skips sub-second accuracy. Tests seem good so far; can you please check on macOS?

@jdneo
Copy link
Contributor

jdneo commented Nov 9, 2023

It's working on my Mac now.

Though, I'm not sure only comparing on second is safe enough. Because 1 second can really do so many things for a computer. Not to mention that the hardware will be more powerful as the time goes by.

@mickaelistria
Copy link
Contributor Author

I don't see a way to go reliably under the precision of creationTime (which is good on Linux and Windows apparently). Do you know a way to get more precise creationTime for a file on Mac?

@mickaelistria
Copy link
Contributor Author

I tried something else; dynamically looking at accuracy. Can you please check whether it works on macOS? If it does, then I think it's a good enough fix; that will scale when macOS filesystem gets better precision.

@mickaelistria mickaelistria force-pushed the linked-resources branch 2 times, most recently from 8a3d0e7 to e608650 Compare November 9, 2023 10:52
ChronoUnit smallestSupportedUnit = Stream.of(ChronoUnit.NANOS, ChronoUnit.MILLIS, ChronoUnit.SECONDS) // IMPORTANT: keeps units in Stream from finer to coarser
.filter(creationInstant::isSupported).filter(instant::isSupported) //
.findFirst().orElse(null);
if (smallestSupportedUnit != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check on MacOS today, looks like we still have the problem here.

smallestSupportedUnit is assigned to ChronoUnit.NANOS, though the accuracy of creationInstant is second, makes this method returns false finally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thanks, so should we just hardcode the rule for macOS to trim to the second?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess so. I couldn't think of any better solution for this case.

(I was thinking about find out all the already existing metadata files before import started, but that may have too much perf penalty and may be not worth it.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about find out all the already existing metadata files before import started, but that may have too much perf penalty and may be not worth it.

The difficulty here is that we don't know at that stage what folders the importers are going to consider as projects, so we're not sure which files matter or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you can just those files start from rootPaths, because all the metadata files that exist before import happens will not be linked, so we don't need to care about what folders will be imported by which importers.

I'm not optimistic about the performance though...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean recursively crawling the whole subtrees under rootPAths for pre-existing .classpath, .project and so on?
Indeed, it could impact heavily performance for users using the wrong root (eg their home or filesystem root), but for a regular development project, it should be fast enough (I don't expect it to take more than 1 seconds for the biggest projects). But if we're ready to loose 1 second, we can just put a 1 second wait directly after Instant.now , and the check will then be reliable on all OSs ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the new patch, I've hardcoded that Instant should be truncated and compared by the second on macOS.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if we're ready to loose 1 second, we can just put a 1 second wait directly after Instant.now , and the check will then be reliable on all OSs

That's a good insight! I think there are some key metrics we need to track when roll out this change. Then we can make the decision based on the data, like:

  • how many users happen to meet the two equally instants on second during import.
  • how many users fail to link the files.

For example, if the data shows that very few users have two equally instant on second, then we can just simply trim to the second.

Use linked resources instead of filesystem hack as this one can return
invalid locations through standard API.

This removes the filesystem bundle, the logic is now moved in
ProjectManagers
and a workspace listener takes care of using linked resources for
metadata.

This also remove the
InvisibleProjectMetadataTest.testMetadataFileLocation() test because
creating a new project in Eclipse workspace now enforces creation of a
.settings/org.eclipse.core.resources.prefs file. Where the metadata for
the invisible project is stored doesn't matter as the internal project
is internal and not visible to user anyway.
@jdneo
Copy link
Contributor

jdneo commented Nov 15, 2023

I'm thinking about this case that this solution might not be able to handle:

  • User opens a large-scale project, the import takes a long time.
  • Before the import finishes, user closes VS Code.
  • Now, part of the modules are imported with metadata files generated at project root, but not enough time to link them. And they will be persisted at the project root since then.

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

Successfully merging this pull request may close these issues.

4 participants