Skip to content

Commit

Permalink
Make JavaModelManager.PerProjectInfo a bit more thread safe
Browse files Browse the repository at this point in the history
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 #2753 (comment)
  • Loading branch information
iloveeclipse committed Aug 5, 2024
1 parent 93d46ab commit 45f50f7
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 42 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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<IPath, IClasspathEntry> rootPathToRawEntries; // reverse map from a package fragment root's path to the raw entry
public Map<IPath, IClasspathEntry> rootPathToResolvedEntries; // map from a package fragment root's path to the resolved entry
public IPath outputLocation;
public Map<IPath, ObjectVector> jrtRoots; // A map between a JRT file system (as a string) and the package fragment roots found in it.

public IEclipsePreferences preferences;
public Hashtable<String, String> 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<IPath, IClasspathEntry> rootPathToRawEntries; // reverse map from a package fragment root's path to the raw entry
private Map<IPath, IClasspathEntry> rootPathToResolvedEntries; // map from a package fragment root's path to the resolved entry
public volatile IPath outputLocation;
public volatile Map<IPath, ObjectVector> 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<String, String> options;

private final SecondaryTypes secondaryTypes;

// NB: PackageFragment#getAttachedJavadoc uses this map differently
// and stores String data, not JavadocContents as values
public LRUCache<IJavaElement, Object> javadocCache;
public volatile LRUCache<IJavaElement, Object> 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();
Expand All @@ -1396,6 +1393,14 @@ public synchronized IClasspathEntry[] getResolvedClasspath() {
return this.resolvedClasspath;
}

public synchronized Map<IPath, IClasspathEntry> getRootPathToResolvedEntries() {
Map<IPath, IClasspathEntry> entries = this.rootPathToResolvedEntries;
if (entries == null) {
return Map.of();
}
return entries;
}

public void forgetExternalTimestampsAndIndexes() {
IClasspathEntry[] classpath = this.resolvedClasspath;
if (classpath == null) return;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -2229,7 +2236,8 @@ public IClasspathEntry[] getReferencedClasspathEntries(IClasspathEntry libraryEn
continue;

IClasspathEntry persistedEntry = null;
if ((persistedEntry = perProjectInfo.rootPathToResolvedEntries.get(referencedEntries[index].getPath())) != null) {
Map<IPath, IClasspathEntry> 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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<IPath, IClasspathEntry> 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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<IPath, IClasspathEntry> rootPathToResolvedEntries = project.getPerProjectInfo().getRootPathToResolvedEntries();
resolvedEntry = rootPathToResolvedEntries.get(getPath());
if (resolvedEntry == null) {
throw new JavaModelException(new JavaModelStatus(IJavaModelStatusConstants.ELEMENT_NOT_ON_CLASSPATH, this));
}
Expand Down

0 comments on commit 45f50f7

Please sign in to comment.