Skip to content

Commit

Permalink
Workaround deadlock test issues in Oxygen M5 (#1348)
Browse files Browse the repository at this point in the history 
* Defer WTP dependency graph updates during facet installation to avoid
  deadlocks (https://bugs.eclipse.org/511793)
* Extract App Engine facet/runtime jobs and mark as JST jobs
* Rework waitForIdle() -> waitForProjects() to reduce wait-for-completion times
  • Loading branch information
@briandealwis
briandealwis authored Feb 8, 2017
1 parent e6725d2 commit d337a34
Show file tree
Hide file tree
Showing 18 changed files with 196 additions and 120 deletions.
6 changes: 3 additions & 3 deletions ...st/src/com/google/cloud/tools/eclipse/appengine/facets/StandardFacetInstallationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public void testStandardFacetInstallation() throws IOException, CoreException {
assertTrue(correctAppEngineWebXml.exists());
assertFalse(wrongAppEngineWebXml.exists());

ProjectUtils.waitUntilIdle(); // App Engine runtime is added via a Job, so wait.
ProjectUtils.waitForProjects(project); // App Engine runtime is added via a Job, so wait.
IRuntime primaryRuntime = facetedProject.getPrimaryRuntime();
assertTrue(AppEngineStandardFacet.isAppEngineStandardRuntime(primaryRuntime));
}
Expand All @@ -92,7 +92,7 @@ public void testStandardFacetInstallation_createsWebXml() throws CoreException {

IFacetedProject facetedProject = ProjectFacetsManager.create(project);
AppEngineStandardFacet.installAppEngineFacet(facetedProject, true, null);
ProjectUtils.waitUntilIdle(); // App Engine runtime is added via a Job, so wait.
ProjectUtils.waitForProjects(project); // App Engine runtime is added via a Job, so wait.

assertTrue(project.getFile("src/main/webapp/WEB-INF/web.xml").exists());
}
Expand All @@ -109,7 +109,7 @@ public void testStandardFacetInstallation_DoesNotOverwriteWebXml()

IFacetedProject facetedProject = ProjectFacetsManager.create(project);
AppEngineStandardFacet.installAppEngineFacet(facetedProject, true, null);
ProjectUtils.waitUntilIdle(); // App Engine runtime is added via a Job, so wait.
ProjectUtils.waitForProjects(project); // App Engine runtime is added via a Job, so wait.

assertEmptyFile(webXml);
}
Expand Down
14 changes: 13 additions & 1 deletion ...ne.facets/src/com/google/cloud/tools/eclipse/appengine/facets/AppEngineStandardFacet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.eclipse.jst.j2ee.web.project.facet.WebFacetInstallDataModelProvider;
import org.eclipse.jst.j2ee.web.project.facet.WebFacetUtils;
import org.eclipse.jst.server.core.FacetUtil;
import org.eclipse.wst.common.componentcore.internal.builder.IDependencyGraph;
import org.eclipse.wst.common.frameworks.datamodel.DataModelFactory;
import org.eclipse.wst.common.frameworks.datamodel.IDataModel;
import org.eclipse.wst.common.project.facet.core.IFacetedProject;
Expand Down Expand Up @@ -147,7 +148,18 @@ public static void installAppEngineFacet(IFacetedProject facetedProject,
Object config = null;
facetInstallSet.add(new IFacetedProject.Action(
IFacetedProject.Action.Type.INSTALL, appEngineFacetVersion, config));
facetedProject.modify(facetInstallSet, subMonitor.newChild(100));

// Workaround deadlock bug described in Eclipse bug (https://bugs.eclipse.org/511793).
// There are graph update jobs triggered by the completion of the CreateProjectOperation
// above (from resource notifications) and from other resource changes from modifying the
// project facets. So we force the dependency graph to defer updates.
try {
IDependencyGraph.INSTANCE.preUpdate();

facetedProject.modify(facetInstallSet, subMonitor.newChild(100));
} finally {
IDependencyGraph.INSTANCE.postUpdate();
}
}
}

Expand Down
83 changes: 48 additions & 35 deletions ...m/google/cloud/tools/eclipse/appengine/facets/AppEngineStandardRuntimeChangeListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,14 @@

package com.google.cloud.tools.eclipse.appengine.facets;

