From 45f50f72c7d45319f43d4460980293b29321e2f3 Mon Sep 17 00:00:00 2001 From: Andrey Loskutov Date: Mon, 5 Aug 2024 19:53:53 +0200 Subject: [PATCH] Make JavaModelManager.PerProjectInfo a bit more thread safe PerProjectInfo with almost all fields being public is a big mess. Every thread can access every non final field at any time without any consistency checks etc. Let make all non final fields volatile so that they read at least the latest data, even if only partially consistent. The object inconsistency may appear because most of the fields are set in PerProjectInfo.setClasspath() in a non-atomic way, so readers who don't lock on PerProjectInfo may observe partially updated PerProjectInfo object. Ideally all the fields should be made private and accessed from dedicated synchronized methods on PerProjectInfo. See https://github.com/eclipse-jdt/eclipse.jdt.core/pull/2753#issuecomment-2269557239 --- .../jdt/internal/core/JavaModelManager.java | 72 ++++++++++--------- .../jdt/internal/core/JavaProject.java | 11 ++- .../internal/core/PackageFragmentRoot.java | 6 +- 3 files changed, 47 insertions(+), 42 deletions(-) diff --git a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaModelManager.java b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaModelManager.java index 411f69d9fba..de2507fa698 100644 --- a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaModelManager.java +++ b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaModelManager.java @@ -1357,34 +1357,31 @@ public static class PerProjectInfo { static final IJavaModelStatus NEED_RESOLUTION = new JavaModelStatus(); - public IProject project; - public Object savedState; - public boolean triedRead; - public IClasspathEntry[] rawClasspath; - public IClasspathEntry[] referencedEntries; - public IJavaModelStatus rawClasspathStatus; - public int rawTimeStamp = 0; - public boolean writtingRawClasspath = false; - public IClasspathEntry[] resolvedClasspath; - public IJavaModelStatus unresolvedEntryStatus; - public Map rootPathToRawEntries; // reverse map from a package fragment root's path to the raw entry - public Map rootPathToResolvedEntries; // map from a package fragment root's path to the resolved entry - public IPath outputLocation; - public Map jrtRoots; // A map between a JRT file system (as a string) and the package fragment roots found in it. - - public IEclipsePreferences preferences; - public Hashtable options; + public final IProject project; + public volatile Object savedState; + public volatile boolean triedRead; + public volatile IClasspathEntry[] rawClasspath; + public volatile IClasspathEntry[] referencedEntries; + public volatile IJavaModelStatus rawClasspathStatus; + public volatile int rawTimeStamp; + public volatile boolean writtingRawClasspath; + public volatile IClasspathEntry[] resolvedClasspath; + public volatile IJavaModelStatus unresolvedEntryStatus; + public volatile Map rootPathToRawEntries; // reverse map from a package fragment root's path to the raw entry + private Map rootPathToResolvedEntries; // map from a package fragment root's path to the resolved entry + public volatile IPath outputLocation; + public volatile Map jrtRoots; // A map between a JRT file system (as a string) and the package fragment roots found in it. + + public volatile IEclipsePreferences preferences; + public volatile Hashtable options; private final SecondaryTypes secondaryTypes; // NB: PackageFragment#getAttachedJavadoc uses this map differently // and stores String data, not JavadocContents as values - public LRUCache javadocCache; + public volatile LRUCache javadocCache; public PerProjectInfo(IProject project) { - - this.triedRead = false; - this.savedState = null; this.project = project; this.javadocCache = new LRUCache<>(JAVADOC_CACHE_INITIAL_SIZE); this.secondaryTypes = new SecondaryTypes(); @@ -1396,6 +1393,14 @@ public synchronized IClasspathEntry[] getResolvedClasspath() { return this.resolvedClasspath; } + public synchronized Map getRootPathToResolvedEntries() { + Map entries = this.rootPathToResolvedEntries; + if (entries == null) { + return Map.of(); + } + return entries; + } + public void forgetExternalTimestampsAndIndexes() { IClasspathEntry[] classpath = this.resolvedClasspath; if (classpath == null) return; @@ -1453,16 +1458,18 @@ private ClasspathChange setClasspath(IClasspathEntry[] newRawClasspath, IClasspa } ClasspathChange classpathChange = addClasspathChange ? addClasspathChange() : null; - if (referencedEntries != null) this.referencedEntries = referencedEntries; - if (this.referencedEntries == null) this.referencedEntries = ClasspathEntry.NO_ENTRIES; - this.rawClasspath = newRawClasspath; - this.outputLocation = newOutputLocation; - this.rawClasspathStatus = newRawClasspathStatus; - this.resolvedClasspath = newResolvedClasspath; - this.rootPathToRawEntries = newRootPathToRawEntries; - this.rootPathToResolvedEntries = newRootPathToResolvedEntries; - this.unresolvedEntryStatus = newUnresolvedEntryStatus; - this.javadocCache = new LRUCache<>(JAVADOC_CACHE_INITIAL_SIZE); + synchronized (this) { + if (referencedEntries != null) this.referencedEntries = referencedEntries; + if (this.referencedEntries == null) this.referencedEntries = ClasspathEntry.NO_ENTRIES; + this.rawClasspath = newRawClasspath; + this.outputLocation = newOutputLocation; + this.rawClasspathStatus = newRawClasspathStatus; + this.resolvedClasspath = newResolvedClasspath; + this.rootPathToRawEntries = newRootPathToRawEntries; + this.rootPathToResolvedEntries = newRootPathToResolvedEntries; + this.unresolvedEntryStatus = newUnresolvedEntryStatus; + this.javadocCache = new LRUCache<>(JAVADOC_CACHE_INITIAL_SIZE); + } return classpathChange; } @@ -2229,7 +2236,8 @@ public IClasspathEntry[] getReferencedClasspathEntries(IClasspathEntry libraryEn continue; IClasspathEntry persistedEntry = null; - if ((persistedEntry = perProjectInfo.rootPathToResolvedEntries.get(referencedEntries[index].getPath())) != null) { + Map rootPathToResolvedEntries = perProjectInfo.getRootPathToResolvedEntries(); + if ((persistedEntry = rootPathToResolvedEntries.get(referencedEntries[index].getPath())) != null) { // TODO: reconsider this - may want to copy the values instead of reference assignment? referencedEntries[index] = persistedEntry; } diff --git a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaProject.java b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaProject.java index 3ac1de85994..834f5ca7e65 100644 --- a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaProject.java +++ b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaProject.java @@ -1801,15 +1801,14 @@ public IPackageFragmentRoot[] getAllPackageFragmentRoots(Map rootToResolvedEntri public IClasspathEntry getClasspathEntryFor(IPath path) throws JavaModelException { getResolvedClasspath(); // force resolution PerProjectInfo perProjectInfo = getPerProjectInfo(); - if (perProjectInfo == null) + if (perProjectInfo == null) { return null; - Map rootPathToResolvedEntries = perProjectInfo.rootPathToResolvedEntries; - if (rootPathToResolvedEntries == null) - return null; - IClasspathEntry classpathEntry = (IClasspathEntry) rootPathToResolvedEntries.get(path); + } + Map rootPathToResolvedEntries = perProjectInfo.getRootPathToResolvedEntries(); + IClasspathEntry classpathEntry = rootPathToResolvedEntries.get(path); if (classpathEntry == null) { path = getProject().getWorkspace().getRoot().getLocation().append(path); - classpathEntry = (IClasspathEntry) rootPathToResolvedEntries.get(path); + classpathEntry = rootPathToResolvedEntries.get(path); } return classpathEntry; } diff --git a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/PackageFragmentRoot.java b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/PackageFragmentRoot.java index 3a891d3eaf5..6e3186a8d9e 100644 --- a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/PackageFragmentRoot.java +++ b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/PackageFragmentRoot.java @@ -625,10 +625,8 @@ public IClasspathEntry getResolvedClasspathEntry() throws JavaModelException { IClasspathEntry resolvedEntry = null; JavaProject project = getJavaProject(); project.getResolvedClasspath(); // force the resolved entry cache to be populated - Map rootPathToResolvedEntries = project.getPerProjectInfo().rootPathToResolvedEntries; - if (rootPathToResolvedEntries != null) { - resolvedEntry = (IClasspathEntry) rootPathToResolvedEntries.get(getPath()); - } + Map rootPathToResolvedEntries = project.getPerProjectInfo().getRootPathToResolvedEntries(); + resolvedEntry = rootPathToResolvedEntries.get(getPath()); if (resolvedEntry == null) { throw new JavaModelException(new JavaModelStatus(IJavaModelStatusConstants.ELEMENT_NOT_ON_CLASSPATH, this)); }