diff options
author | Anton Leherbauer | 2015-04-23 11:33:26 +0000 |
---|---|---|
committer | Anton Leherbauer | 2015-04-23 12:35:06 +0000 |
commit | cfa8c3b4d3b6f0815b4e8dc73d16dfbbafc10e28 (patch) | |
tree | 7182aadc5f6b569a797e9ed4cf8c7fe34a626cfc /target_explorer | |
parent | 05cfd00814ad6d0854ffdb6127b190217747695c (diff) | |
download | org.eclipse.tcf-cfa8c3b4d3b6f0815b4e8dc73d16dfbbafc10e28.tar.gz org.eclipse.tcf-cfa8c3b4d3b6f0815b4e8dc73d16dfbbafc10e28.tar.xz org.eclipse.tcf-cfa8c3b4d3b6f0815b4e8dc73d16dfbbafc10e28.zip |
Target Explorer: Fix race conditions when canceling stepper jobs
- Avoid manipulating JobMonitor after job has finished
- Spurious ConcurrentModificationException in CancelJobsStep
Change-Id: Ia7baea03fc9f9ce093d9f26ba3e176e1405246ff
Signed-off-by: Anton Leherbauer <anton.leherbauer@windriver.com>
Diffstat (limited to 'target_explorer')
3 files changed, 110 insertions, 84 deletions
diff --git a/target_explorer/plugins/org.eclipse.tcf.te.runtime.stepper/src/org/eclipse/tcf/te/runtime/stepper/internal/PropertyTester.java b/target_explorer/plugins/org.eclipse.tcf.te.runtime.stepper/src/org/eclipse/tcf/te/runtime/stepper/internal/PropertyTester.java index 540314f45..6c1de434b 100644 --- a/target_explorer/plugins/org.eclipse.tcf.te.runtime.stepper/src/org/eclipse/tcf/te/runtime/stepper/internal/PropertyTester.java +++ b/target_explorer/plugins/org.eclipse.tcf.te.runtime.stepper/src/org/eclipse/tcf/te/runtime/stepper/internal/PropertyTester.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2011, 2014 Wind River Systems, Inc. and others. All rights reserved. + * Copyright (c) 2011, 2015 Wind River Systems, Inc. and others. All rights reserved. * This program and the accompanying materials are made available under the terms * of the Eclipse Public License v1.0 which accompanies this distribution, and is * available at http://www.eclipse.org/legal/epl-v10.html @@ -10,7 +10,6 @@ package org.eclipse.tcf.te.runtime.stepper.internal; import java.util.List; -import java.util.Map; import org.eclipse.core.runtime.jobs.Job; import org.eclipse.tcf.te.runtime.services.ServiceManager; @@ -35,12 +34,10 @@ public class PropertyTester extends org.eclipse.core.expressions.PropertyTester if ("isRunning".equals(property)) { //$NON-NLS-1$ if (operation != null) { - Map<String,List<Job>> jobs = StepperJob.getJobs(receiver); - if (jobs != null && jobs.containsKey(operation)) { - for (Job job : jobs.get(operation)) { - if (job instanceof StepperJob && !((StepperJob)job).isCanceled() && !((StepperJob)job).isFinished()) { - return true; - } + List<Job> jobs = StepperJob.getJobsForOperation(receiver, operation); + for (Job job : jobs) { + if (job instanceof StepperJob && !((StepperJob)job).isCanceled() && !((StepperJob)job).isFinished()) { + return true; } } } @@ -48,12 +45,10 @@ public class PropertyTester extends org.eclipse.core.expressions.PropertyTester if ("isRunningOrCanceled".equals(property)) { //$NON-NLS-1$ if (operation != null) { - Map<String,List<Job>> jobs = StepperJob.getJobs(receiver); - if (jobs != null && jobs.containsKey(operation)) { - for (Job job : jobs.get(operation)) { - if (job instanceof StepperJob && !((StepperJob)job).isFinished()) { - return true; - } + List<Job> jobs = StepperJob.getJobsForOperation(receiver, operation); + for (Job job : jobs) { + if (job instanceof StepperJob && !((StepperJob)job).isFinished()) { + return true; } } } diff --git a/target_explorer/plugins/org.eclipse.tcf.te.runtime.stepper/src/org/eclipse/tcf/te/runtime/stepper/job/StepperJob.java b/target_explorer/plugins/org.eclipse.tcf.te.runtime.stepper/src/org/eclipse/tcf/te/runtime/stepper/job/StepperJob.java index d9dad274c..3452e05a6 100644 --- a/target_explorer/plugins/org.eclipse.tcf.te.runtime.stepper/src/org/eclipse/tcf/te/runtime/stepper/job/StepperJob.java +++ b/target_explorer/plugins/org.eclipse.tcf.te.runtime.stepper/src/org/eclipse/tcf/te/runtime/stepper/job/StepperJob.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2013, 2014 Wind River Systems, Inc. and others. All rights reserved. + * Copyright (c) 2013, 2015 Wind River Systems, Inc. and others. All rights reserved. * This program and the accompanying materials are made available under the terms * of the Eclipse Public License v1.0 which accompanies this distribution, and is * available at http://www.eclipse.org/legal/epl-v10.html @@ -10,6 +10,7 @@ package org.eclipse.tcf.te.runtime.stepper.job; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -18,6 +19,7 @@ import org.eclipse.core.runtime.Assert; import org.eclipse.core.runtime.CoreException; import org.eclipse.core.runtime.IProgressMonitor; import org.eclipse.core.runtime.IStatus; +import org.eclipse.core.runtime.ProgressMonitorWrapper; import org.eclipse.core.runtime.Status; import org.eclipse.core.runtime.jobs.IJobChangeEvent; import org.eclipse.core.runtime.jobs.IJobChangeListener; @@ -56,68 +58,85 @@ public final class StepperJob extends Job { private boolean isCanceled = false; private boolean statusHandled = false; - private class NotCancelableProgressMonitor implements IProgressMonitor { + /** + * A progress monitor wrapper which blocks any access to a JobMonitor + * after the job has finished. This prevents creation of stale progress + * items in the ProgressManager. + */ + private static class StepperProgressMonitor extends ProgressMonitorWrapper { - private final IProgressMonitor monitor; + private final boolean cancelable; + private volatile boolean jobDone; + private volatile boolean canceled; + + public StepperProgressMonitor(IProgressMonitor monitor, boolean isCancelable) { + super(monitor); + this.cancelable = isCancelable; + } - public NotCancelableProgressMonitor(IProgressMonitor monitor) { - this.monitor = monitor; + void jobDone(boolean canceled) { + // Job has finished - block any access to the JobMonitor + jobDone = true; + this.canceled = canceled; } @Override public void beginTask(String name, int totalWork) { - monitor.beginTask(name, totalWork); + if (!jobDone) super.beginTask(name, totalWork); } @Override public void done() { - monitor.done(); + if (!jobDone) super.done(); } @Override public void internalWorked(double work) { - monitor.internalWorked(work); + if (!jobDone) super.internalWorked(work); } @Override public boolean isCanceled() { - return false; + if (cancelable && !jobDone) return super.isCanceled(); + return canceled; } @Override public void setCanceled(boolean value) { - monitor.setCanceled(false); + canceled = cancelable && value; + if (!jobDone) super.setCanceled(canceled); } @Override public void setTaskName(String name) { - monitor.setTaskName(name); + if (!jobDone) super.setTaskName(name); } @Override public void subTask(String name) { - monitor.subTask(name); + if (!jobDone) super.subTask(name); } @Override public void worked(int work) { - monitor.worked(work); + if (!jobDone) super.worked(work); } } private class JobChangeListener extends JobChangeAdapter { + private final StepperProgressMonitor jobMonitor; + /** * Constructor. */ - public JobChangeListener() { + public JobChangeListener(StepperProgressMonitor monitor) { + this.jobMonitor = monitor; } - /* (non-Javadoc) - * @see org.eclipse.core.runtime.jobs.JobChangeAdapter#done(org.eclipse.core.runtime.jobs.IJobChangeEvent) - */ @Override public void done(IJobChangeEvent event) { + jobMonitor.jobDone(isCanceled()); handleStatus(event.getResult()); removeJobChangeListener(this); @@ -132,7 +151,7 @@ public final class StepperJob extends Job { * @param data The stepper data. * @param stepGroupId The step group id to execute. * @param operation The operation to execute. - * @param isCancelable <code>true</code> if the job can be canceled. + * @param cancelable <code>true</code> if the job can be canceled. * @param handleStatus <code>true</code> if the job should handle the status itself and return always <code>Status.OK_STATUS</code>. */ public StepperJob(String name, IStepContext stepContext, IPropertiesContainer data, String stepGroupId, String operation, boolean isCancelable, boolean handleStatus) { @@ -182,20 +201,34 @@ public final class StepperJob extends Job { return jobs; } + public static List<Job> getJobsForOperation(Object context, String operation) { + Map<String, List<Job>> jobs = getJobs(context); + synchronized (jobs) { + List<Job> jobsForOperation = jobs.get(operation); + if (jobsForOperation == null) + return Collections.emptyList(); + return new ArrayList<Job>(jobsForOperation); + } + } + public static void addJob(Map<String,List<Job>> jobs, Job job, String operation) { - List<Job> jobsForOperation = jobs.get(operation); - if (jobsForOperation == null) { - jobsForOperation = new ArrayList<Job>(); - jobs.put(operation, jobsForOperation); - } - jobsForOperation.add(job); + synchronized (jobs) { + List<Job> jobsForOperation = jobs.get(operation); + if (jobsForOperation == null) { + jobsForOperation = new ArrayList<Job>(); + jobs.put(operation, jobsForOperation); + } + jobsForOperation.add(job); + } } public static void removeJob(Map<String,List<Job>> jobs, Job job, String operation) { - List<Job> jobsForOperation = jobs.get(operation); - if (jobsForOperation != null) { - jobsForOperation.remove(job); - } + synchronized (jobs) { + List<Job> jobsForOperation = jobs.get(operation); + if (jobsForOperation != null) { + jobsForOperation.remove(job); + } + } } public static void setJobs(Map<String,List<Job>> jobs, Object context) { @@ -234,11 +267,10 @@ public final class StepperJob extends Job { @Override public final IStatus run(IProgressMonitor monitor) { - if (!isCancelable) { - monitor = new NotCancelableProgressMonitor(monitor); - } + StepperProgressMonitor stepperMonitor = new StepperProgressMonitor(monitor, isCancelable); + monitor = stepperMonitor; - IJobChangeListener listener = new JobChangeListener(); + IJobChangeListener listener = new JobChangeListener(stepperMonitor); addJobChangeListener(listener); // The stepper instance to be used diff --git a/target_explorer/plugins/org.eclipse.tcf.te.runtime.stepper/src/org/eclipse/tcf/te/runtime/stepper/steps/CancelJobsStep.java b/target_explorer/plugins/org.eclipse.tcf.te.runtime.stepper/src/org/eclipse/tcf/te/runtime/stepper/steps/CancelJobsStep.java index a199b4634..7dc1087ed 100644 --- a/target_explorer/plugins/org.eclipse.tcf.te.runtime.stepper/src/org/eclipse/tcf/te/runtime/stepper/steps/CancelJobsStep.java +++ b/target_explorer/plugins/org.eclipse.tcf.te.runtime.stepper/src/org/eclipse/tcf/te/runtime/stepper/steps/CancelJobsStep.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2013, 2014 Wind River Systems, Inc. and others. All rights reserved. + * Copyright (c) 2013, 2015 Wind River Systems, Inc. and others. All rights reserved. * This program and the accompanying materials are made available under the terms * of the Eclipse Public License v1.0 which accompanies this distribution, and is * available at http://www.eclipse.org/legal/epl-v10.html @@ -10,6 +10,8 @@ package org.eclipse.tcf.te.runtime.stepper.steps; +import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; @@ -57,46 +59,43 @@ public class CancelJobsStep extends AbstractStep { Job thisJob = (StepperJob)StepperAttributeUtil.getProperty(IStepAttributes.ATTR_STEPPER_JOB, fullQualifiedId, data); final AsyncCallbackCollector collector = new AsyncCallbackCollector(); - Map<String,List<Job>> jobs = StepperJob.getJobs(context.getContextObject()); - final AtomicInteger numJobsToCancel = new AtomicInteger(0); + Map<String,List<Job>> jobs = new HashMap<String, List<Job>>(StepperJob.getJobs(context.getContextObject())); final AtomicInteger canceledJobs = new AtomicInteger(0); - for (String op : jobs.keySet()) { - for (Job job : jobs.get(op)) { - if (job != thisJob && - (!(job instanceof StepperJob) || ((StepperJob)job).isCancelable())) { - numJobsToCancel.set(numJobsToCancel.get()+1); - } + final List<Job> jobsToCancel = new ArrayList<Job>(); + synchronized (jobs) { + for (String op : jobs.keySet()) { + for (Job job : jobs.get(op)) { + if (job != thisJob && + (!(job instanceof StepperJob) || ((StepperJob)job).isCancelable())) { + jobsToCancel.add(job); + } + } } } - for (String op : jobs.keySet()) { - for (Job job : jobs.get(op)) { - if (job != thisJob) { - if (job instanceof StepperJob && ((StepperJob)job).isCancelable()) { - Callback jobCb = new Callback(((StepperJob)job).getJobCallback()) { - @Override - protected void internalDone(Object caller, IStatus status) { - canceledJobs.set(canceledJobs.get()+1); - ProgressHelper.worked(monitor, getTotalWork(context, data) / numJobsToCancel.get()); - ProgressHelper.setSubTaskName(monitor, canceledJobs.get() + " of " + numJobsToCancel.get() + " Jobs canceled."); //$NON-NLS-1$ //$NON-NLS-2$ - collector.removeCallback(this); - } - }; - if (job.getState() == Job.RUNNING) { - collector.addCallback(jobCb); - ((StepperJob)job).setJobCallback(jobCb); - } - else { - canceledJobs.set(canceledJobs.get()+1); - } - } - else { - canceledJobs.set(canceledJobs.get()+1); - } - job.cancel(); - } - } - - } + for (Job job : jobsToCancel) { + if (job instanceof StepperJob && ((StepperJob)job).isCancelable()) { + Callback jobCb = new Callback(((StepperJob)job).getJobCallback()) { + @Override + protected void internalDone(Object caller, IStatus status) { + canceledJobs.incrementAndGet(); + ProgressHelper.worked(monitor, getTotalWork(context, data) / jobsToCancel.size()); + ProgressHelper.setSubTaskName(monitor, canceledJobs.get() + " of " + jobsToCancel.size() + " Jobs canceled."); //$NON-NLS-1$ //$NON-NLS-2$ + collector.removeCallback(this); + } + }; + if (job.getState() == Job.RUNNING) { + collector.addCallback(jobCb); + ((StepperJob)job).setJobCallback(jobCb); + } + else { + canceledJobs.incrementAndGet(); + } + } + else { + canceledJobs.incrementAndGet(); + } + job.cancel(); + } collector.initDone(); |