import com.google.cloud.tools.eclipse.util.status.StatusUtil;
import org.eclipse.core.resources.IProject;
import org.eclipse.core.runtime.CoreException;
import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.core.runtime.IStatus;
import org.eclipse.core.runtime.MultiStatus;
import org.eclipse.core.runtime.Status;
import org.eclipse.core.runtime.jobs.Job;
import org.eclipse.jst.j2ee.refactor.listeners.J2EEElementChangedListener;
import org.eclipse.wst.common.project.facet.core.IFacetedProject;
import org.eclipse.wst.common.project.facet.core.events.IFacetedProjectEvent;
import org.eclipse.wst.common.project.facet.core.events.IFacetedProjectListener;
Expand All @@ -35,6 +36,51 @@
*/
public class AppEngineStandardRuntimeChangeListener implements IFacetedProjectListener {

/** A Job to install out App Engine facet given that we've added a runtime. */
private static class InstallAppEngineFacetJob extends Job {
private final IFacetedProject facetedProject;
private final IRuntime newRuntime;

private InstallAppEngineFacetJob(IFacetedProject facetedProject,
IRuntime newRuntime) {
super(Messages.getString("appengine.add.facet.to.project",
facetedProject.getProject().getName())); // $NON-NLS$
this.facetedProject = facetedProject;
this.newRuntime = newRuntime;
}

/**
* Mark this job as a component update job. Useful for our tests to ensure project configuration
* is complete.
*/
@Override
public boolean belongsTo(Object family) {
return J2EEElementChangedListener.PROJECT_COMPONENT_UPDATE_JOB_FAMILY.equals(family);
}

@Override
protected IStatus run(IProgressMonitor monitor) {
IStatus installStatus = Status.OK_STATUS;

try {
AppEngineStandardFacet.installAppEngineFacet(facetedProject,
false /* installDependentFacets */, monitor);
return installStatus;
} catch (CoreException ex1) {
// Displays missing constraints that prevented facet installation
installStatus = ex1.getStatus();

// Remove App Engine as primary runtime
try {
facetedProject.removeTargetedRuntime(newRuntime, monitor);
return installStatus;
} catch (CoreException ex2) {
return StatusUtil.merge(installStatus, ex2.getStatus());
}
}
}
}

@Override
public void handleEvent(IFacetedProjectEvent event) {
// PRIMARY_RUNTIME_CHANGED occurs in scenarios such as selecting runtimes on the
Expand Down Expand Up @@ -62,40 +108,7 @@ public void handleEvent(IFacetedProjectEvent event) {

// Add the App Engine facet
IProject project = facetedProject.getProject();
Job addFacetJob = new Job("Add App Engine facet to " + project.getName()) {

@Override
protected IStatus run(IProgressMonitor monitor) {

IStatus installStatus = Status.OK_STATUS;

try {
AppEngineStandardFacet.installAppEngineFacet(facetedProject, false /* installDependentFacets */, monitor);
return installStatus;
} catch (CoreException ex1) {
// Displays missing constraints that prevented facet installation
installStatus = ex1.getStatus();

// Remove App Engine as primary runtime
try {
facetedProject.removeTargetedRuntime(newRuntime, monitor);
return installStatus;
} catch (CoreException ex2) {
MultiStatus multiStatus;
if (installStatus instanceof MultiStatus) {
multiStatus = (MultiStatus) installStatus;
} else {
multiStatus = new MultiStatus(installStatus.getPlugin(), installStatus.getCode(),
installStatus.getMessage(), installStatus.getException());
}
multiStatus.merge(ex2.getStatus());
return multiStatus;
}
}

}

};
Job addFacetJob = new InstallAppEngineFacetJob(facetedProject, newRuntime);
addFacetJob.schedule();
}

Expand Down
18 changes: 14 additions & 4 deletions ...ets/src/com/google/cloud/tools/eclipse/appengine/facets/StandardFacetInstallDelegate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.eclipse.core.runtime.Status;
import org.eclipse.core.runtime.SubMonitor;
import org.eclipse.core.runtime.jobs.Job;
import org.eclipse.jst.j2ee.refactor.listeners.J2EEElementChangedListener;
import org.eclipse.wst.common.project.facet.core.IFacetedProject;
import org.eclipse.wst.common.project.facet.core.IProjectFacet;
import org.eclipse.wst.common.project.facet.core.IProjectFacetVersion;
Expand All @@ -55,8 +56,7 @@ private void installAppEngineRuntimes(IProject project) throws CoreException {

// Modifying targeted runtimes while installing/uninstalling facets is not allowed,
// so schedule a job as a workaround.
Job installJob = new AppEngineRuntimeInstallJob(
"Install App Engine runtimes in " + project.getName(), facetedProject);
Job installJob = new AppEngineRuntimeInstallJob(facetedProject);
// Schedule immediately so that it doesn't go into the SLEEPING state. Ensuring the job is
// active is necessary for unit testing.
installJob.schedule();
Expand All @@ -70,11 +70,21 @@ private static class AppEngineRuntimeInstallJob extends Job {

private IFacetedProject facetedProject;

private AppEngineRuntimeInstallJob(String name, IFacetedProject facetedProject) {
super(name);
private AppEngineRuntimeInstallJob(IFacetedProject facetedProject) {
super(Messages.getString("appengine.install.runtime.to.project", // $NON-NLS$
facetedProject.getProject().getName()));
this.facetedProject = facetedProject;
}

/**
* Mark this job as a component update job. Useful for our tests to ensure project configuration
* is complete.
*/
@Override
public boolean belongsTo(Object family) {
return J2EEElementChangedListener.PROJECT_COMPONENT_UPDATE_JOB_FAMILY.equals(family);
}

private void waitUntilJsdtIsFixedFacet() {
try {
IProjectFacet jsdtFacet = ProjectFacetsManager.getProjectFacet(JSDT_FACET_ID);
Expand Down
61 changes: 38 additions & 23 deletions ...s/src/com/google/cloud/tools/eclipse/appengine/facets/StandardFacetUninstallDelegate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.eclipse.core.runtime.IStatus;
import org.eclipse.core.runtime.Status;
import org.eclipse.core.runtime.jobs.Job;
import org.eclipse.jst.j2ee.refactor.listeners.J2EEElementChangedListener;
import org.eclipse.wst.common.project.facet.core.IDelegate;
import org.eclipse.wst.common.project.facet.core.IFacetedProject;
import org.eclipse.wst.common.project.facet.core.IProjectFacetVersion;
Expand All @@ -31,38 +32,52 @@

public class StandardFacetUninstallDelegate implements IDelegate {

@Override
public void execute(IProject project, IProjectFacetVersion version, Object config,
IProgressMonitor monitor) throws CoreException {
uninstallAppEngineRuntimes(project);
}

/**
* Removes all the App Engine server runtimes from the list of targeted runtimes for
* <code>project</code>.
*/
private void uninstallAppEngineRuntimes(final IProject project) {
// Modifying targeted runtimes while installing/uninstalling facets is not allowed,
// so schedule a job as a workaround.
Job uninstallJob = new Job("Uninstall App Engine runtimes in " + project.getName()) {
private final class UninstallAppEngineRuntimesJob extends Job {
private final IFacetedProject facetedProject;

private UninstallAppEngineRuntimesJob(IFacetedProject facetedProject) {
super(Messages.getString("appengine.remove.runtimes.from.project", // $NON-NLS$
facetedProject.getProject().getName()));
this.facetedProject = facetedProject;
}

@Override
protected IStatus run(IProgressMonitor monitor) {
try {
IFacetedProject facetedProject = ProjectFacetsManager.create(project);
Set<IRuntime> targetedRuntimes = facetedProject.getTargetedRuntimes();
/**
* Mark this job as a component update job. Useful for our tests to ensure project configuration
* is complete.
*/
@Override
public boolean belongsTo(Object family) {
return J2EEElementChangedListener.PROJECT_COMPONENT_UPDATE_JOB_FAMILY.equals(family);
}

for (IRuntime targetedRuntime : targetedRuntimes) {
if (AppEngineStandardFacet.isAppEngineStandardRuntime(targetedRuntime)) {
facetedProject.removeTargetedRuntime(targetedRuntime, monitor);
}
@Override
protected IStatus run(IProgressMonitor monitor) {
try {
Set<IRuntime> targetedRuntimes = facetedProject.getTargetedRuntimes();

for (IRuntime targetedRuntime : targetedRuntimes) {
if (AppEngineStandardFacet.isAppEngineStandardRuntime(targetedRuntime)) {
facetedProject.removeTargetedRuntime(targetedRuntime, monitor);
}
return Status.OK_STATUS;
} catch (CoreException ex) {
return ex.getStatus();
}
return Status.OK_STATUS;
} catch (CoreException ex) {
return ex.getStatus();
}
};
}
}

@Override
public void execute(IProject project, IProjectFacetVersion version, Object config,
IProgressMonitor monitor) throws CoreException {
// Modifying targeted runtimes while installing/uninstalling facets is not allowed,
// so schedule a job as a workaround.
IFacetedProject facetedProject = ProjectFacetsManager.create(project);
Job uninstallJob = new UninstallAppEngineRuntimesJob(facetedProject);
uninstallJob.schedule();
}

Expand Down
4 changes: 3 additions & 1 deletion ....appengine.facets/src/com/google/cloud/tools/eclipse/appengine/facets/messages.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ cloud.sdk.out.of.date=Project setup failed because the Google Cloud SDK is too o
appengine.java.component.missing=Project setup failed because the Cloud SDK App Engine Java \
component is not installed. Fix by running 'gcloud components install app-engine-java' on the \
command-line.

appengine.add.facet.to.project=Add App Engine facet to "{0}"
appengine.install.runtime.to.project=Install App Engine runtimes in "{0}"
appengine.remove.runtimes.from.project=Remove App Engine runtimes from "{0}"
project.conversion.error=Failed to convert project "{0}".
web.facet.incompatible.title=Incompatible Dynamic Web Module Facet version
web.facet.incompatible.message=Cannot convert project "{0}". App Engine Standard project requires \
Expand Down
4 changes: 3 additions & 1 deletion ...ools/eclipse/appengine/newproject/maven/CreateMavenBasedAppEngineStandardProjectTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.google.cloud.tools.eclipse.test.util.project.ProjectUtils;
import java.lang.reflect.InvocationTargetException;
import org.eclipse.core.resources.IProject;
import org.eclipse.core.runtime.CoreException;
import org.eclipse.core.runtime.NullProgressMonitor;
import org.eclipse.m2e.core.project.IProjectConfigurationManager;
Expand Down Expand Up @@ -45,7 +46,8 @@ public void testConstructor()
operation.projectConfigurationManager = manager;

operation.execute(monitor);
ProjectUtils.waitUntilIdle(); // App Engine runtime is added via a Job, so wait.
// App Engine runtime is added via a Job, so wait.
ProjectUtils.waitForProjects(operation.getArchetypeProjects().toArray(new IProject[0]));
}

}
9 changes: 8 additions & 1 deletion ...ud/tools/eclipse/appengine/newproject/maven/CreateMavenBasedAppEngineStandardProject.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ public class CreateMavenBasedAppEngineStandardProject extends WorkspaceModifyOpe
private IPath location;
private Archetype archetype;
private HashSet<String> appEngineLibraryIds = new HashSet<String>();

private List<IProject> archetypeProjects;
private IFile mostImportant;

/**
Expand All @@ -61,6 +63,10 @@ IFile getMostImportant() {
return mostImportant;
}

List<IProject> getArchetypeProjects() {
return archetypeProjects;
}

@Override
protected void execute(IProgressMonitor monitor)
throws CoreException, InvocationTargetException, InterruptedException {
Expand Down Expand Up @@ -97,7 +103,7 @@ protected void execute(IProgressMonitor monitor)
ProjectImportConfiguration importConfiguration = new ProjectImportConfiguration();
String packageName = this.packageName == null || this.packageName.isEmpty()
? groupId : this.packageName;
List<IProject> archetypeProjects = projectConfigurationManager.createArchetypeProjects(location,
archetypeProjects = projectConfigurationManager.createArchetypeProjects(location,
archetype, groupId, artifactId, version, packageName, properties,
importConfiguration, progress.newChild(40));

Expand All @@ -107,6 +113,7 @@ protected void execute(IProgressMonitor monitor)
if (pom.exists()) {
this.mostImportant = pom;
}

IFacetedProject facetedProject = ProjectFacetsManager.create(
project, true, loopMonitor.newChild(1));
AppEngineStandardFacet.installAppEngineFacet(facetedProject,
Expand Down
Loading

0 comments on commit d337a34

Please sign in to comment.