Bug 558035 - DynamicSourcesWorkingSetUpdater job can access deleted projects
Summary: DynamicSourcesWorkingSetUpdater job can access deleted projects
Status: NEW
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.12   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: JDT-UI-Inbox CLA
QA Contact:
URL:
Whiteboard: stalebug
Keywords:
Depends on:
Blocks:
 
Reported: 2019-12-09 10:59 EST by Simeon Andreev CLA
Modified: 2022-05-28 00:41 EDT (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Simeon Andreev CLA 2019-12-09 10:59:31 EST
DynamicSourcesWorkingSetUpdater.updateElements(IWorkingSet[], IProgressMonitor) has the following code:

if (project.getProject().isOpen()) {
	for (IPackageFragmentRoot iPackageFragmentRoot : project.getPackageFragmentRoots()) {
		IClasspathEntry classpathEntry= iPackageFragmentRoot.getRawClasspathEntry();
		if (classpathEntry.getEntryKind() == IClasspathEntry.CPE_SOURCE) {
			if (classpathEntry.isTest()) {
				testResult.add(iPackageFragmentRoot);
			} else {
				mainResult.add(iPackageFragmentRoot);
			}
		}
	}
}

Its possible that the project in question is deleted *after* the if block is entered, resulting in an exception from this code such as:

!ENTRY org.eclipse.jdt.core 4 0 2019-12-06 20:36:20.553
!MESSAGE Unexpected internal error
!STACK 1
Java Model Exception: Java Model Status [... does not exist]
	at org.eclipse.jdt.internal.core.JavaElement.newJavaModelException(JavaElement.java:583)
	at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:256)
	at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:596)
	at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:326)
	at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:312)
	at org.eclipse.jdt.internal.core.JavaElement.getChildren(JavaElement.java:267)
	at org.eclipse.jdt.internal.core.PackageFragmentRoot.getSourceModuleDescription(PackageFragmentRoot.java:897)
	at org.eclipse.jdt.internal.core.PackageFragmentRoot.getModuleDescription(PackageFragmentRoot.java:890)
	at org.eclipse.jdt.internal.core.JarPackageFragmentRoot.getModuleDescription(JarPackageFragmentRoot.java:321)
	at org.eclipse.jdt.internal.core.JrtPackageFragmentRoot.getModule(JrtPackageFragmentRoot.java:144)
	at org.eclipse.jdt.internal.core.JavaProject.lambda$1(JavaProject.java:800)
	at org.eclipse.jdt.internal.core.JavaProject.internalDefaultRootModules(JavaProject.java:822)
	at org.eclipse.jdt.internal.core.JavaProject.defaultRootModules(JavaProject.java:798)
	at org.eclipse.jdt.internal.core.JavaProject.computePackageFragmentRoots(JavaProject.java:745)
	at org.eclipse.jdt.internal.core.JavaProject.computePackageFragmentRoots(JavaProject.java:1053)
	at org.eclipse.jdt.internal.core.JavaProject.computePackageFragmentRoots(JavaProject.java:998)
	at org.eclipse.jdt.internal.core.JavaProject.computePackageFragmentRoots(JavaProject.java:975)
	at org.eclipse.jdt.internal.core.JavaProject.buildStructure(JavaProject.java:489)
	at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:268)
	at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:596)
	at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:326)
	at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:312)
	at org.eclipse.jdt.internal.core.JavaElement.getChildren(JavaElement.java:267)
	at org.eclipse.jdt.internal.core.JavaProject.getPackageFragmentRoots(JavaProject.java:2281)
	at org.eclipse.jdt.internal.ui.workingsets.DynamicSourcesWorkingSetUpdater.updateElements(DynamicSourcesWorkingSetUpdater.java:211)
	at org.eclipse.jdt.internal.ui.workingsets.DynamicSourcesWorkingSetUpdater.updateElements(DynamicSourcesWorkingSetUpdater.java:192)
	at org.eclipse.jdt.internal.ui.workingsets.DynamicSourcesWorkingSetUpdater$1.run(DynamicSourcesWorkingSetUpdater.java:160)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)
