Skip to content

Commit

Permalink
Don't replace the monitor set by a group when starting a new job
Browse files Browse the repository at this point in the history
- 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 #664
  • Loading branch information
fedejeanne committed Sep 11, 2023
1 parent f385650 commit a055cfa
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 14 deletions.
2 changes: 1 addition & 1 deletion runtime/bundles/org.eclipse.core.jobs/META-INF/MANIFEST.MF
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -554,15 +554,12 @@ private <T, J extends InternalJob> T withWriteLock(J job, Function<J, T> 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
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -1602,6 +1620,37 @@ protected IStatus run(IProgressMonitor monitor) {
group.done();
}

public void testSetProgressGroup_propagatesGroupCancellation() throws InterruptedException {
Job checkMonitorJob = createJobReactingToCancelRequest();

final IProgressMonitor progressGroup = Job.getJobManager().createProgressGroup();
progressGroup.beginTask("", 2);

checkMonitorJob.setProgressGroup(progressGroup, 1);

progressGroup.setCanceled(true);

checkMonitorJob.schedule();
checkMonitorJob.join();

assertEquals("The job should have been canceled", IStatus.CANCEL, checkMonitorJob.getResult().getSeverity());
}

private Job createJobReactingToCancelRequest() {
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
*/
Expand Down

0 comments on commit a055cfa

Please sign in to comment.