Community
Participate
Working Groups
an Exception thrown in a Java service called by AQL does not lead to a rollback. At least for the user defined services the safest default behavior would probably be the one where an exception lead to a rollback.
To complete: this means that a tool which uses AQL expressions and services to modify the model may crash in the middle of its execution and leave the semantic model in an intermediate and potentially invalid state.
See https://git.eclipse.org/r/#/c/74188/ for a draft of the required change in the AQL runtime, to be validated on our side that it has the expected effect and no negative side-effects.
*** Bug 496155 has been marked as a duplicate of this bug. ***
Treating this would be good opportunity to clarify our reaction to various kinds of errors during expression evaluation: not just for AQL, and not just for exceptions in services. Depending on the kinds of changes needed to get a more consistent and robust behavior, some parts may need to be defered to the next major release instead of 4.1.
New Gerrit change created: https://git.eclipse.org/r/80798
I realize now after discussing with Pierre-Charles that there is the general problem (this bug) and there is the scenario which bugged me which is IMO slightly more critical which is: 1. I have a property view definition (for instance the reflectively generated ones) leading to a text field. 2. I type "AABAABA" whereas it is not a valid text format for the corresponding datatype => EMF throws an exception => result: AABAABA is still in the textfield, I have to click somewhere else and then go back to see the actual value, I had no feedback whatsoever something went wrong. => expected result: we display the actual model value
New Gerrit change created: https://git.eclipse.org/r/80825
New Gerrit change created: https://git.eclipse.org/r/80827
I've pushed prototype fix for the specific case mentioned in comment #6. At first glance it seems to work, but requires more work to make the code cleaner. In addition: * the currently proposed fix for bug #498748, at https://git.eclipse.org/r/80894, removes the asynchronous part of the tool application, so if we merged it it would impact this one (making it simpler); * as it is, there are probably some race conditions to deal with 1. The user starts editing X => XY 2. We launch the tool, trying to apply "XY" 3. While the tool is running, the using restarts typing, XY => XYZ 4. We finally get the result of the command: it failed. Should we revert to "XY", the last known correct value computed from the model (and lose the user's last edit), or to "XYZ" and re-trigger the tool with that new value.?
I've opened bug #502048 for the specific issue mentioned by Cédric in #c6, which is more critical but properties-specific, and should be fixed for 4.1.0. The more general issue is still present, but would require deeper changes, including probably breaks in the interpreter APIs and in AQL, so I'm keeping this ticket open but moving it to 5.0.
Rewording in more general terms as this in not only about AQL. I'm not sure we can do anything for Query Legacy, MTL (and OCL) at this point except strongly recommending users to switch to AQL, but we must make sure that "service:" works in the same way as AQL (once we've ironed the details), and clearly document the expectations for any custom/future interpreted language.
See https://git.eclipse.org/r/#/c/89213/ for a proposed change in "service:". It's marked as a DRAFT not attached to this ticket because I'm not sure it's the right direction to go. In particular, making a special case for one kind of exception (OperationCancelledException) this deep in the stack seems wrong to me. This kind of decision on how to react to which kind of error would be better handled higher in the call chain, where we have more context. My current feeling is that interpreter implementations should: * abort evaluation as soon as an exception is detected in user-supplied code (Java services of course, but also EMF feature access, which may contain manual code or otherwise trigger exceptions that can be thrown by adapters); * not log anything. It's up to the client code to know if, when and where to log any message; * report the error condition to the caller with as much information as possible about the root cause. We currently use a mix of EvaluationException for older interpreters (whose API did not provide in-band mechanism for error reporting) and rich IEvaluationResult for "modern-style" IInterpreterWithDiagnostic. My preference goes to IEvaluationResult (which has other potential benefits like enforcing consistent type coercions between all interpreters). How to react to an IEvaluationResult corresponding to an error should be handled on the caller side. "Client" code (Sirius code requesting expression evaluation) normally interacts almost exclusively with the ODesignGenericInterpreter associated to the session, so its API could be augmented to support better error handling. Some ideas (open to discussion): * have a version of evaluateExpression() which takes a Setting as an additional argument, to record the where the expression comes from (in the VSM). This is similar to the API exposed by RuntimeLoggerInterpreter, except that it would not conflate context tracking with logging policy. If provided, the Setting would be stored in the IEvaluationResult for the expression, as an additional data point available to decide of the appropiate reaction. * the IEvaluationResult class could have baked-in (but optional) error handling support. For example result.throwOnError() to rethrow any exception which was captured in the IEvaluationResult. This is not incompatible with what I said earlier: it would be up to the client code to decide, depending on its context, if rethrowing is the proper thing to do. In some cases (e.g. when evaluating a semanticCandidatesExpression during a refresh), simply using a default value (e.g. empty set) and maybe logging the error could be the proper reaction. In others, e.g. when evaluating the valueExpression of a SetValue model operation inside a tool, immediatly aborting the tool execution (and rollbacking the whole transaction higher in the stack) would be better than changing the user's model with a value obtained after an explicit error occured.
New Gerrit change created: https://git.eclipse.org/r/95600
New Gerrit change created: https://git.eclipse.org/r/95601
New Gerrit change created: https://git.eclipse.org/r/95599
New Gerrit change created: https://git.eclipse.org/r/95598
New Gerrit change created: https://git.eclipse.org/r/96451
New Gerrit change created: https://git.eclipse.org/r/96450
Gerrit change https://git.eclipse.org/r/95598 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=3e23db43db076083dfa6d4237ff1d9833ed86f56
Gerrit change https://git.eclipse.org/r/95599 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=a73afe69001d953dd0058dbf08c89418e6cd0f44
Gerrit change https://git.eclipse.org/r/95600 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=6d36166e3938b9aa495584bec0f382f705fba269
Gerrit change https://git.eclipse.org/r/96450 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=714d146df15b6b6e3b1696d0cbc0bf5959e0ba7f
Gerrit change https://git.eclipse.org/r/95601 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=055cae502c47e5cd91a9bcde0d298ccfb492e32e
Some internal changes have been made for 5.0, but actually modifying the existing behavior will require some time to tweak the policies (see https://git.eclipse.org/r/#/c/96451/), so moving to 5.1 to finish this.
If we rework the interpreter API, we'll probably need to update all call sites. We'll need a strategy which does not require a "big bang" and allows for progressive update. While we're at it, as we'll pass over all these call sites, we should take this opportunity to improve other aspects of this code, as long as it does not slow down the main issue. One aspects to be aware of is Bug #427813 - Do not depend on Session when only an IInterpreter is needed, but there are probably other (I'll try to reference them here if I see them).
New Gerrit change created: https://git.eclipse.org/r/115315
Gerrit change https://git.eclipse.org/r/115315 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=0166c3c29a3b1d980e169a79129d6aae8a0a38b8