!SUBENTRY 1 org.eclipse.jdt.core 4 969 2019-12-06 20:36:20.555
!MESSAGE ... does not exist

This job should have at least some rule, probably the workspace one.
Comment 1 Eclipse Genie CLA 2019-12-09 11:10:10 EST
New Gerrit change created: https://git.eclipse.org/r/154125
Comment 2 Till Brychcy CLA 2019-12-13 15:26:45 EST
I think we can avoid any locks or expensive computation in the UI Thread and just suppress the error message - if a project (or src folder) disappears before the job is executed in the ui thread, another update of the working sets will be triggered anyway.
Comment 3 Simeon Andreev CLA 2019-12-16 03:53:01 EST
(In reply to Till Brychcy from comment #2)
> I think we can avoid any locks or expensive computation in the UI Thread and
> just suppress the error message - if a project (or src folder) disappears
> before the job is executed in the ui thread, another update of the working
> sets will be triggered anyway.

Unfortunately there is "org.eclipse.jdt.internal.core.PackageFragmentRoot.getSourceModuleDescription()":

private IModuleDescription getSourceModuleDescription() {
	try {
		IJavaElement[] pkgs = getChildren();
		for (int j = 0, length = pkgs.length; j < length; j++) {
			// only look in the default package
			if (pkgs[j].getElementName().length() == 0) {
				OpenableElementInfo info = null;
				if (getKind() == IPackageFragmentRoot.K_SOURCE) {
					ICompilationUnit unit = ((PackageFragment) pkgs[j])
							.getCompilationUnit(TypeConstants.MODULE_INFO_FILE_NAME_STRING);
					if (unit instanceof CompilationUnit && unit.exists()) {
						info = (CompilationUnitElementInfo) ((CompilationUnit) unit)
								.getElementInfo();
						if (info != null)
							return info.getModule();
					}
				} else {
					IModularClassFile classFile = ((IPackageFragment)pkgs[j]).getModularClassFile();
					if (classFile.exists()) {
						return classFile.getModule();
					}
				}
				break;
			}
		}
	} catch (JavaModelException e) {
		Util.log(e);
	}
	return null;
}

The exception is logged from that code. "org.eclipse.jdt.internal.ui.workingsets.DynamicSourcesWorkingSetUpdater.updateElements(IWorkingSet[], IProgressMonitor)" catches the exception and does nothing with it (returns a cancel status). But its "too late".
Comment 4 Till Brychcy CLA 2019-12-16 04:20:58 EST
I guess the logging there shouldn't be done (or maybe only if some debug-setting is enabled.)

Also, it looks like it is not a project that is missing, JrtPackageFragmentRoot means that the JDK is being looked at.

Have you edited the stack trace, or is there really a "..." in:

"... does not exist"
Comment 5 Simeon Andreev CLA 2019-12-16 04:36:38 EST
The stack trace is from 4.12, our product is not updated to 4.14+. We have many tests that create a project during JUnit set-up and remove it on tear-down. We had multiple issues with not joining the DynamicSourcesWorkingSetUpdater job before deleting the project.

Unfortunately joining the job before deleting the project is apparently not enough. This is the case since we don't join "sleeping" jobs, which the delay-scheduled DynamicSourcesWorkingSetUpdater is. We tried also joining sleeping jobs but that leads to other problems.

I don't know about removing the log from "PackageFragmentRoot.getSourceModuleDescription()". It was done for bug 527131, previously it was a print apparently.
Comment 6 Simeon Andreev CLA 2019-12-16 04:41:23 EST
(In reply to Till Brychcy from comment #4)
> "... does not exist"

Sorry, forgot to mention this. I've edited out the name of the test project, since its product related.
Comment 7 Till Brychcy CLA 2019-12-16 04:57:40 EST
Thanks for the background.

So it the actual problem just the "annoying" error message in the log, or are there any other issues?
Comment 8 Simeon Andreev CLA 2019-12-16 05:09:04 EST
Eclipse is sporadically logging errors in our tests. Unfortunately this is more than annoying; we fail tests on logged errors or warnings (the product is not expected to log anything). We could add an ignore for the error message, but it would be better to fix the code.
Comment 9 Till Brychcy CLA 2019-12-16 05:33:37 EST
@Jay, any objections to replacing "Util.log(e);" by

if (JavaModelManager.VERBOSE) {
	e.printStackTrace();
}
Comment 10 Simeon Andreev CLA 2019-12-16 05:41:23 EST
(In reply to Till Brychcy from comment #9)
> @Jay, any objections to replacing "Util.log(e);" by
> 
> if (JavaModelManager.VERBOSE) {
> 	e.printStackTrace();
> }

How about adding a preference to disable the dynamic working set updater? We have this in our backlog already and our product doesn't need to show dynamic working sets.
Comment 11 Andrey Loskutov CLA 2019-12-16 05:52:20 EST
(In reply to Till Brychcy from comment #9)
> @Jay, any objections to replacing "Util.log(e);" by
> 
> if (JavaModelManager.VERBOSE) {
> 	e.printStackTrace();
> }

I'm not Jay, but yes, I have objections. Eclipse should not spam to the console. Either we log it or we don't catch it, or we don't log it.
Comment 12 Till Brychcy CLA 2019-12-16 06:53:32 EST
(In reply to Andrey Loskutov from comment #11)
> (In reply to Till Brychcy from comment #9)
> > @Jay, any objections to replacing "Util.log(e);" by
> > 
> > if (JavaModelManager.VERBOSE) {
> > 	e.printStackTrace();
> > }
> 
> I'm not Jay, but yes, I have objections. Eclipse should not spam to the
> console. Either we log it or we don't catch it, or we don't log it.

I don't understand you or you don't understand me. JavaModelManager.VERBOSE is false by default, so nothing is spammed, you'd only see something if you explictly enable debug logging.

Completely remove the logging would be ok for me, too. (Not catching would be an API change)

(BTW: I actually copied above snippet from JavaProject)
Comment 13 Till Brychcy CLA 2019-12-16 07:02:45 EST
(In reply to Simeon Andreev from comment #10)

> How about adding a preference to disable the dynamic working set updater? We
> have this in our backlog already and our product doesn't need to show
> dynamic working sets.

A proper preference sounds like quite a bit of work. We wouldn't want to offer stale working sets, so you'd have to remove them when the preference is disabled and recreate them when enabled.

What would be easy would be some non-api internal static boolean, that you can set to disable the updater during tests.

But I think the logging issue should be fixed anyway, as this may be triggered other async jobs as well (and I don't think every java model access should require a workspace lock)
Comment 14 Simeon Andreev CLA 2019-12-16 07:38:34 EST
(In reply to Till Brychcy from comment #13)
> A proper preference sounds like quite a bit of work. We wouldn't want to
> offer stale working sets, so you'd have to remove them when the preference
> is disabled and recreate them when enabled.
> 
> What would be easy would be some non-api internal static boolean, that you
> can set to disable the updater during tests.

A preference available only through product customization ini? Then there is no option to disable/enable in a running Eclipse.

> But I think the logging issue should be fixed anyway, as this may be
> triggered other async jobs as well (and I don't think every java model
> access should require a workspace lock)

Probably we can add a rule for all the involved projects (seems like an unlikely option), or maybe start a rule at each project in the loop that produces the exception. But looking at the code in the updater, I think its meant to be "lightweight".
Comment 15 Till Brychcy CLA 2019-12-17 02:33:48 EST
(In reply to Till Brychcy from comment #12)
> (In reply to Andrey Loskutov from comment #11)
> > (In reply to Till Brychcy from comment #9)
> > > if (JavaModelManager.VERBOSE) {
> > > 	e.printStackTrace();
> > > }
> > 
> > I'm not Jay, but yes, I have objections. Eclipse should not spam to the
> > console. Either we log it or we don't catch it, or we don't log it.
> 
> I don't understand you or you don't understand me. JavaModelManager.VERBOSE [...]

@Andrey, can you please have a look how the VERBOSE flags are used in jdt.core and if you then still have objections, clarify them.
Comment 16 Andrey Loskutov CLA 2019-12-17 04:12:05 EST
(In reply to Till Brychcy from comment #15)
> @Andrey, can you please have a look how the VERBOSE flags are used in
> jdt.core and if you then still have objections, clarify them.

I see your point, but I believe it still makes sense to log the possible errors.

In this concrete case we have a stack that points to some kind of broken (?) JRE state, where we shouldn't actually have any errors, so it is worth look into the real root cause.

Looking closely at the original error log, we see that the error is about *not existing project*, so why does it appear in the stack at all?

Again, this happens because we don't have any locks on workspace, so we have parallel changes on the resources tree that *also* modify the JDT internal data.

So between JavaProject.buildStructure and JavaElement.newJavaModelException the underlined project was deleted. But this shouldn't affect JRE modules detection!

And now the most surprising problem: WHY the code in org.eclipse.jdt.internal.core.JavaProject.defaultRootModules(Iterable<IPackageFragmentRoot>) that is supposed to list module names for system modules stumbles over deleted project? THIS makes no sense for me.

So from this point of view, the log entry was important. We see probably some really weird implementation issue in JDT, where JRE module detection depends on the project state that triggered this detection.

I need to debug that to get understanding why do we fail here...

So beside this really strange finding, I personally think the DynamicSourcesWorkingSetUpdater fUpdateJob should be a regular workspace job with the WS root rule. This way it will never run in any troubles while iterating over ever changing resources.
Comment 17 Andrey Loskutov CLA 2019-12-17 04:50:05 EST
OK, the strange relationship of JavaProject.defaultRootModules on project state is coming from this stack:

Thread [Worker-1: Update dynamic Java sources working sets] (Suspended)	
	JrtPackageFragmentRoot(PackageFragmentRoot).validateOnClasspath() line: 811	
	JrtPackageFragmentRoot(PackageFragmentRoot).validateExistence(IResource) line: 863	
	JrtPackageFragmentRoot(Openable).generateInfos(Object, HashMap, IProgressMonitor) line: 254	
	JrtPackageFragmentRoot(JavaElement).openWhenClosed(Object, boolean, IProgressMonitor) line: 596	
	JrtPackageFragmentRoot(JavaElement).getElementInfo(IProgressMonitor) line: 326	
	JrtPackageFragmentRoot(JavaElement).getElementInfo() line: 312	
	JrtPackageFragmentRoot(JavaElement).getChildren() line: 267	
	JrtPackageFragmentRoot(PackageFragmentRoot).getSourceModuleDescription() line: 911	
	JrtPackageFragmentRoot(PackageFragmentRoot).getModuleDescription() line: 904	
	JrtPackageFragmentRoot(JarPackageFragmentRoot).getModuleDescription() line: 342	
	JrtPackageFragmentRoot.getModule() line: 148	
	JavaProject.lambda$1(IPackageFragmentRoot) line: 800	
	602425664.apply(Object) line: not available	
	JavaProject.internalDefaultRootModules(Iterable<T>, Function<T,String>, Function<T,IModule>) line: 822	
	JavaProject.defaultRootModules(Iterable<IPackageFragmentRoot>) line: 798	
	JavaProject.computePackageFragmentRoots(IClasspathEntry, ObjectVector, HashSet, IClasspathEntry, boolean, boolean, Map, boolean) line: 745	
	JavaProject.computePackageFragmentRoots(IClasspathEntry[], ObjectVector, HashSet, IClasspathEntry, boolean, boolean, Map, boolean) line: 1054	
	JavaProject.computePackageFragmentRoots(IClasspathEntry[], boolean, boolean, Map, boolean) line: 999	
	JavaProject.computePackageFragmentRoots(IClasspathEntry[], boolean, boolean, Map) line: 976	
	JavaProject.buildStructure(OpenableElementInfo, IProgressMonitor, Map, IResource) line: 489	
	JavaProject(Openable).generateInfos(Object, HashMap, IProgressMonitor) line: 268	
	JavaProject(JavaElement).openWhenClosed(Object, boolean, IProgressMonitor) line: 596	
	JavaProject(JavaElement).getElementInfo(IProgressMonitor) line: 326	
	JavaProject(JavaElement).getElementInfo() line: 312	
	JavaProject(JavaElement).getChildren() line: 267	
	JavaProject.getPackageFragmentRoots() line: 2287	
	DynamicSourcesWorkingSetUpdater.updateElements(IWorkingSet[], IProgressMonitor) line: 211	
	DynamicSourcesWorkingSetUpdater.updateElements(IProgressMonitor) line: 192	
	DynamicSourcesWorkingSetUpdater$1.run(IProgressMonitor) line: 160	
	Worker.run() line: 63	

JrtPackageFragmentRoot has as a parent the current project.
So if I delete project while the code is querying validateOnClassPath(), I get this stack on master:

!ENTRY org.eclipse.jdt.core 4 0 2019-12-17 10:48:26.476
!MESSAGE Unexpected internal error
!STACK 1
Java Model Exception: Java Model Status [A2 does not exist]
	at org.eclipse.jdt.internal.core.JavaElement.newJavaModelException(JavaElement.java:583)
	at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:256)
	at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:596)
	at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:326)
	at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:312)
	at org.eclipse.jdt.internal.core.JavaElement.getChildren(JavaElement.java:267)
	at org.eclipse.jdt.internal.core.PackageFragmentRoot.getSourceModuleDescription(PackageFragmentRoot.java:911)
	at org.eclipse.jdt.internal.core.PackageFragmentRoot.getModuleDescription(PackageFragmentRoot.java:904)
	at org.eclipse.jdt.internal.core.JarPackageFragmentRoot.getModuleDescription(JarPackageFragmentRoot.java:342)
	at org.eclipse.jdt.internal.core.JrtPackageFragmentRoot.getModule(JrtPackageFragmentRoot.java:148)
	at org.eclipse.jdt.internal.core.JavaProject.lambda$1(JavaProject.java:800)
	at org.eclipse.jdt.internal.core.JavaProject.internalDefaultRootModules(JavaProject.java:822)
	at org.eclipse.jdt.internal.core.JavaProject.defaultRootModules(JavaProject.java:798)
	at org.eclipse.jdt.internal.core.JavaProject.computePackageFragmentRoots(JavaProject.java:745)
	at org.eclipse.jdt.internal.core.JavaProject.computePackageFragmentRoots(JavaProject.java:1054)
	at org.eclipse.jdt.internal.core.JavaProject.computePackageFragmentRoots(JavaProject.java:999)
	at org.eclipse.jdt.internal.core.JavaProject.computePackageFragmentRoots(JavaProject.java:976)
	at org.eclipse.jdt.internal.core.JavaProject.buildStructure(JavaProject.java:489)
	at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:268)
	at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:596)
	at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:326)
	at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:312)
	at org.eclipse.jdt.internal.core.JavaElement.getChildren(JavaElement.java:267)
	at org.eclipse.jdt.internal.core.JavaProject.getPackageFragmentRoots(JavaProject.java:2287)
	at org.eclipse.jdt.internal.ui.workingsets.DynamicSourcesWorkingSetUpdater.updateElements(DynamicSourcesWorkingSetUpdater.java:211)
	at org.eclipse.jdt.internal.ui.workingsets.DynamicSourcesWorkingSetUpdater.updateElements(DynamicSourcesWorkingSetUpdater.java:192)
	at org.eclipse.jdt.internal.ui.workingsets.DynamicSourcesWorkingSetUpdater$1.run(DynamicSourcesWorkingSetUpdater.java:160)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)
Comment 18 Till Brychcy CLA 2019-12-17 05:49:14 EST
Still sounds like you think that the java model should only be accessed with a workspace lock, but I understood it was built with concurrency in mind, see e.g. org.eclipse.jdt.internal.core.JavaModelManager.putInfos.

Personally I'd probably simply not have catched the JavaModelException in PackageFragmentRoot.getSourceModuleDescription but thrown it to the caller, but I guess the assumption was that a null result was good enough for the usages, and the original printStackTrace was just the generated default action.

As we know understand that this situation can happen when the project is deleted, I think we can omit the logging (maybe unless VERBOSE is set).

If you think, it still might be interesting to log for other situations, we could also modify the catch handler so it logs only if the project still exists (using the normal log infrastructure)
Comment 19 Andrey Loskutov CLA 2019-12-17 07:29:11 EST
(In reply to Till Brychcy from comment #18)
> Still sounds like you think that the java model should only be accessed with
> a workspace lock, but I understood it was built with concurrency in mind,
> see e.g. org.eclipse.jdt.internal.core.JavaModelManager.putInfos.

It depends. The work that DynamicSourcesWorkingSetUpdater is doing is "optional" in the sense, it is not really needed for core JDT to function properly. There is no UI waiting for us, no user action hanging, etc. We are in the job, and so DynamicSourcesWorkingSetUpdater can wait till it gets the time where workspace is not locked anymore. Also the work it does is not supposed to block workspace for a long time - it "only" inspects the classpath of Java projects - no midification, no extra work needed (in theory). All what I say here, is that for such kind of work we can acquire workspace lock and do whatever we want in a "safe" way, not being disturbed by any parallel workspace modifications.

> Personally I'd probably simply not have catched the JavaModelException in
> PackageFragmentRoot.getSourceModuleDescription but thrown it to the caller,
> but I guess the assumption was that a null result was good enough for the
> usages, and the original printStackTrace was just the generated default
> action.

Fully agree.

> As we know understand that this situation can happen when the project is
> deleted, I think we can omit the logging (maybe unless VERBOSE is set).

Still not sure if we can distinguish which kind of error it is and if it shuold be suppressed. JDT is a very complex beast, and there are probably many ways where something can go wrong and throw JavaModelException.

But I wonder - do really need to depend on a project state in the JrtPackageFragmentRoot.validateOnClasspath()?



JrtPackageFragmentRoot.validateOnClasspath() is called from three places:
1) CommitWorkingCopyOperation.executeOperation() - irrelevant for JrtPackageFragmentRoot
2) org.eclipse.jdt.internal.core.Openable.exists() - should be OK, since JRE exists without any project
3) org.eclipse.jdt.internal.core.Openable.generateInfos(Object, HashMap, IProgressMonitor) - where it does not access any resource reference because it is unrelated to any resource

At least tests are fine if we don't check anything at all: https://git.eclipse.org/r/154645

> If you think, it still might be interesting to log for other situations, we
> could also modify the catch handler so it logs only if the project still
> exists (using the normal log infrastructure)

Coming from "our" case I don't see this will be enough. As long as DynamicSourcesWorkingSetUpdater is running in parallel to all the tests, it will hit us just on another place again I guess.

So from my point of view, we could:
1) Add Workspace rule to DynamicSourcesWorkingSetUpdater fUpdateJob
2) Investigate if we want https://git.eclipse.org/r/154645.
Comment 20 Stephan Herrmann CLA 2019-12-17 15:50:50 EST
(In reply to Andrey Loskutov from comment #19)
> But I wonder - do really need to depend on a project state in the
> JrtPackageFragmentRoot.validateOnClasspath()?

The method actually is from PackageFragmentRoot (so you might be asking if JrtPFR could override with simple "return JavaModelState.VERIFIED_OK").

The introduction of this method came with bug 190094, commented thusly:
> Note that this patch is not an obvious change. It involves the rewriting of
> critical parts of the Java model.

D'uh

I believe the reasoning is: an IJavaElement can be in one of two states:

- it does not exists() and methods are not guaranteed to give meaningful results 

or

- it exists() and accessors give meaningful results including getParent() which will always provide a valid path up to the JavaModel

So, even if conceptually a JrtPackageFragmentRoot needs no links to anything else, it should only be legal to open() when the path to the JavaModel can be established, and that path *must* go via an IJavaProject.

Additionally, and more recently, even a JrtPackageFragmentRoot has a dependency on the IJavaProject through which it is accessed: the project is where things like LIMIT-MODULES, PATH-MODULE etc are defined and woven into the modules.
Comment 21 Till Brychcy CLA 2020-01-01 12:30:01 EST
(In reply to Andrey Loskutov from comment #19)
> > If you think, it still might be interesting to log for other situations, we
> > could also modify the catch handler so it logs only if the project still
> > exists (using the normal log infrastructure)
> 
> Coming from "our" case I don't see this will be enough. As long as
> DynamicSourcesWorkingSetUpdater is running in parallel to all the tests, it
> will hit us just on another place again I guess.

Can you elaborate what kind of problems you've seen or that you expect? The only expected possible side effects are a prepopulation of the jdt model cache, which should change any behaviour for other threads. If that is a problem for your tests, we could add a flag as written in Comment 13.

> 
> So from my point of view, we could:
> 1) Add Workspace rule to DynamicSourcesWorkingSetUpdater fUpdateJob
We don't want to it to unnecessarily block any other jobs, and, as I already wrote, I understand that the jdt model allows concurrent access (and I've seen some other plugins that e.g. do async decoration computations and IIRC they didn't use any locking either)

> 2) Investigate if we want https://git.eclipse.org/r/154645.
Stephan ruled that out.
Comment 22 Till Brychcy CLA 2020-01-01 12:31:20 EST
(In reply to Till Brychcy from comment #9)
> @Jay, any objections to replacing "Util.log(e);" by
> 
> if (JavaModelManager.VERBOSE) {
> 	e.printStackTrace();
> }

@Jay, have you seen this?
Comment 23 Andrey Loskutov CLA 2020-02-29 18:01:31 EST
I've stumbled (actually tests) recently again over that. 

It is not just that this dynamic working set updater runs in parallel to workspace tasks and disturbs our tests, it slows down startup. Every time I start Eclipse I see this job in the progress view, and it is not the fast/cheap one. Why should we run it at all, for over 200 projects? I never ever used search with this two working sets, and I don't know anyone who does that in our lab. I don't see any value in coloring test packages to different color, they are anyway in different bundles in our environment. All the work and troubles done for nothing in our environment.

I think this working set initial population must be done on demand, and it will solve all the problems we have. If no one uses that, it will cost nothing on startup, and it will not disturb users and tests afterwards. If one uses that, only first time access will be longer, but this is similar to type search if the index not updated yet.
Comment 24 Till Brychcy CLA 2020-03-01 06:12:54 EST
(In reply to Andrey Loskutov from comment #23)
> I've stumbled (actually tests) recently again over that. 
> 
> It is not just that this dynamic working set updater runs in parallel to
> workspace tasks and disturbs our tests, it slows down startup. Every time I
> start Eclipse I see this job in the progress view, and it is not the
> fast/cheap one.

For me it is extremely fast and I cannot see it all actually active in the progress view. If you enable "show sleeping and system operations", you can see that it waits for long time to be scheduled in the ui thread, while decorations are being computed.

I think the real problem is that it is the only non-system job that is active during that time, and it is therefore shown as active task in the status bar, so people think they are waiting for it.

@Andrey, I think it was your idea to turn DynamicSourcesWorkingSetUpdater.UpdateUIJob into a non-system job. I think we should revert that. WDYT?

As simple experiment: if I remove the r.run() from DynamicSourcesWorkingSetUpdater.UpdateUIJob.runInUIThread,
it doesn't do anything and I still see the "Updating dynamic working sets" in the status bar.
Comment 25 Andrey Loskutov CLA 2020-03-01 06:22:23 EST
(In reply to Till Brychcy from comment #24) 
> @Andrey, I think it was your idea to turn
> DynamicSourcesWorkingSetUpdater.UpdateUIJob into a non-system job. I think
> we should revert that. WDYT?

This would not fix our random test errors, I'm looking for a real solution, and I personally believe lazy loading would be the way to go, as opposite to the current state. I'm looking into WorkingSet & Co API, if there is a way to avoid all this unneeded work by moving all the updates & listeners init that DynamicSourcesWorkingSetUpdater does from very begin to the point in time where the actual content of the working set is really requested (would be probably never in our case). At same time we could skip persisting / restore work of both working sets too, as they are anyway dynamic.
Comment 26 Till Brychcy CLA 2020-03-01 06:27:10 EST
(In reply to Andrey Loskutov from comment #25)
> (In reply to Till Brychcy from comment #24) 
> > @Andrey, I think it was your idea to turn
> > DynamicSourcesWorkingSetUpdater.UpdateUIJob into a non-system job. I think
> > we should revert that. WDYT?
> 
> This would not fix our random test errors, I'm looking for a real solution,

Still users currently get a wrong impression.


> be probably never in our case). At same time we could skip persisting /
> restore work of both working sets too, as they are anyway dynamic.

yes, this is bug 551152. adding platform api to avoid persisting them should be easy
Comment 27 Andrey Loskutov CLA 2020-03-02 02:31:27 EST
(In reply to Andrey Loskutov from comment #25)
> (In reply to Till Brychcy from comment #24) 
> > @Andrey, I think it was your idea to turn
> > DynamicSourcesWorkingSetUpdater.UpdateUIJob into a non-system job. I think
> > we should revert that. WDYT?
> 
> This would not fix our random test errors, I'm looking for a real solution,
> and I personally believe lazy loading would be the way to go, as opposite to
> the current state. I'm looking into WorkingSet & Co API, if there is a way
> to avoid all this unneeded work by moving all the updates & listeners init
> that DynamicSourcesWorkingSetUpdater does from very begin to the point in
> time where the actual content of the working set is really requested (would
> be probably never in our case). At same time we could skip persisting /
> restore work of both working sets too, as they are anyway dynamic.

See https://git.eclipse.org/r/158650 patch series (WIP, requires  https://git.eclipse.org/r/158649).
Comment 28 Till Brychcy CLA 2020-03-02 03:15:27 EST
(In reply to Andrey Loskutov from comment #27)
> See https://git.eclipse.org/r/158650 patch series (WIP, requires 
> https://git.eclipse.org/r/158649).

Looks interesting, but it might be better to move it to a dedicated bug. 
While it probably solves your test issues, the current bug is about concurrent access to projects being deleted. (which is not a bug, but the log shouldn't be spammed about it, which should be fixed, as I wrote in comment 9.)
Comment 29 Andrey Loskutov CLA 2020-03-02 04:15:43 EST
(In reply to Till Brychcy from comment #28)
> (In reply to Andrey Loskutov from comment #27)
> > See https://git.eclipse.org/r/158650 patch series (WIP, requires 
> > https://git.eclipse.org/r/158649).
> 
> Looks interesting, but it might be better to move it to a dedicated bug. 

Right, I think I will use bug 551676 for that.
Comment 30 Eclipse Genie CLA 2022-05-28 00:41:34 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.