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

Don't evict the full cache of local repo when m2 repository changes. #494

Closed
wants to merge 2 commits into from

Conversation

angelozerr
Copy link
Contributor

Don't evict the full cache of local repo when m2 repository changes.

@angelozerr angelozerr mentioned this pull request Aug 6, 2023
@vrubezhny
Copy link
Contributor

vrubezhny commented Aug 6, 2023

The watcher has the only local repo directory registered, so it may control only the direct children of this directory, not the deeper levels, so it will not be notified if a new version of an existing artifact arrives, or a new group is created inside an already existing one.

Currently, after any change in local repo directory (addind/removing a file or directory) Watcher stops working and exits, probably due to an exception happening like the following one:

Message: Local repo thread watcher event processing error
java.lang.IllegalArgumentException: 'other' is different type of Path
	at java.base/sun.nio.fs.UnixPath.relativize(UnixPath.java:398)
	at java.base/sun.nio.fs.UnixPath.relativize(UnixPath.java:43)
	at org.eclipse.lemminx.extensions.maven.searcher.LocalRepositorySearcher.createArtifact(LocalRepositorySearcher.java:251)
	at org.eclipse.lemminx.extensions.maven.searcher.LocalRepositorySearcher.lambda$getOrCreateLocalArtifactsLastVersion$5(LocalRepositorySearcher.java:143)
	at java.base/java.util.concurrent.CompletableFuture$UniAccept.tryFire(CompletableFuture.java:718)
	at java.base/java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:483)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:373)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1182)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1655)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1622)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:165)

This exception is quite easy to fix, however to make Watcher to react to an artifact changes (not to the only creation of a new/ modification/removal of an existing group) its needed to register Watcher for ALL the child directories of the local repo directory recursively and watching for all the watch keys created (resetting/cancelling etc)

@angelozerr
Copy link
Contributor Author

@vrubezhny I improved the watcher

Copy link
Contributor

@vrubezhny vrubezhny left a comment

Choose a reason for hiding this comment

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

Update the comments please

@vrubezhny
Copy link
Contributor

I have asked a question on using this Oracle license: https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/9848
I believe we are allowed to incorporate such a class, but if I'n wrong we'll be forced to roll the commit back and replace it with some other solution.

@vrubezhny
Copy link
Contributor

When looking for a latest artifact versions a "release" version is to be treated as more recent version comparing to an according "snapshot" or a "qualifier" version, e.g:: 0.9.1 > 0.9.1-SNAPSHOT > 0.9.0

@vrubezhny
Copy link
Contributor

@angelozerr I've made it to correctly react on different kinds of artifacts deletions (permanent & trash) and additions (copy to & building with maven).
Could you, please, give it a try when you have time?
My changes are added to your branch as a fixup.

@vrubezhny
Copy link
Contributor

The PR cannot be applied as it generates errors like:

Aug 06, 2023 4:50:52 PM org.eclipse.lemminx.extensions.maven.searcher.LocalRepositorySearcher lambda$getOrCreateLocalArtifactsLastVersion$3
SEVERE: Cannot create local repo thread watcher error
java.io.IOException: User limit of inotify watches reached
	at java.base/sun.nio.fs.LinuxWatchService$Poller.implRegister(LinuxWatchService.java:264)
	at java.base/sun.nio.fs.AbstractPoller.processRequests(AbstractPoller.java:266)
	at java.base/sun.nio.fs.LinuxWatchService$Poller.run(LinuxWatchService.java:364)
	at java.base/java.lang.Thread.run(Thread.java:833)

due to exceeding the fs.inotify.max_user_watches quota.
It's not good to force users to modify system parameters, so other solution is to be applied to update the Local Repo Searcher's cache.

@vrubezhny vrubezhny closed this Aug 7, 2023
@mickaelistria
Copy link
Contributor

Isn't the issue caused by the watchService not properly disconnecting the watch(key) before plugging a new one? I think this PR has merits and it could be worth keeping it open and trying to make it work.

@mickaelistria mickaelistria reopened this Aug 17, 2023
@vrubezhny
Copy link
Contributor

vrubezhny commented Aug 17, 2023

@mickaelistria Why is this PR restored? It's outdated and doesn't add any value now.

Isn't the issue caused by the watchService not properly disconnecting the watch(key) before plugging a new one? I think this PR has merits and it could be worth keeping it open and trying to make it work.

We don't use WatchService anymore for two reasons:

  1. To properly update Local Repository Caches it's needed to add a watch key for every directory in the repository (recursively) - it's impossible to do so without modifying the user environment confoguration
  2. The old implementation was just dropping the full cache on those changes it had been able to detect (a far away from all the possible changes) - now we're not dropping the cache, but updating the only changed artifacts in case they're just built and cched with lemminx-maven project cache builder and if the local repository is changed from outside (every 30 minutes)

@mickaelistria
Copy link
Contributor

I didn't realize that the WatchService was not looking at folder recursively. In another discussion, @angelozerr made me realize that, and I agree with you both that this PR can be closed.
However, some polling can still be useful to update the repo more regularly on changes. I would recommend re-implementing the initially intended, but never working, behavior, using Apache FileAlterationMonitor, as described in https://commons.apache.org/proper/commons-io/apidocs/org/apache/commons/io/monitor/FileAlterationObserver.html

@vrubezhny
Copy link
Contributor

I would recommend re-implementing the initially intended, but never working, behavior, using Apache FileAlterationMonitor, as described in https://commons.apache.org/proper/commons-io/apidocs/org/apache/commons/io/monitor/FileAlterationObserver.html

It probably will not be useful for updating after a workspace maven project is built, but if it doesn't rely internally on Watch Service API then it worth to use it instead of the current updater thread (that currently monitors the changes "from outside" on Local repository directories for every 30 minutes).

I would creating a new issue to track this.

@vrubezhny
Copy link
Contributor

Created #507 to consider using Apache FileAlterationMonitor

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

Successfully merging this pull request may close these issues.

None yet

3 participants