Wrapped the OperationCanceledException in InterruptedException

[amartya4256]

eclipse-platform/eclipse.platform#853

Wrapped the OperationCanceledException in InterruptedException

[merks]

I'm not sure this is a generally and universally necessary change.

In any case, please don't merge in commits to your PR. Keep the PR with a single commit and rebase on master when necessary. The PR in this state current cannot be properly reviewed or processed...

[merks]

@amartya4256

That's better. 😄

@HeikoKlare

Are such changes generally wanted?

[github-actions]

Test Results

     900 files  +     15       900 suites  +15   46m 1s ⏱️ + 10m 25s
  7 469 tests ±       0    7 316 ✔️ ±       0  153 💤 ±    0  0 ❌ ±0 
23 556 runs  +1 962  23 047 ✔️ +1 835  509 💤 +127  0 ❌ ±0 

Results for commit 90351d0. ± Comparison against base commit bdc673c.

♻️ This comment has been updated with latest results.

[HeikoKlare]

I think this kind of change makes sense, as there are several points in the code at which InterruptedExceptions are thrown in order to make a checked exception out of an OperationCanceledException without containing information about the original exception. This makes it impossible to eventually find out where the exception was originally thrown and what its cause was (an actual operation cancellation or, for example, an interrupted thread).

As far as I know, the first kind of change improving this exception handling was d8c2f86
And another recent change that was driven by need to finding out the reasons for a thrown exception was #1259

Do you see any negative impact in these changes, @merks?

[merks]

I don’t see anything harmful. It’s just annoying (very verbose) that there is no convenient constructor. Which begs the question why there is no such constructor? Is it an unusual pattern to throw this type of exception as being caused by something else? Might we encapsulate the conversation somewhere? E.g., a toInterruptedException method on OperationCanceledException?

[HeikoKlare]

First: You are right, the repetition is verbose and error prone and we should choose some better solution.

This kind of exception wrapping seems to exist for a long time and I do not completely understand why. The obvious thing is that OperationCanceledExceptions are unchecked while InterruptedExceptions are checked and maybe InterruptedExceptions were chosen as a wrapper to make the exception checked as they were the semantically "best" fitting one (but that's only my guess). That's why I think this pattern is rather unusual and can only be found when wrapping OperationCanceledExceptions into checked ones. And since the checked exceptions are part of the API, we can also not exchange them easily.

To avoid the repetition, I would propose the following: add some wrapping method to the OperationCanceledException, such as InterruptedException wrapIntoInterruptedException() or void rethrowAsInterruptedException() throws InterruptedException. We cannot provide a convience constructor for the InterruptedException, as it's part of the JDK, so we can only provide the functionality at the OperationCanceledException or at some independent place such as a factory class.

[merks]

Yes I would be much happier to see a concise, single-line replacement. The rethrow method is interesting but the compiler won’t know that flow will not continue as it knows when there is an explicit throw statement

[HannesWell]

Yes I would be much happier to see a concise, single-line replacement. The rethrow method is interesting but the compiler won’t know that flow will not continue as it knows when there is an explicit throw statement

That's right. But if the wrap method returns an exception one can just writethrow before the method call.

[merks]

Exactly. That is why I think a method that returns the wrapping exception would be ideal.

[laeubi]

Which begs the question why there is no such constructor?

The answer is simple, because this exception is actually not to be created by user code except you have cleared the interrupted flag(!) so it can never be really "caused" by something see:
https://docs.oracle.com/javase/8/docs/api/java/lang/InterruptedException.html

Thrown when a thread is waiting, sleeping, or otherwise occupied, and the thread is interrupted, either before or during the activity.

Nothing of those is happening here! Sadly InterruptedException is misuses at several places in eclipse while the proper one to use would be CancellationException but also in this case there is no causing exception because this can't be caused by another exception, in such case one has to use ExecutionException a good example is the method get(long timeout, TimeUnit) unit) of future task

Is it an unusual pattern to throw this type of exception as being caused by something else? Might we encapsulate the conversation somewhere? E.g., a toInterruptedException method on OperationCanceledException?

I would avoid / replace this entirely instead of making it more convenient to follow this wrong pattern.

[HeikoKlare]

I would avoid / replace this entirely instead of making it more convenient to follow this wrong pattern.

I agree that no new code should follow this pattern, but we cannot easily change existing code either, as in some cases it affects public API (the InterruptedException is part of the method signature), also see eclipse-platform/eclipse.platform#853 (comment)

Also note that this misused exception is only passed outside internal code rather rarely. If I remember correctly we now have around 5 places touched in Platform and Platform UI code.

Nothing of those is happening here! Sadly InterruptedException is misuses at several places in eclipse while the proper one to use would be CancellationException but also in this case there is no causing exception because this can't be caused by another exception, in such case one has to use ExecutionException a good example is the method get(long timeout, TimeUnit) unit) of future task

It's correct that these exception would fit better, but please also note that they have been introduced into Java after at least some of this Eclipse code has been written initially.

[laeubi]

It's correct that these exception would fit better, but please also note that they have been introduced into Java after at least some of this Eclipse code has been written initially.

But even before it was wrong to use that and eclipse has OperationCanceledException for the good old days... also just because its in the API it is not appropriate to use it for different purpose ;-)

[HeikoKlare]

But even before it was wrong to use that and eclipse has OperationCanceledException for the good old days... also just because its in the API it is not appropriate to use it for different purpose ;-)

I totally agree with you :-) I just tried to understand (and partly justify) the decision made long years ago. But, anyway, we cannot change that anymore, so my primary goals would be to (1) avoid that this kind of misuse is repeated in new code and (2) that we make the best of the existing code in terms of providing information about the wrapped exception in case it is provided to non-internal code.

[HeikoKlare]

@amartya4256 can you please rebase on master instead of merging master into this PR?

[szarnekow]

I miss a systematic approach here. In the same file org.eclipse.jface.operation.ModalContext.run(IRunnableWithProgress, boolean, IProgressMonitor, Display) also created an InterruptedException. Why isn't that code path updated?

[HeikoKlare]

That code path was already updated some weeks ago to add the causing exception (the OperationCanceledException) to the InterruptedException. The systematics in this PR is that we went through all the Platform UI code to see where this pattern of misuse of InterruptedException happened as well (and was not "improved" yet). The place touched here is the only remaining one we found.
We already discussed systematic ways of wrapping the OperationCanceledException in this PR, but since we agree that this pattern is just legacy and should not be implemented anywhere else, I agree with @laeubi (#1364 (comment)) that we should not make this pattern more convenient to implement.

[szarnekow]

Oh, my bad. I was looking at a different local version of that file.