Bugzilla – Full Text Bug Listing
| Summary: | [jobs] Job should allow cancel() to notify someone a cancel has been requested | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Patrick Mueller <pmuellr> | ||||
| Component: | Runtime | Assignee: | John Arthorne <john.arthorne> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | enhancement | ||||||
| Priority: | P3 | CC: | bokowski, eclipse, heath.borders, john.arthorne, rngadam | ||||
| Version: | 3.2 | ||||||
| Target Milestone: | --- | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 200997 | ||||||
| Attachments: |
|
||||||
|
Description
Patrick Mueller
I understand the motivation, but I believe a synchronous cancelation notification would be deadlock prone. If one party called Job.cancel() while holding some lock, and then a cancel event listener tried to obtain another lock, it could very easily deadlock. The CVS plugin has similar code for cancelling an unresponsive network connection. It essentially does the read in a separate thread, and then the calling thread can be left to check for cancelation and react accordingly. Will you reconsider this enhancement request? I think that the potential deadlock problem should be documented but the functionnality should still be provided as it would be very useful. Otherwise, isn't it a bit heavy to force two different threads in the same Job? I thought I had replied to John's note, but I guess I hadn't. Like Ricky, I believe the deadlock potential should be documented, but should not be a reason to not implement something like this. We're big boys and girls, we can deal with loaded guns. Besides, polling with extra threads is so 1980's ... Is there or will there be any further discussions about this particular issue for Eclipse 3.3? Or is this a WONTFIX? I'm considering the option of adding a hook method on Job that subclasses can override to specialize the cancel behaviour. This feels a bit safer than allowing an arbitrary listener to hook the cancel behaviour of any job. Even the most careful client could be deadlocked by an unknown third party listener acquiring a lock during a cancellation callback. This would be similar to the shouldSchedule and shouldRun hook methods that occur on schedule() and prior to run(). I'm very much in favor of some kind of mechanism for being notified in some way that a not was canceled by the user. I have nearly the exact same situation as Patrick does, and this would be very helpful. I can obviously work around the lack of functionality with a polling Thread, but I just hate the idea of polling. I will add the hook method. I have added a new protected API method - Job#canceling. This method is called when the job is running and its progress monitor cancel flag has been set to true for the first time. I.e., a cancelation request has been made, but the job is still running. Thank you John, I'm looking forward to using your new method! This will help replace the kludge we have right now. [Did it really just take you 5mn (17:31-17:36) to do this, tests and all? ;-).] I think I had already decided on the exact solution when I added the comment. Adding a hook method isn't very complicated ;) I later added automated tests in org.eclipse.core.tests.runtime.jobs.JobTest#testCanceling() *** Bug 162577 has been marked as a duplicate of this bug. *** You marked bug Bug 162577 as duplicated. Unfortunately, the fix does not solve the problem. When I run the following job in eclipse (with the CVS version of the jobs plugin) and cancel it using the cancel button in the progress bar, the job does not get canceled (canceled does not get called). class BlockingJob extends Job { private Thread fCurrentThread; public BlockingJob() { super("Blocking Job"); } protected IStatus run(IProgressMonitor monitor) { synchronized(this) { fCurrentThread=Thread.currentThread(); } try { // the only way to get out is to call // Thread.interrupt(); Thread.sleep(10000000); } catch (InterruptedException e) { return Status.CANCEL_STATUS; } finally { // the job is done and we have don't // want to interrupt another job synchronized(this) { fCurrentThread=null; } } return Status.OK_STATUS; } protected synchronized void canceled() { if(fCurrentThread!=null) fCurrentThread.interrupt(); } } I don't understand comment #1. How would calling Thread.interrupt() cause a deadlock? Blocking calls and Thread.interrupt() are meant to work together and I don't understand why this basic notification mechanism is not supported by the jobs framework. Here is a solution using another thread (java 1.4). Creating another thread (or using the java.util.concurrent.Executor) adds more complexity and I agree with the other posters that polling seems pretty stupid. class InterruptableJob extends Job { /** * synchronize is not needed because the set 'happens-before' the get. * http://www-128.ibm.com/developerworks/java/library/j-jtp03304/ */ private IStatus fStatus; public InterruptableJob() { super("Blocking Job"); } protected IStatus run(IProgressMonitor monitor) { Thread thread=new Thread("Annoying Polling IMonitor Thread") { public void run() { try { // the only way to get out is to call // Thread.interrupt(); Thread.sleep(10000000); } catch (InterruptedException e) { fStatus=Status.CANCEL_STATUS; } fStatus=Status.OK_STATUS; } }; thread.start(); try { // a stupid polling loop while(thread.isAlive()) { if(monitor.isCanceled()) { thread.interrupt(); break; } Thread.sleep(1); } thread.join(); } catch (InterruptedException e) { return Status.CANCEL_STATUS; } return fStatus; } } What I really want is that canceling a running job interrupts the thread. What is the problem with that? Ok, for backward compatibility, it should be possible to the a flag on the job to interrupt on cancel: myJob.setInterruptOnCancel(true); And then cancel would interrupt the thread. No callback needed. And I think it would solve all the problems described in the bug. Created attachment 52904 [details] Patch to optionally call Thread.interrupt() on Job.cancel() I created a patch that adds an interruptOnCancel attribute to Job. If it is true, then the job is running Thread.interrupt() gets called when the job is canceled. This code now works as expected: class InterruptableJob extends Job { public InterruptableJob() { super(" Interruptable Job"); setInterruptOnCancel(true); } protected IStatus run(IProgressMonitor monitor) { try { // the only way to get out is to call // Thread.interrupt(); Thread.sleep(10000000); } catch (InterruptedException e) { return Status.CANCEL_STATUS; } return Status.OK_STATUS; } } Reading the comments again, I come to the conclusion that there are two separate problems: 1) Being able to call arbitrary code on cancel. 2) Calling Thread.interrupt() to interrupt blocking calls. Problem 1) probably boils down to call Thread.interrupt(). However some external libraries might have some other methods that set some flags before calling Thread.interrupt(). This has potential deadlock problems. Problem 2) is easier to solve and less problematic. However, the only potential problem is that one interrupt might not be enough, because there is a lot code swallows interrupts (and because eclipse does not use interrupts most people don't even know why swallowing interrupts is bad): try { Thread.sleep(1); } catch (InterruptedException e2) { // ignore } This is really bad practice (see bug 157604 and http://michaelscharf.blogspot.com/2006/09/dont-swallow-interruptedexception-call.html). Therefore a job should be cancelable multiple times or re-interrupt after a timeout, but this is probably another problem.... Why couldn't you just call Thread.currentThread().interrupt() inside your Job's cancelling method? > Why couldn't you just call Thread.currentThread().interrupt() inside
> your Job's cancelling method?
No! For two reasons:
1. Job.cancel() is final and therefore not overridable.
2. Job.cancel() essentially calls JobManager.cancel(this). This method is called from other places as well, like JobManager.shutDown(). Even if Job.cancel() would be overridden, it would not get called on JobManager.shutDown(). This is why a hook is needed or direct support for Thread.interrupt.
While I don't want to talk someone out of using interrupt() if they insist, I've found the semantics of it to be fairly muddy. In my case, why I submitted the bug in the first place, I actually have a controlled way of doing the 'interrupt' with quite clear semantics (I'm blocked on a socket, and want to 'close' it on cancel to get the socket to 'wake up'). I would hope this would suffice for a great number of people. And so I'm wondering if you can't arrange to make your interrupt() call within the new canceling() method that John added. And as a generalist comment, I wonder if people won't get confused with an interruptOnCancel setting, thinking they'll get some kind of magical behaviour out of it, when in fact, they might not. Or they might, in an intermittent fashion (during testing, but not during production, say). That, combined with my general leeriness of the whole interrupt() thing has my gut telling me adding this API isn't the best thing to do. I'm talking about the Job#cancelling hook method that was just added. Of course, you cannot override the Job#cancel method as it is final. Comment #14 is the same question as comment #16 I tried the new canceling method and it simply does not work as expected (see comment #12). Maybe it needs to be fixed. In that case just forget my proposal, because it allows to solve the Thread.interrupt() problem. Thread.interrupt() is the only way to get out of blocking calls. Interrupts can be used for anything. But the common case it to use it for canceling. It is cooperative because the caller and the receiver have to agree what it means. IProgressMonitor.isCanceled() also cooperative . The cancellable operation has to periodically check the cancel state. But it does not provide a way to interrupt a blocking call. So, we have two cooperative way of canceling. One is build into java and allows interrupting blocking calls. The other cannot deal with blocking calls and it requires state polling. I would find it very natural to extend the eclipse polling mechanism to use Thread.interrupt if desired. This is fine for code that fully under control of eclipse developer. This is what I wanted to address with the interruptOnCancel. There is another case, where an existing library has to be integrated into eclipse. In this case polling for IProgressMonitor.isCanceled() is not enough, because the 3rd party library is not prepared for that. Instead some method has to be called to cancel the library call. That's the case where a hook mechanism is needed. In that hook method Thread.interrupt() could be called. However this generic mechanism opens the door for nasty deadlocks. The interruptOnCancel would not have the deadlock problems but it would not solve the general problem. The reason Job.canceled() does not work in some cases is because it is *not* called when the ProgressMonitor is used to cancel the Job. It is only called when Job.cancel() is called and the IProgressMonitor.isCanceled() returns false (see comment #12). John says in comment #8: > This method is called when the job is running and its progress > monitor cancel flag has been set to true for the first time When the job gets canceled the progress monitor is set to canceled before Job.cancel() is called and therefore Job.canceling gets never called... John, is this a bug or a feature? w/r/t comment #18 ... you mention the new canceling() method doesn't work and point to comment #12, but I don't see that the code in the comment using canceling(). "Thread.interrupt() is the only way to get out of blocking calls." Ouch. That's EXACTLY what I was afraid of when I mentioned "magical behaviour". That method most ASSUREDLY is NOT the only way to get out of blocking calls. Most importantly, it is not guaranteed to get you out of any blocking call, only things that respond to it (like the new InterruptibleChannel stuff). If Sun had forced every call that can 'block' to be interrruptable, that would be one thing. But they didn't. While it might have been intended to be a general purpose facility for any blocking behaviour, it currently IS NOT. It only works for an interesting, but limited # of blocking methods. As such, I'm not sure it deserves special status with an API of it's own. I might well ask for an API 'closeOnCancel(InputStream)' that would do a 'close' on the specified InputStream when a cancel was requested. Where do you stop? Yikes, I don't know where to weigh in here. Michael, I marked your bug as a duplicate because you wanted additional behaviour when Job.cancel() was called. This has been implemented by the new canceling() hook method. However, as you have since discovered, programmatic cancelation and end-user cancelation have different code paths. In the case of end-user cancelation, the progress widget in the UI directly sets the canceled flag in the IProgressMonitor, and no code is called at all at the core jobs level. If you wanted Thread.interrupt() to be called whenever IProgressMonitor#setCanceled was called, this behaviour would need to be added to all progress monitor implementations. Personally I don't think this is good idea, and very unlikely to happen, but you're welcome to enter an enhancement at the UI level. There is nothing unique to the job API here in its use of the general Eclipse IProgressMonitor mechanism. If you wanted such a behaviour change to how progress monitors operate, any solution should not be specific to jobs, but apply to all IProgressMonitors. So, we still need to poll to determine when the user requests a cancel? I thought that was the whole point of adding this hook. From the beginning, this request has been about adding a hook when Job#cancel() is called, and that is what I have added. Hooking the behaviour when the UI sets the progress monitor's cancelation flag is a whole other problem... I think the implied assumption of this request was that Job.cancel() gets called when the user cancels the request from the UI. This is obviously not true. IProgressMonitor uses polling to cancel a request. Job.cancel is a direct call which allows a hook like canceling(). There are currently two ways to combine the two approaches: 1) Do the work in separate thread and let the job poll this thread (see comment #12) 2) Have one IProgressMonitor watch thread, that polls periodically the IProgressMonitor.isCanceled() of all active jobs that want a notification when canceling. Once the isCanceled state changes it could call the Job.canceling... Approach 1) adds an additional thread for each cancelable Job. Approach 2) would add only one more thread. It could be implemented in the jobs plugin. Oops, I mean:
> 1) Do the work in separate thread and let the job poll this thread
1) Do the work in separate thread and let the job poll the progress monitor
|