Skip to content

Commit

Permalink
Add optimistic locking to OpenableElementInfo add/delete/set operations
Browse files Browse the repository at this point in the history
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 eclipse-jdt#2716
  • Loading branch information
iloveeclipse committed Jul 24, 2024
1 parent b7ffba3 commit 42d4169
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2418,6 +2418,10 @@ public void run(IProgressMonitor monitor) throws CoreException {
assertTrue("Not Java project: " + project, project.hasNature(JavaCore.NATURE_ID));
IProject sameProject = getWorkspaceRoot().getProject(projectName);
assertEquals("Returned project doesn't match same project from workspace: " + sameProject + " vs " + project, sameProject, project);

List<IJavaProject> javaProjects = List.of(getJavaModel().getJavaProjects());
boolean foundInModel = javaProjects.stream().anyMatch(p -> projectName.equals(p.getElementName()));
assertTrue("Project '" + projectName + "' should be present in JavaModel, but we found only: " + javaProjects, foundInModel);
return javaProject;
}
protected IJavaProject importJavaProject(String projectName, String[] sourceFolders, String[] libraries, String output) throws CoreException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,20 +42,43 @@ public class OpenableElementInfo extends JavaElementInfo {
*/
protected Object[] nonJavaResources;

public void addChild(IJavaElement child) {
public void addChild(final IJavaElement child) {
IJavaElement[] oldChildren = this.children;
int length = oldChildren.length;
if (length == 0) {
this.children = new IJavaElement[] {child};
synchronized (this) {
if (oldChildren == this.children) {
this.children = new IJavaElement[] {child};
} else {
// try again, holding a lock
addChild(child);
}
}
} else {
for (int i = 0; i < length; i++) {
if (oldChildren[i].equals(child))
return; // already included
if (oldChildren[i].equals(child)) {
synchronized (this) {
if (oldChildren == this.children){
return; // already included
} else {
// try again, holding a lock
addChild(child);
return;
}
}
}
}
IJavaElement[] newChildren = new IJavaElement[length+1];
System.arraycopy(oldChildren, 0, newChildren, 0, length);
newChildren[length] = child;
this.children = newChildren;
synchronized (this) {
if (oldChildren == this.children) {
this.children = newChildren;
} else {
// try again, holding a lock
addChild(child);
}
}
}
}

Expand All @@ -71,26 +94,44 @@ public boolean isStructureKnown() {
return this.isStructureKnown;
}

public void removeChild(IJavaElement child) {
public void removeChild(final IJavaElement child) {
IJavaElement[] oldChildren = this.children;
for (int i = 0, length = oldChildren.length; i < length; i++) {
if (oldChildren[i].equals(child)) {
if (length == 1) {
this.children = JavaElement.NO_ELEMENTS;
synchronized (this) {
if (oldChildren == this.children) {
this.children = JavaElement.NO_ELEMENTS;
} else {
// try again, holding a lock
removeChild(child);
return;
}
}
} else {
IJavaElement[] newChildren = new IJavaElement[length-1];
System.arraycopy(oldChildren, 0, newChildren , 0, i);
if (i < length-1)
System.arraycopy(oldChildren, i+1, newChildren, i, length-1-i);
this.children = newChildren;
synchronized (this) {
if (oldChildren == this.children) {
this.children = newChildren;
} else {
// try again, holding a lock
removeChild(child);
return;
}
}
}
break;
}
}
}

public void setChildren(IJavaElement[] children) {
this.children= (children.length > 0) ? children : JavaElement.NO_ELEMENTS;
synchronized (this) {
this.children= (children.length > 0) ? children : JavaElement.NO_ELEMENTS;
}
}

public void setModule(IModuleDescription module) {
Expand Down

0 comments on commit 42d4169

Please sign in to comment.