Bug 461764

Comment 1 Ed Willink 2015-03-09 18:07:27 EDT

Surely Xtext should absorb an OperationCanceledException in a ValidationJob?

Comment 2 Sebastian Zarnekow 2015-03-09 18:10:48 EDT

It's up to the client of the CancelableDiagnostician to handle OperationCancelledExceptions. As I figure from the stacktrace, OCL uses its own resource validator so that one should handle it.

Background: The IResourceValidator is also used on platforms (e.g. headless, other IDEs) that do know nothing about OperationCancelledExceptions (it's not even on the classpath in these cases). Therefore the validator may not expose it to its clients.

Comment 3 Ed Willink 2015-03-09 18:34:38 EDT

(In reply to Sebastian Zarnekow from comment #2)
> It's up to the client of the CancelableDiagnostician to handle
> OperationCancelledExceptions.

PivotResourceValidator extends ResourceValidatorImpl which has changed significantly in Xtext 2.8 through amongst other things the introduction of an OperationCanceledManager, but since that is new in 2.8.0 we don;t have the option to exploit it.

Need to review the overload of the old ResourceValidatorImpl  and see what is appropriate now.

Comment 4 Ed Willink 2015-03-09 18:50:42 EDT

This is an Xtext 2.8.0 regression.

The OCL PivotResourceValidator is based on Xtext 2.4..2.7 ResourceValidatorImpl for which CancelableDiagnostician

doValidateContents(EObject eObject, DiagnosticChain diagnostics, Map<Object, Object> context) {
		if (isCanceled(context))
			return true;
		return super.doValidateContents(eObject, diagnostics, context);
	}

did not throw a RuntimeException for a cancel.

Now in Xtext 2.8

	@Override
	protected boolean doValidateContents(EObject eObject, DiagnosticChain diagnostics, Map<Object, Object> context) {
		if (isCanceled(context))
			throw new OperationCanceledException();
		return super.doValidateContents(eObject, diagnostics, context);
	}

does throw an exception so at the end of 'ResourceValidator'.doValidate

		} catch (RuntimeException e) {
			log.error(e.getMessage(), e);
		}
		if (monitor.isCanceled())
			return null;


logs the exception rather than handling the monitor cancel.

Comment 5 Sebastian Zarnekow 2015-03-10 09:46:21 EDT

Thanks for the analysis.

Comment 6 Sebastian Zarnekow 2015-03-18 15:21:21 EDT

(In reply to Ed Willink from comment #4)
> This is an Xtext 2.8.0 regression.
> 

You're definitely right that we changed the contract of the method. It now throws an OperationCancelledError that is a wrapper around OperationCancelledException without osgi dep. That type was not present in earlier versions.

But since you override the method that wraps the OperationCancelledException, you could insert a try-catch around the call to diagnostician.validate and handle the OperationCancelledException there, e.g. return an empty List. Would that be an option for you?

Comment 7 Ed Willink 2015-03-19 05:58:01 EDT

(In reply to Sebastian Zarnekow from comment #6)
> Would that be an option for you?

I had to work quite hard to create a repro in which the validate job was slow enough to allow a cancel from the progress view. Repro possible using /org.eclipse.qvtd.examples.qvtrelation.reltocore/qvtrsrc/RelToCore.qvtr after cutting and pasting to increase line count five-fold. Then just context->Validate, click cancel in Progress view as quickly as possible.

Once reproduced, all that happens is one unnecessary console log entry.

So this is clearly not the biggest problem that Eclipse or Xtext faces.

I'm sure I can workaround; no choice since 2.8.0 cannot change and a 2.8.0a cannot be enforced on users. However I think that 2.8.1 should endeavor to restore the accidentally broken API contract.

Comment 8 Sebastian Zarnekow 2015-03-19 06:03:43 EDT

(In reply to Ed Willink from comment #7)
> (In reply to Sebastian Zarnekow from comment #6)

> I'm sure I can workaround; no choice since 2.8.0 cannot change and a 2.8.0a
> cannot be enforced on users. However I think that 2.8.1 should endeavor to
> restore the accidentally broken API contract.

I'm afraid that is not possible without great effort on our site. Background: We are working on cross platform support for Xtext languages, e.g. they should be supported not only in Eclipse but in IntelliJ or the web, too. Therefore we put some effort into proper platform neutral propagation of cancel events. Thus the error that is thrown now. All you would have to do in your code is to change the PivotResourceValidator.validate(..) and add a try catch around the call to the CancelableDiagnostician to get hold on the thrown OperationCancelledException and return an empty list as it was done prior to 2.8. We cannot do that, though since in IntelliJ it is not "allowed" to catch a ProcessCancelledException (their equivalent). For OCL it should work. What do you think?

Comment 9 Ed Willink 2015-03-19 07:36:35 EDT

(In reply to Sebastian Zarnekow from comment #8)
> (In reply to Ed Willink from comment #7)
> > (In reply to Sebastian Zarnekow from comment #6)
> 
> > I'm sure I can workaround; no choice since 2.8.0 cannot change and a 2.8.0a
> > cannot be enforced on users. However I think that 2.8.1 should endeavor to
> > restore the accidentally broken API contract.
> 
> I'm afraid that is not possible without great effort on our site.

I don't understand why generic support requires a return-false contract to change to a throw; the reverse might have been necessary. Your 2.8.0 release review states that you have Eclipse API quality. IMHO, that means you must endeavour to restore the API contract unless you move to 3.0.

Comment 10 Sven Efftinge 2015-03-19 08:00:08 EDT

(In reply to Ed Willink from comment #9)
> (In reply to Sebastian Zarnekow from comment #8)
> > (In reply to Ed Willink from comment #7)
> > > (In reply to Sebastian Zarnekow from comment #6)
> > 
> > > I'm sure I can workaround; no choice since 2.8.0 cannot change and a 2.8.0a
> > > cannot be enforced on users. However I think that 2.8.1 should endeavor to
> > > restore the accidentally broken API contract.
> > 
> > I'm afraid that is not possible without great effort on our site.
> 
> I don't understand why generic support requires a return-false contract to
> change to a throw; the reverse might have been necessary.

It's important to signal the callers that the process was canceled. Just returning early is not the same.

> Your 2.8.0 release
> review states that you have Eclipse API quality. IMHO, that means you must
> endeavour to restore the API contract unless you move to 3.0.

I don't think it helps much to come up with formalisms, but just out of curiosity, where is it stated that this breaks "Eclipse Quality API" contract?

Comment 11 Ed Willink 2015-03-20 14:03:59 EDT

(In reply to Sven Efftinge from comment #10)
> It's important to signal the callers that the process was canceled. Just
> returning early is not the same.

That's what monitor.isCanceled() does. I just needed an if to suppress the spurious message. Surely you just need similar if's to distinguish an early return from a normal return.

> I don't think it helps much to come up with formalisms, but just out of
> curiosity, where is it stated that this breaks "Eclipse Quality API"
> contract?

Hardly a formalism. It seems common sense to me that if APIs are stable they do not change.

Comment 12 Sebastian Zarnekow 2015-03-21 06:55:20 EDT

(In reply to Ed Willink from comment #11)
> That's what monitor.isCanceled() does.

I'm afraid your conclusion is based on wrong assumptions. Other platforms, e.g. IntelliJ IDEA will raise a ProcessCancelledException from framework code that may not be caught or suppressed. Thus we are not allowed to handle it differently than propagating it. 

Further analysis reveals that we throw an OperationCancelledError from #vaidate which is not caught / handled / unwrapped in ValidationJob.createIssues(IProgressMonitor) or ValidationJob.run(..), though. We may have to use propagateIfCancelException instead of propagateAsErrorIfCancelException

Comment 13 Sebastian Zarnekow 2015-03-23 04:20:28 EDT

PR https://github.com/eclipse/xtext/pull/70

Comment 14 Sebastian Zarnekow 2015-03-23 06:16:45 EDT

Merged into master and maintenance.