search

eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
160 stars 129 forks source link

Add optimistic locking to OpenableElementInfo add/delete/set operations #2753

Closed iloveeclipse closed 2 months ago

iloveeclipse commented 2 months ago

I have an impression that JavaModel.getJavaProjects() that uses OpenableElementInfo to maintain children may sometimes return stale data because OpenableElementInfo is not synchronized and probably could be accessed from different threads (the volatile children field indicates that the original authors assumed multi-threaded access to that data).

This could explain why we sometimes see absolutely fully unexpected test failures related to the Java projects state.

See https://github.com/eclipse-jdt/eclipse.jdt.core/issues/2716

iloveeclipse commented 2 months ago

Really interesting test fails to analyze, the added extra check shows unexpected results: Test Result (21 failures ) org.eclipse.jdt.core.tests.model.JavaProjectTests.testProjectImport org.eclipse.jdt.core.tests.model.JavaProjectTests.testBug148859 org.eclipse.jdt.core.tests.model.ModuleBuilderTests.testBug520147 org.eclipse.jdt.core.tests.model.SearchTests.testChangeClasspath org.eclipse.jdt.core.tests.model.SearchTests.testConcurrentJob org.eclipse.jdt.core.tests.model.SearchTests.testRemoveOuterFolder org.eclipse.jdt.core.tests.model.JavaSearchScopeTests.testBug250211 org.eclipse.jdt.core.tests.model.JavaIndexTests.testUseIndexInternalJarAfterRestart org.eclipse.jdt.core.tests.model.ClassNameTests.testBug152841 org.eclipse.jdt.core.tests.model.ClasspathTests.testClasspathCreateLocalJarLibraryEntry org.eclipse.jdt.core.tests.model.ClasspathTests.testPerfDenseCycleDetection1 org.eclipse.jdt.core.tests.model.ClasspathTests.testPerfDenseCycleDetection2 org.eclipse.jdt.core.tests.model.ClasspathTests.testPerfDenseCycleDetection3 org.eclipse.jdt.core.tests.model.ClasspathTests.testNoCycleDetection1 org.eclipse.jdt.core.tests.model.ClasspathTests.testNoCycleDetection2 org.eclipse.jdt.core.tests.model.JavaElementDeltaTests.testAddTwoJavaProjects org.eclipse.jdt.core.tests.model.JavaElementDeltaTests.testAddTwoJavaProjectsWithExtraSetClasspath org.eclipse.jdt.core.tests.model.JavaElementDeltaTests.testRemoveAddBinaryProject org.eclipse.jdt.core.tests.model.JavaElementDeltaTests.testRemoveAddJavaProject org.eclipse.jdt.core.tests.model.ExclusionPatternsTests.testSearchPotentialMatchInOutput org.eclipse.jdt.core.tests.model.InclusionPatternsTests.testSearchPotentialMatchInOutput

iloveeclipse commented 2 months ago

Really interesting test fails to analyze, the added extra check shows unexpected results: org.eclipse.jdt.core.tests.model.JavaProjectTests.testProjectImport

This one happens because of nested workspace operations that create projects. It looks like that only the last operation triggers delta notifications, and so no project is "found" by JavaModel because even if it is created and exists in the workspace, JavaModel didn't get the change event yet and so didn't "officially" knows it.

Looking into other tests...

iloveeclipse commented 2 months ago

Most recent test fail is interesting, shows inconsistency in yet another place:

java.lang.NullPointerException: Cannot invoke "java.util.Map.get(Object)" because "perProjectInfo.rootPathToResolvedEntries" is null
    at org.eclipse.jdt.internal.core.JavaModelManager.getReferencedClasspathEntries(JavaModelManager.java:2232)
    at org.eclipse.jdt.core.JavaCore.getReferencedClasspathEntries(JavaCore.java:5960)
    at org.eclipse.jdt.core.tests.model.ClasspathTests.testBug252341b(ClasspathTests.java:6455)
    at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)
    at junit.framework.TestCase.runTest(TestCase.java:177)
    at org.eclipse.jdt.core.tests.junit.extension.TestCase.runTest(TestCase.java:972)
    at junit.framework.TestCase.runBare(TestCase.java:142)
    at junit.framework.TestResult$1.protect(TestResult.java:122)
    at junit.framework.TestResult.runProtected(TestResult.java:142)
    at junit.framework.TestResult.run(TestResult.java:125)
    at junit.framework.TestCase.run(TestCase.java:130)
    at junit.framework.TestSuite.runTest(TestSuite.java:241)
    at org.eclipse.jdt.core.tests.model
merks commented 2 months ago

@cdietrich

This breaks XBase and hence large technology stack:

https://ci.eclipse.org/xpect/job/Xpect/job/master/1078//console:

(org.eclipse.xtext.xbase.ui.validation.XbaseUIValidator is in unnamed module of loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @248397bb; org.eclipse.jdt.internal.core.JavaModelManager$PerProjectInfo is in unnamed module of loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @2c9fef73)
    at org.eclipse.xtext.xbase.ui.validation.XbaseUIValidator.getResolvedClasspathEntry(XbaseUIValidator.java:226)
    at org.eclipse.xtext.xbase.ui.validation.XbaseUIValidator.computeRestriction(XbaseUIValidator.java:196)
    at org.eclipse.xtext.xbase.ui.validation.XbaseUIValidator.checkRestrictedType(XbaseUIValidator.java:158)
    at org.eclipse.xtext.xbase.ui.validation.XbaseUIValidator.checkRestrictedType(XbaseUIValidator.java:118)

https://github.com/eclipse/xtext/blob/87b6681c980013cf1bc4a6cfc705f330de3b1d07/org.eclipse.xtext.xbase.ui/src/org/eclipse/xtext/xbase/ui/validation/XbaseUIValidator.java#L226