From c42251d992e4568503c43fad2f3caa16ba194d8f Mon Sep 17 00:00:00 2001 From: fedejeanne Date: Thu, 7 Sep 2023 09:09:53 +0200 Subject: [PATCH] Don't replace the monitor set by a group when starting a new job - Only set the progress monitor of the Job if it hasn't one already. This makes sure that the progress monitor set via Job::setProgressGroup is not replaced with a new one and ergo when the group is canceled, all the monitors in that group are canceled too. - Add regression test: JobTest.testSetProgressGroup_progressGroupIsCanceled_monitorInJobIsCanceled() - Simplify private method JobManager::createMonitor(Job). Remove the annotation "@GuardedBy" in it. Fixes https://github.com/eclipse-platform/eclipse.platform/issues/664 --- .../META-INF/MANIFEST.MF | 2 +- .../core/internal/jobs/JobManager.java | 16 ++--- .../core/tests/runtime/jobs/JobTest.java | 60 +++++++++++++++++-- 3 files changed, 64 insertions(+), 14 deletions(-) diff --git a/runtime/bundles/org.eclipse.core.jobs/META-INF/MANIFEST.MF b/runtime/bundles/org.eclipse.core.jobs/META-INF/MANIFEST.MF index 5b598798272..18a7aebcca2 100644 --- a/runtime/bundles/org.eclipse.core.jobs/META-INF/MANIFEST.MF +++ b/runtime/bundles/org.eclipse.core.jobs/META-INF/MANIFEST.MF @@ -2,7 +2,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-Name: %pluginName Bundle-SymbolicName: org.eclipse.core.jobs; singleton:=true -Bundle-Version: 3.15.0.qualifier +Bundle-Version: 3.15.100.qualifier Bundle-Vendor: %providerName Bundle-Localization: plugin Export-Package: org.eclipse.core.internal.jobs;x-internal:=true, diff --git a/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/JobManager.java b/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/JobManager.java index bd4f71721f4..77c49abf392 100644 --- a/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/JobManager.java +++ b/runtime/bundles/org.eclipse.core.jobs/src/org/eclipse/core/internal/jobs/JobManager.java @@ -554,15 +554,12 @@ private T withWriteLock(J job, Function functio /** * Returns a new progress monitor for this job. Never returns null. - * @GuardedBy("lock") */ private IProgressMonitor createMonitor(Job job) { - IProgressMonitor monitor = null; - if (progressProvider != null) - monitor = progressProvider.createMonitor(job); - if (monitor == null) - monitor = new NullProgressMonitor(); - return monitor; + if (progressProvider != null) { + return progressProvider.createMonitor(job); + } + return new NullProgressMonitor(); } @Override @@ -1785,7 +1782,10 @@ protected Job startJob(Worker worker) { synchronized (internal.jobStateLock) { if (internal.internalGetState() == InternalJob.ABOUT_TO_RUN) { if (shouldReallyRun && !internal.isAboutToRunCanceled()) { - internal.setProgressMonitor(createMonitor(j)); + // only set the monitor if it wasn't already set. + if (internal.getProgressMonitor() == null) { + internal.setProgressMonitor(createMonitor(j)); + } //change from ABOUT_TO_RUN to RUNNING internal.setThread(worker); internal.internalSetState(Job.RUNNING); diff --git a/runtime/tests/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/JobTest.java b/runtime/tests/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/JobTest.java index 56bda15601e..a70d82036a3 100644 --- a/runtime/tests/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/JobTest.java +++ b/runtime/tests/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/runtime/jobs/JobTest.java @@ -23,11 +23,29 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; -import java.util.concurrent.atomic.*; -import org.eclipse.core.internal.jobs.*; -import org.eclipse.core.runtime.*; -import org.eclipse.core.runtime.jobs.*; -import org.eclipse.core.tests.harness.*; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicIntegerArray; +import java.util.concurrent.atomic.AtomicLong; +import org.eclipse.core.internal.jobs.InternalJob; +import org.eclipse.core.internal.jobs.JobListeners; +import org.eclipse.core.internal.jobs.JobManager; +import org.eclipse.core.internal.jobs.Worker; +import org.eclipse.core.runtime.IProgressMonitor; +import org.eclipse.core.runtime.IStatus; +import org.eclipse.core.runtime.OperationCanceledException; +import org.eclipse.core.runtime.QualifiedName; +import org.eclipse.core.runtime.Status; +import org.eclipse.core.runtime.jobs.IJobChangeEvent; +import org.eclipse.core.runtime.jobs.IJobChangeListener; +import org.eclipse.core.runtime.jobs.IJobManager; +import org.eclipse.core.runtime.jobs.ISchedulingRule; +import org.eclipse.core.runtime.jobs.Job; +import org.eclipse.core.runtime.jobs.JobChangeAdapter; +import org.eclipse.core.runtime.jobs.LockListener; +import org.eclipse.core.tests.harness.FussyProgressMonitor; +import org.eclipse.core.tests.harness.TestBarrier2; +import org.eclipse.core.tests.harness.TestJob; import org.junit.Assert; /** @@ -1602,6 +1620,38 @@ protected IStatus run(IProgressMonitor monitor) { group.done(); } + public void testSetProgressGroup_progressGroupIsCanceled_monitorInJobIsCanceled() throws InterruptedException { + Job checkMonitorIsCanceled = createJobThatChecksItsOwnMonitor(); + + final IProgressMonitor progressGroup = Job.getJobManager().createProgressGroup(); + progressGroup.beginTask("", 2); + + checkMonitorIsCanceled.setProgressGroup(progressGroup, 1); + + // let the job do the cancellation + progressGroup.setCanceled(true); + + checkMonitorIsCanceled.schedule(); + checkMonitorIsCanceled.join(); + + assertEquals("The job should have been canceled", IStatus.CANCEL, checkMonitorIsCanceled.getResult().getSeverity()); + } + + private Job createJobThatChecksItsOwnMonitor() { + return new Job("Check if own monitor is canceled") { + + @Override + protected IStatus run(IProgressMonitor monitor) { + + if (monitor.isCanceled()) { + return Status.CANCEL_STATUS; + } + + return Status.OK_STATUS; + } + }; + } + /* * see bug #43459 */