[crac] RFR: Fix ordering of invocation on Resources [v3]

[Radim Vansa]

On Tue, 2 May 2023 10:04:21 GMT, Anton Kozlov <akozlov at openjdk.org> wrote:

>> Theoretically, the method could do that. However, here the purpose of `semanticContext()` is to pass the context to which it was registered rather than the subcontext where it is stored (but this is an implementation detail that the resource does not know about).
>
> Just realized that this is required for PriorityContext implementation. But that is the implementation of that class, it's wrong ACI has to care about that.

Yes, it's a bit of enforced flexibility of the base class (through allowing to override a method), though it doesn't need about the use case.

I could use just a list rather than sub-contexts but that would require duplicated code.

>> The contexts are supposed to run all resources regardless of whether any failed, therefore there's no point in propagating and re-wrapping the exceptions. We could have a method to check whether this C/R is 'marked for rollback' (has any exceptions), but I don't see why would the parent context decide on the type - because it would get just CheckpointException with a list of other failures from components it does not understand.
>> 
>> About the distinction: the difference is that if you get a CheckpointException you'd unwrap it, recording only the inner suppressed ones. But I should push that to `recordExceptions` and rather decide based on CheckpointException message than number of suppressions.
>
>> The contexts are supposed to run all resources regardless of whether any failed, therefore there's no point in propagating and re-wrapping the exceptions. ...  I don't see why would the parent context decide on the type - because it would get just CheckpointException with a list of other failures from components it does not understand.
> 
> The parent Context may implement an artbitrary handling (for example, unloading a component compleltely if that is throwing an exception). So throwing an exception is something useful.
> 
> With that, the new Core.recordException is completely new exception flow, that just opimizes somthing the generic throw scheme. With that, the generic schemes should be something good enough already, we don't need to complicate the interface, the code,.. CheckpointExcepotions are still exceptions, that is, we don't expect them often, there is no need to optimize them.

It's not about optimization (in the sense of performance) but about removing code bloat, and the need for parent context to tediously copy failures (deciding whether something was a wrapper exception or the actual failure), when all you need to do in the end is to report them in bulk. It's not a new exception flow, it's removing the flow as there is no exception flow needed.

Your example is invalid: If a throwing resource is to be removed, it's the task of the parent context - and that one will see the exception. The parent context should not remove its child context since one of the N resources in the child context is failing.

-------------

PR Review Comment: https://git.openjdk.org/crac/pull/60#discussion_r1182382341
PR Review Comment: https://git.openjdk.org/crac/pull/60#discussion_r1182379465