Comment 1 Ed Willink 2021-07-07 05:56:04 EDT

(In reply to Ed Willink from comment #0)
> Following a Windows update, 
> https://support.microsoft.com/en-us/topic/july-6-2021-kb5004945-os-builds-
> 19041-1083-19042-1083-and-19043-1083-out-of-band-44b34928-0a71-4473-aa22-
> ecf3b83eed0e
> Eclipse crashes repeatedly while analyzing the
> org.eclipse.ocl.examples.build blackbox. (5 identical crashes so far).

KB5004945 seems to be an emergency update released to fix PrintNightmare chnaging security policies in ways that no doubt outwit Java 11.

Seems unlikely that QVTo's Java usage is the only thing that will be affected.

But if Microsoft do not retract, QVTo may need to adjust its code to avoid the crash.

Comment 2 Ed Willink 2021-07-08 02:24:36 EDT

(In reply to Ed Willink from comment #1)
> Seems unlikely that QVTo's Java usage is the only thing that will be
> affected.

Bug 574693 reports a different problem that might also be due to changed security access and so this problem. Otherwise, 24 hours on, cross-project-dev seems quiet, so it seems unlikely that an adequate bandwagon will force a fast Microsoft resolution.
 
> But if Microsoft do not retract, QVTo may need to adjust its code to avoid
> the crash.

A possible 'explanation' is that in the absence of a definitive Microsoft specification of its OS, the VM exploits empirical discovery to poll / respond to notifications from the OS. The Microsoft fix changes the timing of notifications and the QVTo style of usage catches the VM in an unexpected state.

e.g. suppose a File Node was previously created with unrestricted access, then tightened up when contents were added, but is now created with zero access that is loosened when contents are added. (This could prevent a security breach by an application that continued to use an early unrestricted file handle.)

Anyway, we need to identify what is distinctive / provocative about the QVTo access so that we can do something slightly different to workaround the bug. Is a specialized ClassLoader in use?

Comment 3 Ed Willink 2021-07-08 03:20:06 EDT

Oops I was (still) using

# JRE version: OpenJDK Runtime Environment (11.0.1+13) (build 11.0.1+13)
# Java VM: OpenJDK 64-Bit Server VM (11.0.1+13, mixed mode, tiered, compressed oops, g1 gc, windows-amd64)

Switching to jdk-11.0.11_windows-x64_bin.exe is more instructure.

With org.eclipse.emf.ecore open in my workspace, I get

java.lang.LinkageError: loader constraint violation for class org.eclipse.ocl.examples.build.utilities.UMLStandaloneSetup: when selecting overriding method org.eclipse.ocl.examples.build.utilities.UMLStandaloneSetup.setResourceSet(Lorg/eclipse/emf/ecore/resource/ResourceSet;)V the class loader org.eclipse.m2m.internal.qvt.oml.jdt.runtime.blackbox.ProjectClassLoader @5c75ba1f (instance of org.eclipse.m2m.internal.qvt.oml.jdt.runtime.blackbox.ProjectClassLoader, child of org.eclipse.m2m.internal.qvt.oml.jdt.runtime.blackbox.ProjectClassLoader$1 @3a29d118 org.eclipse.m2m.internal.qvt.oml.jdt.runtime.blackbox.ProjectClassLoader$1) of the selected method's type org.eclipse.ocl.examples.build.utilities.UMLStandaloneSetup, and the class loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @2e0af4da (instance of org.eclipse.osgi.internal.loader.EquinoxClassLoader, child of 'platform' jdk.internal.loader.ClassLoaders$PlatformClassLoader) for its super type org.eclipse.emf.mwe.utils.StandaloneSetup have different Class objects for the type org.eclipse.emf.ecore.resource.ResourceSet used in the signature
	at java.base/java.lang.Class.getDeclaredConstructors0(Native Method)
	at java.base/java.lang.Class.privateGetDeclaredConstructors(Class.java:3137)
	at java.base/java.lang.Class.getConstructor0(Class.java:3342)
	at java.base/java.lang.Class.getDeclaredConstructor(Class.java:2553)
	at org.eclipse.m2m.internal.qvt.oml.blackbox.java.JavaModuleLoader.isModuleClassValid(JavaModuleLoader.java:127)
	at org.eclipse.m2m.internal.qvt.oml.blackbox.java.JavaModuleLoader.loadModule(JavaModuleLoader.java:58)
	at org.eclipse.m2m.internal.qvt.oml.blackbox.java.JavaBlackboxProvider$JavaUnitDescriptor.load(JavaBlackboxProvider.java:197)
	at org.eclipse.m2m.internal.qvt.oml.jdt.runtime.blackbox.JdtBlackboxProvider$JdtDescriptor.load(JdtBlackboxProvider.java:268)
	at org.eclipse.m2m.internal.qvt.oml.blackbox.BlackboxProvider.getBlackboxCallHandler(BlackboxProvider.java:127)
	at org.eclipse.m2m.internal.qvt.oml.blackbox.BlackboxRegistry.getBlackboxCallHandler(BlackboxRegistry.java:80)
	at org.eclipse.m2m.internal.qvt.oml.ast.parser.QvtOperationalVisitorCS.visitMappingMethodCS(QvtOperationalVisitorCS.java:3750)
	at org.eclipse.m2m.internal.qvt.oml.ast.parser.QvtOperationalVisitorCS.visitMethodBodies(QvtOperationalVisitorCS.java:2412)
	at org.eclipse.m2m.internal.qvt.oml.ast.parser.QvtOperationalVisitorCS.visitUnitCS(QvtOperationalVisitorCS.java:2109)
	at org.eclipse.m2m.internal.qvt.oml.compiler.QVTOCompiler.analyze(QVTOCompiler.java:279)
	at org.eclipse.m2m.internal.qvt.oml.compiler.QVTOCompiler.doCompile(QVTOCompiler.java:444)
	at org.eclipse.m2m.internal.qvt.oml.compiler.QVTOCompiler.compileSingleFile(QVTOCompiler.java:325)
	at org.eclipse.m2m.internal.qvt.oml.compiler.QVTOCompiler.compile(QVTOCompiler.java:183)
	at org.eclipse.m2m.internal.qvt.oml.project.builder.QVTOBuilder.rebuildAll(QVTOBuilder.java:216)
	at org.eclipse.m2m.internal.qvt.oml.project.builder.QVTOBuilder.fullBuild(QVTOBuilder.java:94)
	at org.eclipse.m2m.internal.qvt.oml.project.builder.QVTOBuilder.build(QVTOBuilder.java:81)
	at org.eclipse.core.internal.events.BuildManager$2.run(BuildManager.java:846)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
	at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:229)
	at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:277)
	at org.eclipse.core.internal.events.BuildManager$1.run(BuildManager.java:330)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
	at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:333)
	at org.eclipse.core.internal.events.BuildManager.basicBuildLoop(BuildManager.java:385)
	at org.eclipse.core.internal.events.BuildManager.build(BuildManager.java:406)
	at org.eclipse.core.internal.events.AutoBuildJob.doBuild(AutoBuildJob.java:154)
	at org.eclipse.core.internal.events.AutoBuildJob.run(AutoBuildJob.java:244)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)

If I close org.eclipse.emf.ecore, but have org.eclipse.ocl open I get 

java.lang.LinkageError: loader constraint violation in interface itable initialization for class org.eclipse.m2m.internal.qvt.oml.ast.parser.CustomOclValidationVisitor: when selecting method org.eclipse.ocl.utilities.Visitor.visitVariable(Lorg/eclipse/ocl/expressions/Variable;)Ljava/lang/Object; the class loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @57496422 (instance of org.eclipse.osgi.internal.loader.EquinoxClassLoader, child of 'platform' jdk.internal.loader.ClassLoaders$PlatformClassLoader) for super interface org.eclipse.ocl.utilities.Visitor, and the class loader org.eclipse.m2m.internal.qvt.oml.jdt.runtime.blackbox.ProjectClassLoader @11464d99 (instance of org.eclipse.m2m.internal.qvt.oml.jdt.runtime.blackbox.ProjectClassLoader, child of org.eclipse.m2m.internal.qvt.oml.jdt.runtime.blackbox.ProjectClassLoader$1 @5d21a7b2 org.eclipse.m2m.internal.qvt.oml.jdt.runtime.blackbox.ProjectClassLoader$1) of the selected method's type, org.eclipse.ocl.parser.ValidationVisitor have different Class objects for the type org.eclipse.ocl.expressions.Variable used in the signature
	at java.base/java.lang.Class.getDeclaredConstructors0(Native Method)
	at java.base/java.lang.Class.privateGetDeclaredConstructors(Class.java:3137)
	at java.base/java.lang.Class.getConstructor0(Class.java:3342)
	at java.base/java.lang.Class.getDeclaredConstructor(Class.java:2553)
	at org.eclipse.m2m.internal.qvt.oml.blackbox.java.JavaModuleLoader.isModuleClassValid(JavaModuleLoader.java:127)
	at org.eclipse.m2m.internal.qvt.oml.blackbox.java.JavaModuleLoader.loadModule(JavaModuleLoader.java:58)
	at org.eclipse.m2m.internal.qvt.oml.blackbox.java.JavaBlackboxProvider$JavaUnitDescriptor.load(JavaBlackboxProvider.java:197)
	at org.eclipse.m2m.internal.qvt.oml.jdt.runtime.blackbox.JdtBlackboxProvider$JdtDescriptor.load(JdtBlackboxProvider.java:268)
	at org.eclipse.m2m.internal.qvt.oml.blackbox.BlackboxProvider.getBlackboxCallHandler(BlackboxProvider.java:127)
	at org.eclipse.m2m.internal.qvt.oml.blackbox.BlackboxRegistry.getBlackboxCallHandler(BlackboxRegistry.java:80)
	at org.eclipse.m2m.internal.qvt.oml.ast.parser.QvtOperationalVisitorCS.visitMappingMethodCS(QvtOperationalVisitorCS.java:3750)
	at org.eclipse.m2m.internal.qvt.oml.ast.parser.QvtOperationalVisitorCS.visitMethodBodies(QvtOperationalVisitorCS.java:2412)
	at org.eclipse.m2m.internal.qvt.oml.ast.parser.QvtOperationalVisitorCS.visitUnitCS(QvtOperationalVisitorCS.java:2109)
	at org.eclipse.m2m.internal.qvt.oml.compiler.QVTOCompiler.analyze(QVTOCompiler.java:279)
	at org.eclipse.m2m.internal.qvt.oml.compiler.QVTOCompiler.doCompile(QVTOCompiler.java:444)
	at org.eclipse.m2m.internal.qvt.oml.compiler.QVTOCompiler.compileSingleFile(QVTOCompiler.java:325)
	at org.eclipse.m2m.internal.qvt.oml.compiler.QVTOCompiler.compile(QVTOCompiler.java:183)
	at org.eclipse.m2m.internal.qvt.oml.project.builder.QVTOBuilder.rebuildAll(QVTOBuilder.java:216)
	at org.eclipse.m2m.internal.qvt.oml.project.builder.QVTOBuilder.incrementalBuild(QVTOBuilder.java:140)
	at org.eclipse.m2m.internal.qvt.oml.project.builder.QVTOBuilder.build(QVTOBuilder.java:83)
	at org.eclipse.core.internal.events.BuildManager$2.run(BuildManager.java:846)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
	at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:229)
	at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:277)
	at org.eclipse.core.internal.events.BuildManager$1.run(BuildManager.java:330)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
	at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:333)
	at org.eclipse.core.internal.events.BuildManager.basicBuildLoop(BuildManager.java:385)
	at org.eclipse.core.internal.events.BuildManager.build(BuildManager.java:406)
	at org.eclipse.core.internal.events.AutoBuildJob.doBuild(AutoBuildJob.java:154)
	at org.eclipse.core.internal.events.AutoBuildJob.run(AutoBuildJob.java:244)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)

These are very close to the same line as the original crash. They appear to show that some class loader, probably org.eclipse.m2m.internal.qvt.oml.jdt.runtime.blackbox.ProjectClassLoader, is loading both plugin and project classes and getting confused.

Comment 4 Ed Willink 2021-07-08 03:30:02 EDT

(In reply to Ed Willink from comment #3)
> If I close org.eclipse.emf.ecore, but have org.eclipse.ocl open I get 
> 
> They appear to
> show that some class loader, probably
> org.eclipse.m2m.internal.qvt.oml.jdt.runtime.blackbox.ProjectClassLoader, is
> loading both plugin and project classes and getting confused.

If I close the org.eclipse.ocl project, building incurs no errors.

Why is any class loader using workspace class files?

Comment 5 Ed Willink 2021-07-08 07:29:23 EDT

Fortuitously my tower had not yet applied the Windows Update, so I create a very similar 4.20-based installation in which everything builds as normal without issue.

After applying the Windows Update (painful: it seemed stuck after 45 mins, so power off and on and eventual success.) with org.ecipse.ocl open builds no problem. After opening org.eclipse.emf.ecore the comment#3 stack trace recurs.

After closing org.eclipse.emf.ecore the problem still occurs. But this is probably the standard JDT/PDE bug whereby closing a project after its classes were in use doesn't always work. Restarting and cleaning with org.eclipse.emf.ecore closed, org.eclipse.ocl open builds ok. Restarting and cleaning with org.eclipse.emf.ecore open, org.eclipse.ocl open and the build failure on two ResourceSet classes occurs.

So there is a clear but slightly inconsistent correlation with the Windows Update.

Suspicion: the Update can cause some low level, almost internal, aspect of a *.class file node to change so that the Java Class Loader now thinks that the two alternative classes on the classpath are different.

If so, the solution is to make sure that there are never two alternatives on the classpath in the first place. Fix the QVTo usage to exclude the plugin when the project class is available.

Comment 6 Ed Willink 2021-07-08 07:53:56 EDT

(In reply to Ed Willink from comment #5)
> slightly inconsistent correlation

The inconsistency can be explained by whether the project files, and so classes, for org.eclipse.ocl and org.eclipse.emf.ecore are identical to the 2021-06 release. For org.eclipse.ocl on my tower they matched, for org.eclipse.emf.ecore they were different.

This strengthens the suspicion that the QVTo class loader is allowing both project and plugin classes to conflict.

Prior to the KB5004945 Windows Update, either redundant plugins were ignored or were compared solely by name.

Subsequent to the Windows Update, both project and plugin classes are considered and subtle internal differences are detected.

Comment 7 Christopher Gerking 2021-07-08 09:15:52 EDT

(In reply to Ed Willink from comment #4) 
> Why is any class loader using workspace class files?
Blackbox development in the workspace sometimes requires project classes to act as blackboxes.


(In reply to Ed Willink from comment #3)
> They appear to show that some class loader, probably
> org.eclipse.m2m.internal.qvt.oml.jdt.runtime.blackbox.ProjectClassLoader, is
> loading both plugin and project classes
Indeed, it's due to bug 573752, which prevents plugin classes from being loaded again.


> ... and getting confused.
This shouldn't happen. Will try to reproduce.


(In reply to Ed Willink from comment #5)
> If so, the solution is to make sure that there are never two alternatives on
> the classpath in the first place. Fix the QVTo usage to exclude the plugin
> when the project class is available.
That's exactly the intention of ProjectClassLoader.getProjectClassPath(...), which aims to exclude plugin classes. Instead, ProjectClassLoader.loadClass(...) implements a delegation to the corresponding bundle class loader, which loads plugin classes that cannot be found among the project classes. The probem could be that loading some referenced classes will invoke the bundle class loader's loadClass(...) directly instead of ProjectClassLoader.loadClass(...), which should always be invoked first.

Comment 8 Ed Willink 2021-07-08 09:36:26 EDT

Given that when invoked from the builder, the class file access is 100% standard, why is there any special class loader at all? Eclipse already sorts out hiding plugins when projects are in use.

Comment 9 Christopher Gerking 2021-07-08 10:29:44 EDT

(In reply to Ed Willink from comment #8)
> Given that when invoked from the builder, the class file access is 100%
> standard, why is there any special class loader at all?
Maybe I got something wrong, but how to obtain the project classes from the builder? QVTo's blackbox API requires Class<?> objects to work. If there is an easy way to obtain a Class<?> from the workspace, I'm happy to go that way.

Comment 10 Ed Willink 2021-07-08 13:43:46 EDT

Ah! Subtle.

The classes exist, but it would be good if the Java nature/builder precedes the QVTo nature. (Ideally a QVTo builder warning.)

Provided the blackbox-hosting MANIFEST has a credible classpath, then you just need to ensure that the blackbox class is loaded by the blackbox-hosting plugin's ClassLoader. Clearly a QVTo plugin's ClassLoader won't do since the user class is not on any QVTo plugin classpath.

Presumably you have no guaranteed class to latch on to in the blackbox-hosting project, so you need to use the correct workspace find project facility to locate the bundle and so access the class loader.

You might get away with

	Thread currentThread = Thread.currentThread();
	ClassLoader contextClassLoader = currentThread.getContextClassLoader();
	Class<?> loadedClass = contextClassLoader.loadClass(qualifiedInterfaceName);

Else have a look at 

org.eclipse.ocl.xtext.base.ui.builder.MultiValidationJob.getClassLoader()

Comment 11 Christopher Gerking 2021-07-08 15:45:27 EDT

(In reply to Ed Willink from comment #10)
> Provided the blackbox-hosting MANIFEST has a credible classpath, then you
> just need to ensure that the blackbox class is loaded by the
> blackbox-hosting plugin's ClassLoader.
But the hosting plugin doesn't have a class loader because it exists only in the form of a plugin project.


> so you need to use the correct workspace find
> project facility to locate the bundle and so access the class loader.
The Bundle doesn't provide access to any class loader. It is possible to obtain a Bundle via Platform.getBundle(project.getName()), which also provides a corresponding BundleWiring. But the BundleWiring doesn't return a ClassLoader, which conforms to the Javadoc of BundleWiring.getClassLoader():

"If this bundle wiring is not in use or this bundle wiring is for a fragment revision, null will be returned."

 
> Else have a look at 
> 
> org.eclipse.ocl.xtext.base.ui.builder.MultiValidationJob.getClassLoader()
That's basically what QVTo is already doing.

Comment 12 Ed Willink 2021-07-09 02:50:46 EDT

(In reply to Christopher Gerking from comment #11)
> (In reply to Ed Willink from comment #10)
> > Provided the blackbox-hosting MANIFEST has a credible classpath, then you
> > just need to ensure that the blackbox class is loaded by the
> > blackbox-hosting plugin's ClassLoader.
> But the hosting plugin doesn't have a class loader because it exists only in
> the form of a plugin project.

I'm assuming that the QVTo nature/builder is just a *.qvto-to-problem-markers transformation and so could be in general project devoid of Java support, but it seems pretty odd to have Java black boxes in a non-Java project. I'm certainly happy to forbid this crazy case so long as we are struggling to make the sensible case working.

Once a project has Java, and possibly even if it doesn't, the QVTo nature/builder is activated in a worker thread on behalf of its hosting plugin and so will have a context class loader reflecting the classpath of the hosting project. So provided the user observes the constraint that all referenced Java is on the classpath, the context class loader should find all referenced Java code. (I recall off-classpath model references being a problem with at least Acceleo and possibly ATL too.)

Comment 13 Christopher Gerking 2021-07-09 12:45:08 EDT

(In reply to Ed Willink from comment #12)
> Once a project has Java, and possibly even if it doesn't, the QVTo
> nature/builder is activated in a worker thread on behalf of its hosting
> plugin
For testing purposes, I registered an org.eclipse.jdt.core.compiler.CompilationParticipant to see if the Java Builder provides a meaningful context class loader. It turns out that the context class loader is a ContextFinder. When it loads a class, it consults an EquinoxClassLoader for the plugin project that contains the CompilationParticipant (in my case, that's org.eclipse.m2m.qvt.oml.runtime.jdt).

> and so will have a context class loader reflecting the classpath of
> the hosting project. 
The thing is that we need a class loader that reflects the classpath of the project being built. The provided loader cannot load any of the classes inside the building project.

Comment 14 Ed Willink 2021-07-12 06:44:09 EDT

(In reply to Christopher Gerking from comment #13)
> The thing is that we need a class loader that reflects the classpath of the
> project being built. The provided loader cannot load any of the classes
> inside the building project.

I just created a trivial Ecore project in a nested Eclipse with an OCL constraint and genmodelled, then edited the Java to throw an ISE. EMF Validation worked as normal but using the OCL source rather than the corrupted generated Java.

I then instrumented the MultiValidationJob to load one of the above EMF classes and the loading succeeded demonstrating that the project class loader successfully loaded from the project classes. (NB probably need "." on classpath and relevant packages exported and a java nature / builder with Java class consumers later. Oops the MultiValidationJob from the OCL nature precedes the Java builder that produces classes the MultiValidationJob might use if its thread runs too quick.)

(The generated Java is not used for EMF validation because nothing puts the XXXValidator.INSTANCE into the Validation Registry. This is at least an EMF feature if not a bug; support for Dynamic EMF has been consistently patchy.)

Bottom line, the MultiValidationJob runs with a project-blind classpath and successfully obtains a ClassLoader from the Project's BundleWiring that successfully loads classes that only exist in the project's bin folder.

The QVTo nature/builder should start up with a project-aware class loader and so Thread.getContextClassLoader() should work.

(When I edited the MultiValidationJob, the QVTo blackbox analyzer took multiple long times so that I was eventually forced to kill it.)

Comment 15 Christopher Gerking 2021-07-12 07:37:28 EDT

(In reply to Ed Willink from comment #14)
> I then instrumented the MultiValidationJob to load one of the above EMF
> classes and the loading succeeded demonstrating that the project class
> loader successfully loaded from the project classes.
Of course loading succeeds, since MultiValidationJob turns the full classpath into an URLClassLoader that reloads all classes from scratch. This is exactly what QVTo did for 2021-03. But it gets more complicated when classes from required bundles are not to be reloaded, but reused from the platform. This is exactly what QVTo is trying to do for 2021-06 by including the appropriate bundle class loaders (see bug 573752). 

This bug is even more complicated because a platform bundle (such as org.eclipse.emf.ecore) is hidden by a workspace project. This invalidates the class loaders of all bundles that depend on org.eclipse.emf.ecore, resulting in the bug at hand. It is quite difficult to figure out which classes can be safely reused from the platform and which classes must be reloaded due to hiding.

I suggest a conservative approach: If there is at least one project that hides a required bundle, then reload all classes using the URLClassLoader. Otherwise, prevent classes from reloading by using the bundle class loaders.

Comment 16 Ed Willink 2021-07-12 07:46:28 EDT

(In reply to Ed Willink from comment #14)
> (The generated Java is not used for EMF validation because nothing puts the
> XXXValidator.INSTANCE into the Validation Registry. This is at least an EMF
> feature if not a bug; support for Dynamic EMF has been consistently patchy.)

When I try to put the relevant Validator in the Validator.Registry I get CCEs.

a) The Validator inherits from the 'wrong' EObjectValidator
b) The EPackage inherits from the 'wrong' EPackage

It seems that the BundleWiring is not re-using dependent class loaders.

I suspect that this may be a 'modules' problem which I have observed causes significant trouble for many Java versions that are worked around by e.g.

--add-modules=ALL-SYSTEM

in Eclipse.ini. I wonder if some other magic is required for this project class loading.

Presumably any blackbox will fail through inheriting the 'wrong' org.eclipse.m2m.qvt.oml.blackbox.java.Module

When I comment out the BundleWiring part of MultiValidationJob.getClassLoader to force the URLClassLoader I see exactly the same failure since the URLClassLoader has the full classpath enabling it to reload rather than delegate.

If there is no 'modules' solution then maybe org.eclipse.ocl.examples.codegen.dynamic.ExplicitClassLoader is the solution. It has a very narrow loading capability designed to ensure that dynamically generated classes could be reloaded after each regeneration.

Comment 17 Eclipse Genie 2021-08-31 06:58:15 EDT

New Gerrit change created: https://git.eclipse.org/r/c/mmt/org.eclipse.qvto/+/184801

Comment 18 Christopher Gerking 2021-08-31 13:03:42 EDT

(In reply to Christopher Gerking from comment #15)
> It is quite difficult to figure out which
> classes can be safely reused from the platform and which classes must be
> reloaded due to hiding.
Done in commit 38cf9255553ccb5d6cfe7631b2ed4dffd4b088fc, so that classes loaded from the workspace are now clearly separated from those loaded from a bundle.

With org.eclipse.emf.ecore open in the workspace, the LinkageError does no longer appear for me. Can you confirm?

Comment 19 Eclipse Genie 2021-09-02 02:23:59 EDT

New Gerrit change created: https://git.eclipse.org/r/c/mmt/org.eclipse.qvto/+/184904

Comment 20 Eclipse Genie 2021-09-05 06:08:49 EDT

New Gerrit change created: https://git.eclipse.org/r/c/mmt/org.eclipse.qvto/+/184987

Comment 21 Eclipse Genie 2021-09-05 06:08:51 EDT

New Gerrit change created: https://git.eclipse.org/r/c/mmt/org.eclipse.qvto/+/184986

Comment 22 Eclipse Genie 2021-09-05 08:09:52 EDT

New Gerrit change created: https://git.eclipse.org/r/c/mmt/org.eclipse.qvto/+/184993

Comment 23 Eclipse Genie 2021-09-05 08:09:53 EDT

New Gerrit change created: https://git.eclipse.org/r/c/mmt/org.eclipse.qvto/+/184992

Comment 24 Ed Willink 2021-09-09 06:31:57 EDT

Installing cgerking/master that aggregates this fix seems good. I see a couple of momentary "QVTo Analyzing" in the progress view, but almost too quick to see. Excellent.

The main edit is a bit involved for me to review so I can only offer the trivial review on JavaModuleLoader.isModuleClassValid where it appears that you have empirically discovered the need for a LinkageError catch. Surely we really just want to know whether it's a good class or not, so rather than having a crash for yet-to-be-discovered failures, just return false for all Error and probably all Throwable.

try {
	Constructor<?> constructor = javaClass.getDeclaredConstructor(noParams);
	if(Modifier.isPublic(constructor.getModifiers())) {
		return true;
	}
} catch (Throwable e) {} // bad class 
return false;

Comment 25 Eclipse Genie 2021-09-13 03:57:44 EDT

New Gerrit change created: https://git.eclipse.org/r/c/mmt/org.eclipse.qvto/+/185358

Comment 26 Christopher Gerking 2021-09-13 06:00:30 EDT

(In reply to Ed Willink from comment #24)
> Surely we really just want to know whether it's a good class or not, so 
> rather than having a crash for yet-to-be-discovered failures, just return 
> false for all Error and probably all Throwable.
Agreed. Pushed to master.

Comment 27 Eclipse Genie 2021-09-20 03:25:31 EDT

New Gerrit change created: https://git.eclipse.org/r/c/mmt/org.eclipse.qvto/+/185585

Comment 28 Christopher Gerking 2021-09-20 03:34:03 EDT

Setting breakpoints reveals that there are still random LinkageErrors, but this time caused by an attempt to duplicate a class definition. Obviously this is due to a lack of synchronization. We should therefore use getClassLoadingLock(...) provided by ClassLoader.

Pushed to cgerking/574706. Commit ID: bab00916878371abe526140a337b7f903b75ac42

Comment 29 Christopher Gerking 2021-10-22 07:56:42 EDT

(In reply to Christopher Gerking from comment #28)
> We should therefore use getClassLoadingLock(...) provided by ClassLoader.
Can we push the synchronization to master? IMO that's a no-brainer.

Comment 30 Ed Willink 2021-10-22 10:35:37 EDT

(In reply to Christopher Gerking from comment #29)
> (In reply to Christopher Gerking from comment #28)
> > We should therefore use getClassLoadingLock(...) provided by ClassLoader.
> Can we push the synchronization to master? IMO that's a no-brainer.

Yes, but ...

Searching QVTo for "synchronized" there are just 11 matches.

7 in Blackbox support
2 in QvtRuntimeException
1 in QvtOperationalEvaluationVisitorImpl
1 in QvtEnvironmentBase

Hardly evidence for a solid concurrency design. Hence the Bug 525852 observation 

> Clearly there is an initialization hazard between the main/UI thread and the
> QvtReconciler.

EMF is not threadsafe and so OCL is not threadsafe and so QVTo is not threadsafe.

I would expect QVTo concurrency to be limited to exclusive main-active/worker-active with no actual QVTo concurrency. Any use of "synchronized" would appear to be a well-meaning preliminary contribution to a QVTo future in which concurrency is supported. Until then the "synchronized" gives a small performance degradation and confuses readers into thinking that concurrecy is supported. Ill-considered "synchronized" may introduce deadlock hazards.

Today, if "synchronized" fixes a bug, then the true concurrency bug should be fixed rather than just moving it around with a bit of sticking plaster.

So I'm inclined to reject the no-brainer.

----

The stack dump only shows a QVTo Builder with a class loading issue. Not sure it's actually what this bug addresses, but analyzing...

What if two builders run at once, due to impatient user / platform bug. Presumably the one Eclipse class loader can be loading on two threads.

What if a QVTo Editor provokes a compilation while a builder is active? Perhaps this is where the main/QvtReconciler issue comes from.

If there is a concurrency problem with Eclipse plugin classloaders, I would expect Eclipse to be crashing regularly. So perhaps it's only a custom class loader issue.

Anyway. to manage concurrency, we should probably have a singleton worker that linearizes build and reconcile rather than sticking plasters over empirically discovered failures.

Comment 31 Christopher Gerking 2021-10-25 05:46:13 EDT

(In reply to Ed Willink from comment #30)
> What if a QVTo Editor provokes a compilation while a builder is active?
> Perhaps this is where the main/QvtReconciler issue comes from.
I agree, this is the only cause I can think of. In both cases a QVTOCompiler is active. Different compilers use a shared ProjectClassLoader to prevent the co-existence of multiple identical classes. To be precise, there is one ProjectClassLoader per Java project. 

Using separate ProjectClassLoaders per compiler would be one way to address the concurrency bug. But the disadvantage of this approach is worse performance (see bug 571406) because separate loaders cannot benefit from a shared cache.

On the contrary, ProjectClassLoader is an indirect extension of ClassLoader with its built-in synchronization via getClassLoadingLock(). From the perspective of the Liskov substitution principle, ProjectClassLoader should also be synchronized if ClassLoader is synchronized.

Comment 32 Ed Willink 2021-10-25 09:13:28 EDT

(In reply to Christopher Gerking from comment #31)
> On the contrary, ProjectClassLoader is an indirect extension of ClassLoader
> with its built-in synchronization via getClassLoadingLock(). From the
> perspective of the Liskov substitution principle, ProjectClassLoader should
> also be synchronized if ClassLoader is synchronized.

Yes. A QVTo ClassLoader should certainly be semantically consistent with ClassLoader. I had never before considered synchronized in the context of ClassLoader. It just worked - maybe it was native code magic. Looking at the code now, I see that many methods are carefully coded.

So if a QVTo ClassLoader adds state outside the synchronized barrier, a new synchronized barrier is probably needed. But a slight adjustment to the API might allow the extra state to be maintained inside the standard barrier.

> Using separate ProjectClassLoaders per compiler would be one way to address
> the concurrency bug.

In the short term, if the reported crash is easily reproducible, it should provide a convenient vehicle, possibly augmented by some printf's, to discover how the 'singleton' QVTo engine goes concurrent.

From my understnading of safe QVTo execution, the QVTo ClassLoader should never be invoked in an unsafe way, so improved coding is redundant.

But, once the concurrency hazard is fixed, we should ensure that the QVTo ClassLoader has consistent semantics.

> But the disadvantage of this approach is worse
> performance (see bug 571406) because separate loaders cannot benefit from a
> shared cache.

I'm not convinced that this is true. The old performance problems were because huge numbers of classes were redundantly analyzed, now your fixes trim to a few relevant classes. We no longer need to optimize multiple stupidity.

A shared cache has a minor problem in that it can be difficult to avoid collection of garbage, and a major problem in that stale class loads may persist.

In so far as QVTo is not part of a huge concurrency, and only a few classes are loaded, I see the hazards of sharing as much more serious than the benefits.

Comment 33 Eclipse Genie 2021-11-22 03:10:50 EST

New Gerrit change created: https://git.eclipse.org/r/c/mmt/org.eclipse.qvto/+/187966

Comment 34 Eclipse Genie 2021-11-22 09:42:31 EST

Gerrit change https://git.eclipse.org/r/c/mmt/org.eclipse.qvto/+/187966 was merged to [master].
Commit: http://git.eclipse.org/c/mmt/org.eclipse.qvto.git/commit/?id=e88a2f9e1f6ddf810403e566265d6a14646b5693