Bug 190750 - [api] User cancellation should be reported by OperationCanceledException rather than InterruptedException
Summary: [api] User cancellation should be reported by OperationCanceledException rath...
Status: ASSIGNED
Alias: None
Product: Target Management
Classification: Tools
Component: RSE (show other bugs)
Version: 2.0   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: Future   Edit
Assignee: David Dykstal CLA
QA Contact: Martin Oberhuber CLA
URL:
Whiteboard:
Keywords: api
Depends on: 176606 189483 190803
Blocks:
  Show dependency tree
 
Reported: 2007-06-04 04:39 EDT by Martin Oberhuber CLA
Modified: 2008-02-02 13:01 EST (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Oberhuber CLA 2007-06-04 04:39:04 EDT
Historically, some RSE services like the ConnectorService throw InterruptedException in order to notify calling code of user cancellation.

This is not in line with the API documentation of InterruptedException. Code should throw OperationCanceledException instead.

Even though we cannot change all code immediately to throw OperationCanceledException rather than InterruptedException right now, because it would mean an API breakage, all calling code (like SystemFetchOperation) should be prepared to handle OperationCanceledException the same way as InterruptedException.

This bug should be used to track any changes in this area, and be closed when no more code throws any InterruptedException to notify about user cancellation.
Comment 1 Martin Oberhuber CLA 2007-06-04 04:41:56 EDT
See also bug #189483 comment 6
Comment 2 David Dykstal CLA 2007-06-04 07:49:38 EDT
Martin -- I'm not comfortable with this request. Even eclipse services - particularly those in jface - throw InterruptedException instead of OperationCanceledException for canceld operations.
Comment 3 Martin Oberhuber CLA 2007-06-04 08:06:49 EDT
Dave -- you are right that there are still places where Eclipse throws InterruptedException. These are easily found by selecting the InterruptedException() constructor and performing a referenced-by query.

Doing this, and filtering out those cases where InterruptedException is thrown properly (i.e. after checking for Thread.isInterrupted()), I get the following matches (only one match is listed for each component):

Platform/Compare -- TextMergeViewer
Platform/Search -- InternalSearchUI
Platform/Team -- ModelParticipantAction
Platform/Text -- ConvertRunnable
Platform/IDE -- FileSystemExportOperation
JFace -- ModalContext
CDT -- CApplicationLaunchShortcut

Some of these, e.g. InternalSearchUI, and ProgressManager, even convert the OperationCanceledException into an InterruptedException:

    } catch (OperationCanceledException e) {
        throw new InterruptedException();
    }

Comparing this against the calls of the OperationCanceledException() constructor, I find slightly more places using the OperationCanceledException. 

To me, it looks like InterruptedException is Legacy, and was replaced by the more modern OperationCanceledException. But I might be wrong. Let's defer this discussion for now and pick it up again after 2.0 -- I'd like to get the opinion of some Eclipse Platform Leads on this.

If you want to be extra safe for bug #189483, feel free to catch both OperationCanceledException and InterruptedException. Otherwise, I'm fine with just keeping it as is for 2.0.
Comment 4 David Dykstal CLA 2007-06-04 08:23:59 EDT
(In reply to comment #3)

Yes, I'm sure it is legacy for the platform, just as it is for us. I say we use this report and include it as a part of a larger "clean up the api" item for 3.0 and make sure we get all instances of this. For now, however, I'd like to keep things as they are.
Comment 5 Martin Oberhuber CLA 2007-06-04 09:05:45 EDT
Yes. Assigned to Future.
Comment 6 Martin Oberhuber CLA 2007-06-08 01:23:29 EDT
The big problem with using InterruptedException for reporting user cancellation is that it cannot be distinguieshed from actual thread interruption e.g. as part of the shutdown sequence.

Thread interruption must be handled differently and not just ignored, see 
http://michaelscharf.blogspot.com/2006/09/dont-swallow-interruptedexception-call.html

Therefore we should switch to using OperationCanceledException where we can.