Community
Participate
Working Groups
Created attachment 76826 [details] Fix that seems to enable jobs to be cancelled from the ProgressView Symptom. Create non user non system jobs and can't seem to cancel them from progress view. Debugged it and found the code in JobInfo public void cancel() { this.canceled = true; this.job.cancel(); //Call the refresh so that this is updated immediately ProgressManager.getInstance().refreshJobInfo(this); } Note that this sets the cancel status to true, before calling this.job.cancel This then ends up calling JobManager.cancel(InternalJob aJob){ ... if (monitor != null) { if (!monitor.isCanceled()) { monitor.setCanceled(true); job.canceling(); } return false; } .. Note that what this does is check the monitor to see if it's already cancelled. This does this by returning the JobInfo from the job that was already cancelled and the cancelling method on the job is never called. the fix that seems to work is to set the isCancelled to true before doing the cancel of the job, and also having a flag to stop recursion. private boolean isCancelling = false; public void cancel() { if(!isCancelling){ isCancelling = true; this.job.cancel(); this.canceled = true; // Call the refresh so that this is updated immediately ProgressManager.getInstance().refreshJobInfo(this); } } }
Looking at the code in JobManager we hit the following before we hit the code you are pointing out: synchronized (lock) { switch (job.getState()) { case Job.NONE : return true; case Job.RUNNING : //cannot cancel a job that has already started (as opposed to ABOUT_TO_RUN) if (job.internalGetState() == Job.RUNNING) { monitor = job.getProgressMonitor(); break; } //signal that the job should be canceled before it gets a chance to run job.setAboutToRunCanceled(true); return true; default : changeState(job, Job.NONE); } } We would only get to the code you are concerned about if the job is waiting or if it is sleeping which doesn't jive with what you are describing. Is your problem that sleeping or waiting jobs are not getting the cancelling call?
Sorry - found the use case. It works best on a long running job - my test job was being closed while I debugged. So the problem is that the job gets cancelled from the progress view but cancelling() is not being called correct?
The problem here is the addition of the new #cancelling() API in Job. If a JopbInfo gets cancelled then we need to leave it in an uncancelled state so that Job#cancelling gets called. The problem is that the job will call cancel again on the JobInfo. Keeping a second flag as you suggest is the way to prevent this problem but I am reluctant to add yet another piece of state. John we should talk this over.
Yes, the problem is that the cancelling() method isn't called. The job I have is a long running task that is doing database IO and I need to be called back when it's cancelled so I can close connections and set state so that threads run to completion and so forth. I agree that the extra state is hacky and a better solution should be found - whatever works best as long as the job gets called back. Cheers, Joe
Just FYI - the bug applies to both user and non-user jobs. canceling gets called in a user job if the user action was to click on cancel in the progress DIALOG, while canecling does not get called (as in non-user jobs) one the user jobs is 'pushed' in the background and the canceling request comes thru the progress view rather than the dialog. The dialog invoeks job.cancel() and that works The progressview invokes jobinfo.cancel() and that's broken. could you just fix the progress ui to invoke jobinfo.getJob().cancel() ...
(In reply to comment #5) > could you just fix the progress ui to invoke jobinfo.getJob().cancel() ... well works but it's ugly since there are various places that call org.eclipse.ui.internal.progress.JobTreeElement.cancel() :(
Eduardo re: comment #5 this is what we call in JobInfo#cancel()
(In reply to comment #7) > Eduardo re: comment #5 this is what we call in JobInfo#cancel() sorry I did not explain myself well - the Ui calls JobInfo.cancel() which is not a straight call to job.cancel (therein lies this bug!). this is the call stack Thread [main] (Suspended (breakpoint at line 95 in JobInfo)) JobInfo.cancel() line: 95 ProgressInfoItem.cancelOrRemove() line: 315 ProgressInfoItem$1.widgetSelected(SelectionEvent) line: 228 TypedListener.handleEvent(Event) line: 227 EventTable.sendEvent(Event) line: 66 ToolItem(Widget).sendEvent(Event) line: 1101 Display.runDeferredEvents() line: 3319 Display.readAndDispatch() line: 2971 if the UI called jobinfo.getJob().cancel() instead the cancellation would actually work - but the JobInfo.cancel() class is actually used elsewhere so fixing the UI as I initially thought is not viable. Joe's solution is.
So are you saying this.canceled = true; this.job.cancel(); //Call the refresh so that this is updated immediately ProgressManager.getInstance().refreshJobInfo(this); should be this.job.cancel(); this.canceled = true; //Call the refresh so that this is updated immediately ProgressManager.getInstance().refreshJobInfo(this);
(In reply to comment #9) Tod, I think Joe gave the correct solution in post #1 <quote> the fix that seems to work is to set the isCancelled to true before doing the cancel of the job, and also having a flag to stop recursion. private boolean isCancelling = false; public void cancel() { if(!isCancelling){ isCancelling = true; this.job.cancel(); this.canceled = true; // Call the refresh so that this is updated immediately ProgressManager.getInstance().refreshJobInfo(this); } } } </quote>
(In reply to comment #9) > So are you saying > this.canceled = true; > this.job.cancel(); > //Call the refresh so that this is updated immediately > ProgressManager.getInstance().refreshJobInfo(this); > should be > this.job.cancel(); > this.canceled = true; > //Call the refresh so that this is updated immediately > ProgressManager.getInstance().refreshJobInfo(this); Hi Tod, No this fix won't work by just swopping the two lines. If you do this.job.cancel() before setting the canceled flag to true it goes into a recursive overflow. That's why the extra "isCancelling" flag works best. Regards, Joe
Thanks for the analysis everyone. John and I have talked and we agree that it is better to be solved at the Jobs level as other monitors may have this issue.
Hi everyboidy - this bug seems to have been forgotten. Are there any plans to address it ?
Not forgotten, just not very high on the priority list.
Created attachment 114158 [details] Plug-in/View to test it interactively Plug-in with a view to test easily job canceling in the Workbench
Created attachment 114160 [details] Patch fixing it on jobs level Patch adds a flag to InternalJob and checking it. So core-jobs plug-in works correctly (#canceling is called exactly one time) independent of progress implementation. The check is synchronized without further effort; recursive infinitive loop cannot happen.
Created attachment 114161 [details] Patch for automatic tests Adds an additional test for canceling by progress monitor
Created attachment 114221 [details] Updated patch Stephan, thank you for the attached fix, regression test, and interactive test plugin, you know how to win a busy committer's heart. However, there is one problem with your patch that you never reset the new flag. This means if the job is run a second time the canceling() method is never called. This was causing some deadlocks in other parts of the job test suite. Attached is an updated fix that resets the flag after calling canceling(). I have also updated the test to catch this extra case, and avoided the change to the default progress provider by using a test progress provider we have in the test framework.
(In reply to comment #18) > However, there is one problem with your patch that you never reset the new > flag. This means if the job is run a second time the canceling() method is > never called. This was causing some deadlocks in other parts of the job test > suite. Yes, you are right, I forgot to reset the flag. But I think your code resets the flag too early, it does no longer prevent multiple calls of canceling() in a single run. The right position would be before or after a job is running, perhaps in JobManager#changeState for the case of new state RUNNING/ABOUNT_TO_RUN (ca. line 349)? > and avoided the change to > the default progress provider by using a test progress provider we have in the > test framework. Indeed the better solution.
Created attachment 114231 [details] Plug-in/View to test it interactively update allowing to reuse jobs.
Created attachment 114258 [details] Fix and extended test that canceling() is called only the first time Patch with bugfix as proposed in comment #19.
Created attachment 114366 [details] Updated patch Thanks again Stephan. I made a small change to clear the flag when the state becomes NONE rather than when the job is started. It seems cleaner for the job to be in a completely fresh state when in the NONE (unscheduled) state.
Fix and tests released.
*** Bug 269817 has been marked as a duplicate of this bug. ***
