Try-catch clause #525

kirsanium · 2020-02-25T10:37:33Z

Feature introducing complex error handling via new StepBuilder's methods Try() and Catch().
This works pretty similar to CompensateWith() and is based off of that, but with no reversion and it allows to catch exceptions by type.
Also introducing WorkflowDefinitionValidator to ensure that there are corresponding catches for every try.

It is needed to exclude cases where try is present and catch is not

danielgerlag

This is a significant and impressive contribution 👍
Left a few comments... I'd also like to pull this branch and play with it a bit before merging.

danielgerlag · 2020-03-01T15:35:28Z

src/WorkflowCore/Interface/ICatchStepBuilder.cs

+
+namespace WorkflowCore.Interface
+{
+    public interface ICatchStepBuilder<TData, TStepBody> : IStepBuilder<TData, TStepBody>, ITryStepBuilder<TData, TStepBody>


Do we need an empty interface?

This interface gives a builder full functionality of IStepBuilder and also allows to Catch() if we want to have multiple catches after one Try(). It is used as a return type for builder after the first Catch(), whereas after Try() we want to allow only to Catch().

danielgerlag · 2020-03-01T15:37:16Z

src/WorkflowCore/Interface/IWorkflowDefinitionValidator.cs

+{
+    public interface IWorkflowDefinitionValidator
+    {
+        bool IsDefinitionValid(WorkflowDefinition definition);


is this part of the try/catch feature? or something else?

Right now this only checks that every Try() has corresponding Catch().

danielgerlag · 2020-03-01T15:38:29Z

src/WorkflowCore/Models/ExecutionPointer.cs

@@ -45,6 +45,8 @@ public class ExecutionPointer
        public object Outcome { get; set; }

        public PointerStatus Status { get; set; } = PointerStatus.Legacy;
+
+        public Exception CurrentException { get; set; }


This class needs to be serializable... not sure how different sub-classes of Exception will behave in that case?

As far as I understood, error handling happens in the same persistence round (not sure if this is how it's called tho) in which the exception occurs. So do we really need to serialize this field?

Why do we need this field on the ExecutionPointer?

@danielgerlag We need this field to build StepExecutionContext so that we can retrieve a thrown exception in Catch() step. An example:

builder.Try(b => b.StartWith(_ => throw new ArgumentException("I am Exception1"))) .Catch(new[] {typeof(Exception)}, ctx => { Console.WriteLine($"Caught an exception: Type: '{ctx.CurrentException.GetType().Name}', Message: '{ctx.CurrentException.Message}'"); return ExecutionResult.Next(); })

Does it really need to be serializable though, or do you have any counter-arguments to what I have said above?

UPD: just used here the same SerializableException class as in WorkflowTerminated

danielgerlag · 2020-03-01T15:39:08Z

src/WorkflowCore/Models/LifeCycleEvents/WorkflowTerminated.cs


 namespace WorkflowCore.Models.LifeCycleEvents
 {
    public class WorkflowTerminated : LifeCycleEvent
    {
+        public Exception Exception { get; set; }


This class also needs to be serializable

Would it be fine if we would put ExecutionError in here instead?

There already is WorkflowError lifecycle event. Also, you could terminate a workflow without an error... not sure this field makes sense?

@danielgerlag
When a workflow doesn't catch an exception thrown in Try clause, it seems logical to me that the workflow has to be terminated, because that means something has gone pretty badly. This can be the case if there is no matching exception type in corresponding Catch clauses. It means WorkflowTerminated event has to be published when this happens. I publish WorkflowError event when an exception happens inside of a Try clause, please, correct me, if I'm doing it wrong. If we want to retrieve information about why workflow has been terminated, we have to preserve an exception in some way. If a workflow was terminated without an error, this field just equals to null.
Nevertheless, if this field needs to be serializable, I'm going to make it of a custom class with fields like Type, Message and StackTrace, because I feel like people might make a good use of the additional information on an unexpected workflow termination.

danielgerlag · 2020-03-01T15:42:34Z

src/WorkflowCore/Services/FluentBuilders/StepBuilder.cs

-    public class StepBuilder<TData, TStepBody> : IStepBuilder<TData, TStepBody>, IContainerStepBuilder<TData, TStepBody, TStepBody>
+    public class StepBuilder<TData, TStepBody> 
+        : IContainerStepBuilder<TData, TStepBody, TStepBody>,
+          ICatchStepBuilder<TData, TStepBody>


Not sure I understand why the class would implement ICatchStepBuilder?

Because we need to return ICatchStepBuilder after Catch().

public class StepBuilder<TData, TStepBody> : IContainerStepBuilder<TData, TStepBody, TStepBody>, ICatchStepBuilder<TData, TStepBody>, IStepBuilder<TData, TStepBody>, ITryStepBuilder<TData, TStepBody>

This would be more verbose, but two last lines are redundant in that case.

src/WorkflowCore/Services/WorkflowRegistry.cs

danielgerlag · 2020-03-01T15:45:23Z

src/WorkflowCore/Services/ErrorHandlers/CatchHandler.cs

+
+namespace WorkflowCore.Services.ErrorHandlers
+{
+    public class CatchHandler : IWorkflowErrorHandler


Could you implement a set of unit tests for this class that make it's behavior explicit?

Sure, will do it soon. By the way I have a question concerning behaviour of CompensateHandler. I don't understand what lines 41-45 of CatchHandler do as I copypasted them from lines 56-60 of CompensateHandler, hence I don't understand if they are needed at all. Could you please elaborate on this?

I believe that was for bubbling up the call stack of nested containers

I clearly have troubles understanding this part of bubbling up the call stack. Could you please go into deeper details? And please tell me if it's needed or not in case of Catchclause.

danielgerlag · 2020-03-07T03:53:25Z

Going to be travelling for a few weeks but I'll try take a deeper look soon.

kirsanium · 2020-04-15T13:11:18Z

Just wanted to say I didn't forget about PR, going to fix the issues you noticed some time soon.

…ons is needed

danielgerlag · 2020-07-11T15:10:13Z

@kirsanium I haven't forgotten about this PR. I will try carve out some time in the next week or so to dive deeper into it but on the surface it looks really good!

knoxi · 2020-09-11T12:15:12Z

@danielgerlag @kirsanium Any news on this? Would be very handy to have this in the product.

kirsanium · 2021-03-04T18:44:02Z

@knoxi waiting for the review from @danielgerlag

knoxi · 2021-03-08T15:13:00Z

@danielgerlag any news on that? It's now open since a year.

knoxi · 2021-11-23T08:28:46Z

@kirsanium
Do you see any chance to update the branch and extend the tests?
It would be really great if the feature will be part of the library!

kirsanium · 2022-06-20T18:20:35Z

Sorry I have no time and interest in this one since too much time have passed - I haven't even coded in c# for some time now...but please please finish this one, it was goddamn ready two years ago

kirsanium and others added 11 commits February 25, 2020 17:25

add try-catch clause (in progress)

e8000c3

update to match master branch state (in progress)

61dfc85

cover more cases in try-catch sample

c49a07b

now correctly builds steps after catch clause

718cb69

add more StepBuilder.Catch() overloads

4e94b89

TryContainer's children revert no more

01d9666

remove TryContainer

3a2fa0d

add TryCatchScenario

338a676

workflow terminates if exception in try block isn't caught

1a213d3

add WorkflowDefinition validation.

ddfa996

It is needed to exclude cases where try is present and catch is not

clean TryCatchSample

edf616e

danielgerlag reviewed Mar 1, 2020

View reviewed changes

danielgerlag mentioned this pull request Mar 7, 2020

How to abort a failing step and have the step status Failed? #531

Open

kirsanium added 3 commits May 2, 2020 17:12

Add CatchHandlerFixture

efdaf17

Make try-catch example more complex

94a8a71

Add SerializableException class to use where serialization of excepti…

fe75379

…ons is needed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try-catch clause #525

Try-catch clause #525

kirsanium commented Feb 25, 2020

danielgerlag left a comment

danielgerlag Mar 1, 2020

kirsanium Mar 1, 2020

danielgerlag Mar 1, 2020

kirsanium Mar 1, 2020 •

edited

Loading

danielgerlag Mar 1, 2020

kirsanium Mar 1, 2020 •

edited

Loading

danielgerlag Mar 7, 2020

kirsanium May 2, 2020 •

edited

Loading

danielgerlag Mar 1, 2020

kirsanium Mar 1, 2020

danielgerlag Mar 7, 2020

kirsanium May 2, 2020

danielgerlag Mar 1, 2020

kirsanium Mar 1, 2020 •

edited

Loading

danielgerlag Mar 1, 2020

kirsanium Mar 1, 2020

danielgerlag Mar 7, 2020

kirsanium May 2, 2020

danielgerlag commented Mar 7, 2020

kirsanium commented Apr 15, 2020

danielgerlag commented Jul 11, 2020

knoxi commented Sep 11, 2020

kirsanium commented Mar 4, 2021

knoxi commented Mar 8, 2021

knoxi commented Nov 23, 2021

kirsanium commented Jun 20, 2022

Try-catch clause #525

Are you sure you want to change the base?

Try-catch clause #525

Conversation

kirsanium commented Feb 25, 2020

danielgerlag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirsanium Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirsanium Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirsanium May 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirsanium Mar 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielgerlag commented Mar 7, 2020

kirsanium commented Apr 15, 2020

danielgerlag commented Jul 11, 2020

knoxi commented Sep 11, 2020

kirsanium commented Mar 4, 2021

knoxi commented Mar 8, 2021

knoxi commented Nov 23, 2021

kirsanium commented Jun 20, 2022

kirsanium Mar 1, 2020 •

edited

Loading

kirsanium Mar 1, 2020 •

edited

Loading

kirsanium May 2, 2020 •

edited

Loading

kirsanium Mar 1, 2020 •

edited

Loading