Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrapped the OperationCanceledException in InterruptedException #967

Closed
wants to merge 0 commits into from

Conversation

amartya4256
Copy link
Contributor

What it does

eclipse-platform/eclipse.platform#853

Wrapped the OperationCanceledException in InterruptedException

How to test

Author checklist

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the changes are unnecessary, there's one of which I'm not sure though. Please see if the exception is reaching the user (UI/log file) and if it's not then revert the changes.

Comment on lines 168 to 171
} catch (OperationCanceledException e1) {
throw new InterruptedException();
InterruptedException interruptedException = new InterruptedException(e1.getLocalizedMessage());
interruptedException.initCause(e1);
throw interruptedException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to do this, remove this catch and add a | OperationCanceledException ("multi-catch") in line 182 (//user canceled)

Comment on lines 285 to 287
InterruptedException interruptedException = new InterruptedException(e.getLocalizedMessage());
interruptedException.initCause(e);
throw interruptedException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this one: do you see any call where this exception would reach the UI or any log?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used by the JavaProjectWizard which is provided by jdt.ui.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one is propagated to the user, I have traced the calls and seems like the methods are called in try-finally blocks in atleast one path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good. Thank you!

Comment on lines 326 to 328
throw new InterruptedException();
InterruptedException interruptedException = new InterruptedException(e2.getLocalizedMessage());
interruptedException.initCause(e2);
throw interruptedException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is only used in tests, no need to add the info here.

Comment on lines 167 to 169
InterruptedException interruptedException = new InterruptedException(e.getLocalizedMessage());
interruptedException.initCause(e);
throw interruptedException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: this one is being caught a few lines below so you can simply let it through and change the catch-clause below and make it a multi-catch:

catch  (InterruptedException | OperationCanceledException e) {
	// cancel pressed
}

@fedejeanne
Copy link
Contributor

@amartya4256 did you close this one by accident?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants