Suspend only ConvertJobs instead of all non-system jobs (#2112)

* Suspend only ConvertJobs
* Identify ConvertJob in a more robust way

Author: chanseokoh

plugins/com.google.cloud.tools.eclipse.appengine.facets.test/META-INF/MANIFEST.MF

@@ -12,6 +12,7 @@ Require-Bundle: org.eclipse.wst.common.project.facet.core,
 Import-Package: com.google.cloud.tools.eclipse.test.util,
  com.google.cloud.tools.eclipse.test.util.project,
  org.eclipse.wst.common.project.facet.core,
+ org.eclipse.wst.jsdt.web.core.javascript,
  org.junit;version="4.12.0",
  org.junit.runner;version="4.12.0",
  org.mockito;provider=google;version="1.10.19",

…ne/facets/NonSystemJobSuspenderTest.java …gine/facets/ConvertJobSuspenderTest.javaplugins/com.google.cloud.tools.eclipse.appengine.facets.test/src/com/google/cloud/tools/eclipse/appengine/facets/NonSystemJobSuspenderTest.java renamed to plugins/com.google.cloud.tools.eclipse.appengine.facets.test/src/com/google/cloud/tools/eclipse/appengine/facets/ConvertJobSuspenderTest.java plugins/com.google.cloud.tools.eclipse.appengine.facets.test/src/com/google/cloud/tools/eclipse/appengine/facets/NonSystemJobSuspenderTest.java renamed to plugins/com.google.cloud.tools.eclipse.appengine.facets.test/src/com/google/cloud/tools/eclipse/appengine/facets/ConvertJobSuspenderTest.java

@@ -17,41 +17,72 @@
 package com.google.cloud.tools.eclipse.appengine.facets;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
+import com.google.common.base.Predicates;
 import org.eclipse.core.runtime.IProgressMonitor;
 import org.eclipse.core.runtime.IStatus;
 import org.eclipse.core.runtime.Status;
 import org.eclipse.core.runtime.jobs.Job;
+import org.eclipse.wst.jsdt.web.core.javascript.JsNameManglerUtil;
 import org.junit.After;
+import org.junit.Before;
 import org.junit.Test;
+import org.osgi.framework.Bundle;
+import org.osgi.framework.FrameworkUtil;
 
-public class NonSystemJobSuspenderTest {
+public class ConvertJobSuspenderTest {
 
   private Job job1 = new NoOpSpinJob("Test job 1");
   private Job job2 = new NoOpSpinJob("Test job 2");
 
+  @Before
+  public void setUp() {
+    // Assume all test jobs are "ConvertJob"s.
+    ConvertJobSuspender.isConvertJob = Predicates.alwaysTrue();
+  }
+
   @After
   public void tearDown() {
-    NonSystemJobSuspender.resumeInternal();
+    ConvertJobSuspender.resumeInternal();
     job1.cancel();
     job2.cancel();
   }
 
-  @Test(expected = IllegalStateException.class)
+  @Test
+  public void testConvertJobClassName() {
+    String convertJobClass =
+        ConvertJobSuspender.CONVERT_JOB_CLASS_NAME.replace(".", "/") + ".class";
+    Bundle jsdtWebCoreBundle = FrameworkUtil.getBundle(JsNameManglerUtil.class);
+    assertNotNull(jsdtWebCoreBundle.getResource(convertJobClass));
+  }
+
+  @Test
   public void testCannotSuspendConcurrently() {
-    NonSystemJobSuspender.suspendFutureJobs();
-    NonSystemJobSuspender.suspendFutureJobs();
+    ConvertJobSuspender.suspendFutureConvertJobs();
+    try {
+      ConvertJobSuspender.suspendFutureConvertJobs();
+      fail();
+    } catch (IllegalStateException ex) {
+      assertEquals("Already suspended.", ex.getMessage());
+    }
   }
 
-  @Test(expected = IllegalStateException.class)
+  @Test
   public void testCannotResumeIfNotSuspended() {
-    NonSystemJobSuspender.resume();
+    try {
+      ConvertJobSuspender.resume();
+      fail();
+    } catch (IllegalStateException ex) {
+      assertEquals("Not suspended.", ex.getMessage());
+    }
   }
 
   @Test
-  public void testSuspendFutureJobs() {
-    NonSystemJobSuspender.suspendFutureJobs();
+  public void testSuspendFutureConvertJobs() {
+    ConvertJobSuspender.suspendFutureConvertJobs();
     job1.schedule();
     job2.schedule(10000 /* ms */);
     assertEquals(Job.NONE, job1.getState());
@@ -62,16 +93,16 @@ public void testSuspendFutureJobs() {
   public void testScheduledJobsAreNotSuspended() {
     job1.schedule();
     job2.schedule(10000 /* ms */);
-    NonSystemJobSuspender.suspendFutureJobs();
+    ConvertJobSuspender.suspendFutureConvertJobs();
     assertTrue(Job.WAITING == job1.getState() || Job.RUNNING == job1.getState());
     assertEquals(Job.SLEEPING, job2.getState());
   }
 
   @Test
-  public void testSystemJobsAreNotSuspended() {
-    NonSystemJobSuspender.suspendFutureJobs();
-    job1.setSystem(true);
-    job2.setSystem(true);
+  public void testNonConvertJobsAreNotSuspended() {
+    ConvertJobSuspender.suspendFutureConvertJobs();
+    // Assume all test jobs are not "ConvertJob"s.
+    ConvertJobSuspender.isConvertJob = Predicates.alwaysFalse();
     job1.schedule();
     job2.schedule(10000 /* ms */);
     assertTrue(Job.WAITING == job1.getState() || Job.RUNNING == job1.getState());
@@ -80,13 +111,13 @@ public void testSystemJobsAreNotSuspended() {
 
   @Test
   public void testResume() {
-    NonSystemJobSuspender.suspendFutureJobs();
+    ConvertJobSuspender.suspendFutureConvertJobs();
     job1.schedule();
     job2.schedule(10000 /* ms */);
     assertEquals(Job.NONE, job1.getState());
     assertEquals(Job.NONE, job2.getState());
 
-    NonSystemJobSuspender.resume();
+    ConvertJobSuspender.resume();
     assertTrue(Job.WAITING == job1.getState() || Job.RUNNING == job1.getState());
     assertEquals(Job.SLEEPING, job2.getState());
   }

…facets/NonSystemJobScheduleListener.java …e/facets/ConvertJobScheduleListener.javaplugins/com.google.cloud.tools.eclipse.appengine.facets/src/com/google/cloud/tools/eclipse/appengine/facets/NonSystemJobScheduleListener.java renamed to plugins/com.google.cloud.tools.eclipse.appengine.facets/src/com/google/cloud/tools/eclipse/appengine/facets/ConvertJobScheduleListener.java plugins/com.google.cloud.tools.eclipse.appengine.facets/src/com/google/cloud/tools/eclipse/appengine/facets/NonSystemJobScheduleListener.java renamed to plugins/com.google.cloud.tools.eclipse.appengine.facets/src/com/google/cloud/tools/eclipse/appengine/facets/ConvertJobScheduleListener.java

@@ -17,30 +17,16 @@
 package com.google.cloud.tools.eclipse.appengine.facets;
 
 import org.eclipse.core.runtime.jobs.IJobChangeEvent;
-import org.eclipse.core.runtime.jobs.IJobChangeListener;
+import org.eclipse.core.runtime.jobs.JobChangeAdapter;
 
 /**
- * For private use in {@link NonSystemJobSuspender} only. Listens for every job being scheduled
- * and cancels it if the suspender is active.
+ * For private use in {@link ConvertJobSuspender} only. Listens for every job being scheduled
+ * and cancels it if it is {@link org.eclipse.wst.jsdt.web.core.internal.project.ConvertJob} while
+ * the suspender is active.
  */
-class NonSystemJobScheduleListener implements IJobChangeListener {
+class ConvertJobScheduleListener extends JobChangeAdapter {
   @Override
   public void scheduled(IJobChangeEvent event) {
-    NonSystemJobSuspender.suspendJob(event.getJob(), event.getDelay());
+    ConvertJobSuspender.suspendJob(event.getJob(), event.getDelay());
   }
-
-  @Override
-  public void aboutToRun(IJobChangeEvent event) {}
-
-  @Override
-  public void awake(IJobChangeEvent event) {}
-
-  @Override
-  public void done(IJobChangeEvent event) {}
-
-  @Override
-  public void running(IJobChangeEvent event) {}
-
-  @Override
-  public void sleeping(IJobChangeEvent event) {}
 }

…engine/facets/NonSystemJobSuspender.java …ppengine/facets/ConvertJobSuspender.javaplugins/com.google.cloud.tools.eclipse.appengine.facets/src/com/google/cloud/tools/eclipse/appengine/facets/NonSystemJobSuspender.java renamed to plugins/com.google.cloud.tools.eclipse.appengine.facets/src/com/google/cloud/tools/eclipse/appengine/facets/ConvertJobSuspender.java plugins/com.google.cloud.tools.eclipse.appengine.facets/src/com/google/cloud/tools/eclipse/appengine/facets/NonSystemJobSuspender.java renamed to plugins/com.google.cloud.tools.eclipse.appengine.facets/src/com/google/cloud/tools/eclipse/appengine/facets/ConvertJobSuspender.java

@@ -18,22 +18,34 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
+import com.google.common.base.Predicate;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import org.eclipse.core.runtime.jobs.Job;
 
 /**
- * Prevents scheduling all future non-system jobs once {@link #suspendFutureJobs} is called, until
- * {@link #resume} is called. Jobs already scheduled are not affected and will run to completion.
+ * Prevents scheduling all future {@link org.eclipse.wst.jsdt.web.core.internal.project.ConvertJob}s
+ * once {@link #suspendConvertJobs} is called, until {@link #resume} is called. Jobs already
+ * scheduled are not affected and will run to completion.
  *
  * The class is for https://github.com/GoogleCloudPlatform/google-cloud-eclipse/issues/1155. The
  * purpose of the class is to prevent the second ConvertJob from running. The second ConvertJob is
  * triggered by the first ConvertJob when the latter job installs the JSDT facet.
  *
  * Not recommended to use for other situations, although the workings of the class are general.
  */
-class NonSystemJobSuspender {
+// TODO(chanseok): not needed after we drop support for Mars and Neon, as Oxygen has fixed the
+// issue. Remove this and related code when that happens.
+class ConvertJobSuspender {
+
+  private static final Logger logger = Logger.getLogger(ConvertJobSuspender.class.getName());
+
+  @VisibleForTesting
+  static final String CONVERT_JOB_CLASS_NAME =
+      "org.eclipse.wst.jsdt.web.core.internal.project.ConvertJob";
 
   private static class SuspendedJob {
     private Job job;
@@ -45,15 +57,23 @@ private SuspendedJob(Job job, long scheduleDelay) {
     }
   }
 
+  @VisibleForTesting
+  static Predicate<Job> isConvertJob = new Predicate<Job>() {
+    @Override
+    public boolean apply(Job job) {
+      return CONVERT_JOB_CLASS_NAME.equals(job.getClass().getName());
+    }
+  };
+
   private static final AtomicBoolean suspended = new AtomicBoolean(false);
 
   private static final List<SuspendedJob> suspendedJobs = new ArrayList<>();
 
-  private static final NonSystemJobScheduleListener jobScheduleListener =
-      new NonSystemJobScheduleListener();
+  private static final ConvertJobScheduleListener jobScheduleListener =
+      new ConvertJobScheduleListener();
 
   /** Once called, it is imperative to call {@link #resume()} later. */
-  public static void suspendFutureJobs() {
+  public static void suspendFutureConvertJobs() {
     synchronized (suspendedJobs) {
       Preconditions.checkState(!suspended.getAndSet(true), "Already suspended.");
       Job.getJobManager().addJobChangeListener(jobScheduleListener);
@@ -83,7 +103,7 @@ static void resumeInternal() {
   }
 
   static void suspendJob(Job job, long scheduleDelay) {
-    if (job.isSystem()) {
+    if (!isConvertJob.apply(job)) {
       return;
     }
     synchronized (suspendedJobs) {
@@ -104,8 +124,10 @@ static void suspendJob(Job job, long scheduleDelay) {
           job.schedule(scheduleDelay);
         }
       }
+    } else {
+      logger.log(Level.SEVERE, "job already running: " + job);
     }
   }
 
-  private NonSystemJobSuspender() {}
+  private ConvertJobSuspender() {}
 }

plugins/com.google.cloud.tools.eclipse.appengine.facets/src/com/google/cloud/tools/eclipse/appengine/facets/StandardFacetInstallDelegate.java

@@ -64,7 +64,7 @@ private void installAppEngineRuntimes(IProject project) throws CoreException {
     // https://github.com/GoogleCloudPlatform/google-cloud-eclipse/issues/1155
     // The first ConvertJob has already been scheduled (which installs JSDT facet), and
     // this is to suspend the second ConvertJob temporarily.
-    NonSystemJobSuspender.suspendFutureJobs();
+    ConvertJobSuspender.suspendFutureConvertJobs();
   }
 
   private static class AppEngineRuntimeInstallJob extends Job {
@@ -86,18 +86,14 @@ public boolean belongsTo(Object family) {
       return J2EEElementChangedListener.PROJECT_COMPONENT_UPDATE_JOB_FAMILY.equals(family);
     }
 
-    private void waitUntilJsdtIsFixedFacet() {
+    private void waitUntilJsdtIsFixedFacet(IProgressMonitor monitor) throws InterruptedException {
       try {
         IProjectFacet jsdtFacet = ProjectFacetsManager.getProjectFacet(JSDT_FACET_ID);
-        for (int times = 0;
-            times < MAX_JSDT_CHECK_RETRIES && !facetedProject.isFixedProjectFacet(jsdtFacet);
-            times++) {
-          try {
-            // To prevent going into the SLEEPING state, don't use "Job.schedule(100)".
-            Thread.sleep(100 /* ms */);
-          } catch (InterruptedException ex) {
-            // Keep waiting.
+        for (int times = 0; !monitor.isCanceled() && times < MAX_JSDT_CHECK_RETRIES; times++) {
+          if (facetedProject.isFixedProjectFacet(jsdtFacet)) {
+            return;
           }
+          Thread.sleep(100 /* ms */);
         }
       } catch (IllegalArgumentException ex) {
         // JSDT facet itself doesn't exist. (Should not really happen.) Ignore and fall through.
@@ -109,14 +105,20 @@ protected IStatus run(IProgressMonitor monitor) {
       try {
         // https://github.com/GoogleCloudPlatform/google-cloud-eclipse/issues/1155
         // Wait until the first ConvertJob installs the JSDT facet.
-        waitUntilJsdtIsFixedFacet();
+        waitUntilJsdtIsFixedFacet(monitor);
+        if (monitor.isCanceled()) {
+          return Status.CANCEL_STATUS;
+        }
         AppEngineStandardFacet.installAllAppEngineRuntimes(facetedProject, monitor);
         return Status.OK_STATUS;
+
       } catch (CoreException ex) {
         return ex.getStatus();
+      } catch (InterruptedException ex) {
+        return Status.CANCEL_STATUS;
       } finally {
-        // Now resume all the suspended jobs (including the second ConvertJob).
-        NonSystemJobSuspender.resume();
+        // Now resume the second ConvertJob.
+        ConvertJobSuspender.resume();
       }
     }
   }