Catch the exception and handling the exception in the workflow #5422

fanslead · 2024-05-22T03:38:35Z

Implemented a catch activity to catch the exception in the flowchart workflow.
When the flowchart has a catch activity, it will catch the exception and can excute the custom exception handling workflow.

This change is

fanslead · 2024-05-22T03:40:12Z

@dotnet-policy-service agree

fanslead · 2024-05-30T03:59:24Z

@sfmskywalker any suggestion for this pr?

sfmskywalker · 2024-05-30T05:38:40Z

Hi @fanslead , thank you for your PR. I will need some time to think about this approach and to explore your PR in detail. Thank you for your patience.

fanslead · 2024-06-04T03:02:51Z

Hi @fanslead , thank you for your PR. I will need some time to think about this approach and to explore your PR in detail. Thank you for your patience.

@sfmskywalker I think this will be a very practical, commonly used and flexible feature. For example, in the event of an abnormality, catch activity can execute custom event or http notifications or compensation measures without any coding.

fanslead · 2024-06-18T02:04:26Z

@sfmskywalker Hi, Do you have any thoughts about this PR?

sfmskywalker · 2024-06-18T07:56:56Z

Hi @fanslead , I'm afraid I haven't had the chance yet to sit down and look through the PR and think about the proposed feature, but generally I think it makes sense. I can't say when I might be able to circle back to this, but please rest assured that your PR will be reviewed and that you'll get proper feedback. Thank you for your patience.

fanslead · 2024-08-13T08:48:31Z

@sfmskywalker any update for this pr?

sfmskywalker

All in all, I can see that the Catch activity could be interesting for handling exceptions centrally.

However, I think that we could do something nicer that is similar to BPMN's boundary event mechanisms.

Although it definitely doesn't have to be the same, I think that we might do the following:

Introduce a new activity called Try
The Try activity exposes two outcomes:
1. Done
2. Catch
The Try activity has one embedded port called Body and is of type IActivity - effectively making this a composite activity.
Similar what you did with Flowchart, the Try activity would listen for exception signals.
When an exception signal is received, it completes with the Catch outcome.
Otherwise, it completes with the Done outcome.

This way, we have a kind of a "boundary event" in the form of the "Catch" outcome on the Try activity, achieving the same centrally managed exception handling but without having to update the Flowchart activity.

What do you think?

sfmskywalker · 2024-08-13T19:10:22Z

src/modules/Elsa.Workflows.Core/Activities/Flowchart/Activities/Flowchart.cs

@@ -28,6 +28,7 @@ public Flowchart([CallerFilePath] string? source = default, [CallerLineNumber] i
        OnSignalReceived<ScheduleActivityOutcomes>(OnScheduleOutcomesAsync);
        OnSignalReceived<ScheduleChildActivity>(OnScheduleChildActivityAsync);
        OnSignalReceived<CancelSignal>(OnActivityCanceledAsync);
+        OnSignalCaptured<ExceptionSignal>(OnActivityExceptionAsync);


I believe I had removed the "capturing" phase - do we really need it for this?

sfmskywalker · 2024-08-13T19:12:53Z

src/modules/Elsa.Workflows.Core/Activities/Catch.cs

+    }
+
+    /// <summary>
+    /// Catch all the Exception.


Suggested change

/// Catch all the Exception.

/// A value indicating whether to catch all exceptions from all activities.

sfmskywalker · 2024-08-13T19:13:30Z

src/modules/Elsa.Workflows.Core/Activities/Catch.cs

+    /// <summary>
+    /// Catch all the Exception.
+    /// </summary>
+    [Input(Description = "A value that is the tag a global catcher")]


Suggested change

[Input(Description = "A value that is the tag a global catcher")]

[Input(Description = "A value indicating whether to catch all exceptions from all activities.")]

sfmskywalker · 2024-08-13T19:14:48Z

src/modules/Elsa.Workflows.Core/Activities/Catch.cs

+    [Input(Description = "The Activities will be catch",
+        UIHint = InputUIHints.MultiText)]


Suggested change

[Input(Description = "The Activities will be catch",

UIHint = InputUIHints.MultiText)]

[Input(

Description = "The IDs of the activities to catch exceptions from.",

UIHint = InputUIHints.MultiText)]

sfmskywalker · 2024-08-13T19:15:11Z

src/modules/Elsa.Workflows.Core/Activities/Catch.cs

+    public Input<bool> CatchAll { get; set; } = default!;
+
+    /// <summary>
+    /// Catch the Activities.


Suggested change

/// Catch the Activities.

/// The IDs of the activities to catch exceptions from.

sfmskywalker · 2024-08-13T19:16:42Z

src/modules/Elsa.Workflows.Core/Activities/Flowchart/Activities/Flowchart.cs

+    {
+        var avtivities = Activities.Where(a => a is Catch).Select(a => a as Catch).ToList();
+
+        if (avtivities.Count == 0)


Suggested change

if (avtivities.Count == 0)

if (activities.Count == 0)

sfmskywalker · 2024-08-13T19:17:12Z

src/modules/Elsa.Workflows.Core/Activities/Flowchart/Activities/Flowchart.cs

+            Owner = flowchartContext
+        };
+
+        foreach (var catchAvtivity in avtivities)


Suggested change

foreach (var catchAvtivity in avtivities)

foreach (var catchActivity in activities)

sfmskywalker · 2024-08-13T19:28:33Z

src/modules/Elsa.Workflows.Core/Activities/Flowchart/Activities/Flowchart.cs

+        {
+            var catchContext = workflowExecutionContext.ActivityExecutionContexts.FirstOrDefault(x => x.Activity.Id == catchAvtivity!.Id);
+
+            if (catchContext == null)
+            {
+                catchContext = workflowExecutionContext.CreateActivityExecutionContext(catchAvtivity!, options);
+                workflowExecutionContext.AddActivityExecutionContext(catchContext);
+
+                await catchContext.EvaluateInputPropertiesAsync();
+            }
+
+            var scheduleWorkOptions = new ScheduleWorkOptions
+            {
+                CompletionCallback = OnChildCompletedAsync,
+                ExistingActivityExecutionContext = catchContext
+            };
+
+            var catchAll = catchAvtivity!.CatchAll.Get(catchContext);
+            if (catchAll)
+            {
+                var connections = Connections.Descendants(catchAvtivity).ToList();
+                if (!connections.Any(x => x.Target.Activity == exceptionActivity || x.Source.Activity == exceptionActivity))
+                    await flowchartContext.ScheduleActivityAsync(catchAvtivity, scheduleWorkOptions);
+            }
+            else
+            {
+                var catchActivities = catchAvtivity.CatchActivities?.Get(catchContext);
+                if (catchActivities != null && catchActivities.Contains(exceptionActivity.Id))
+                    await flowchartContext.ScheduleActivityAsync(catchAvtivity, scheduleWorkOptions);
+            }
+        }
+    }


I'm not entirely sure about this approach. Although I understand that you have to evaluate the properties of the Catch activity in order to see if the faulting activity ID is present in its list and whether the CatchAll is set to true, I would think that, after it is determined that the CatchAll activity should execute, you should simply schedule the catch activity for execution. I.e, I think that if either catchAll is true or the faulting activity is present in the "catch activities" list, you should schedule the catch activity.

E.g.

Suggested change

{

var catchContext = workflowExecutionContext.ActivityExecutionContexts.FirstOrDefault(x => x.Activity.Id == catchAvtivity!.Id);

if (catchContext == null)

{

catchContext = workflowExecutionContext.CreateActivityExecutionContext(catchAvtivity!, options);

workflowExecutionContext.AddActivityExecutionContext(catchContext);

await catchContext.EvaluateInputPropertiesAsync();

}

var scheduleWorkOptions = new ScheduleWorkOptions

{

CompletionCallback = OnChildCompletedAsync,

ExistingActivityExecutionContext = catchContext

};

var catchAll = catchAvtivity!.CatchAll.Get(catchContext);

if (catchAll)

{

var connections = Connections.Descendants(catchAvtivity).ToList();

if (!connections.Any(x => x.Target.Activity == exceptionActivity || x.Source.Activity == exceptionActivity))

await flowchartContext.ScheduleActivityAsync(catchAvtivity, scheduleWorkOptions);

}

else

{

var catchActivities = catchAvtivity.CatchActivities?.Get(catchContext);

if (catchActivities != null && catchActivities.Contains(exceptionActivity.Id))

await flowchartContext.ScheduleActivityAsync(catchAvtivity, scheduleWorkOptions);

}

}

}

{

var catchContext = workflowExecutionContext.ActivityExecutionContexts.FirstOrDefault(x => x.Activity.Id == catchAvtivity!.Id);

if (catchContext == null)

{

catchContext = workflowExecutionContext.CreateActivityExecutionContext(catchAvtivity!, options);

workflowExecutionContext.AddActivityExecutionContext(catchContext);

await catchContext.EvaluateInputPropertiesAsync();

}

var scheduleWorkOptions = new ScheduleWorkOptions

{

CompletionCallback = OnChildCompletedAsync,

ExistingActivityExecutionContext = catchContext

};

var catchAll = catchActivity!.CatchAll.Get(catchContext);

var catchActivities = catchActivity.CatchActivities?.Get(catchContext);

var catchFaultingActivity = catchActivities?.Contains(exceptionActivity.Id) == true;

var shouldCatch = catchAll || catchFaultingActivity;

if(shouldCatch)

await flowchartContext.ScheduleActivityAsync(catchActivity, scheduleWorkOptions);

}

}

Please note that I haven't tested it, so I could be totally off here.

When catchAll is true, we need to consider whether abnormal activities are executed after catch activities. Otherwise, an infinite loop may occur.

sfmskywalker · 2024-08-13T19:29:56Z

src/modules/Elsa.Workflows.Core/Middleware/Activities/ExceptionHandlingMiddleware.cs

            strategy.HandleIncident(context);
+
+            await context.SendSignalAsync(new ExceptionSignal(activity, e));


I'm questioning whether we should report an incident if an exception was handled. What are your thoughts on that?

This is a good question. I think if the user chooses Continue With Incidents Strategy, does he still need to add Catch Activity.If Catch Activity is not added, there will be no impact. If it is added, it means he needs this report.

sfmskywalker · 2024-08-13T19:30:18Z

src/modules/Elsa.Workflows.Core/Signals/ExceptionSignal.cs

+namespace Elsa.Workflows.Signals;
+
+/// <summary>
+/// /// Sent by ExceptionHandlingMiddleware to notify their composite container that it has exception.


Suggested change

/// /// Sent by ExceptionHandlingMiddleware to notify their composite container that it has exception.

/// Sent by ExceptionHandlingMiddleware to notify their composite container that an exception occurred.

fanslead · 2024-08-14T03:17:23Z

All in all, I can see that the Catch activity could be interesting for handling exceptions centrally.

However, I think that we could do something nicer that is similar to BPMN's boundary event mechanisms.

Although it definitely doesn't have to be the same, I think that we might do the following:

Introduce a new activity called Try

The Try activity exposes two outcomes:

Done

Catch

The Try activity has one embedded port called Body and is of type IActivity - effectively making this a composite activity.

Similar what you did with Flowchart, the Try activity would listen for exception signals.

When an exception signal is received, it completes with the Catch outcome.

Otherwise, it completes with the Done outcome.

This way, we have a kind of a "boundary event" in the form of the "Catch" outcome on the Try activity, achieving the same centrally managed exception handling but without having to update the Flowchart activity.

What do you think?

@sfmskywalker that is a good idea. But I think composite activity will to some extent affect the readability of the flow. In my eyes, Catch activities should be detached from the main workflow and receive the signal from the main workflow. That is more like to BPMN's boundary event mechanisms. Of course, we can implement both options and leave it to users to choose and use.Just like the Switch Activity and the Switch(flow) Activity.

…core into feature/catchflow * 'feature/catchflow' of https://github.com/fanslead/elsa-core: (200 commits) [Fix] Replace "bundles" with "apps" in the content of README.md to align with current source code structure. (elsa-workflows#5885) Rename Model to ServiceProfile across the codebase. Add TenantId to workflow definition and skip multitenancy test Fix background activity completion (elsa-workflows#5882) Update persistence feature base classes to include self-type Refactor journal data access and improve byte[] handling (elsa-workflows#5878) Changed RegisterClassMap to TryRegisterClassMap on SerializedKeyValuePair classmap registration (elsa-workflows#5877) Add Orchard Core Integration + Agents Module (elsa-workflows#5871) Add customizable DB exception handlers Refactor workflow execution and pipeline handling Update elsa-v3-avatar.png Add new Elsa v3 avatar to design and update README Add MyEvent activity to Elsa services Add TODO Fix logger type Update ElsaStudioVersion to 3.2.0-rc4.473 Fix incorrect serializer reference Update pre-release version in GitHub Actions workflow Rework Workflow Context Feature (elsa-workflows#5861) Add engine exception handling middleware ...

…nContextAsync

feat: catch flow

488b3e9

sfmskywalker reviewed Aug 13, 2024

View reviewed changes

fanslead and others added 6 commits August 14, 2024 12:12

Merge branch 'main' into feature/catchflow

35bdeb4

feat: catch flow

81c2dcb

feat: use Cast<Catch>

8a5882c

feat: update CreateActivityExecutionContext to CreateActivityExecutio…

5e9767b

…nContextAsync

Merge branch 'main' into feature/catchflow

3050f6b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch the exception and handling the exception in the workflow #5422

Catch the exception and handling the exception in the workflow #5422

fanslead commented May 22, 2024 •

edited by sfmskywalker

Loading

fanslead commented May 22, 2024

fanslead commented May 30, 2024

sfmskywalker commented May 30, 2024

fanslead commented Jun 4, 2024 •

edited

Loading

fanslead commented Jun 18, 2024

sfmskywalker commented Jun 18, 2024

fanslead commented Aug 13, 2024

sfmskywalker left a comment

sfmskywalker Aug 13, 2024

sfmskywalker Aug 13, 2024

sfmskywalker Aug 13, 2024

sfmskywalker Aug 13, 2024

sfmskywalker Aug 13, 2024

sfmskywalker Aug 13, 2024

sfmskywalker Aug 13, 2024

sfmskywalker Aug 13, 2024

fanslead Aug 14, 2024

sfmskywalker Aug 13, 2024

fanslead Aug 14, 2024

sfmskywalker Aug 13, 2024

fanslead commented Aug 14, 2024 •

edited

Loading

	/// Catch all the Exception.
	/// A value indicating whether to catch all exceptions from all activities.

	[Input(Description = "A value that is the tag a global catcher")]
	[Input(Description = "A value indicating whether to catch all exceptions from all activities.")]

		[Input(Description = "The Activities will be catch",
		UIHint = InputUIHints.MultiText)]

	/// Catch the Activities.
	/// The IDs of the activities to catch exceptions from.

	foreach (var catchAvtivity in avtivities)
	foreach (var catchActivity in activities)

		strategy.HandleIncident(context);

		await context.SendSignalAsync(new ExceptionSignal(activity, e));

	/// /// Sent by ExceptionHandlingMiddleware to notify their composite container that it has exception.
	/// Sent by ExceptionHandlingMiddleware to notify their composite container that an exception occurred.

Catch the exception and handling the exception in the workflow #5422

Are you sure you want to change the base?

Catch the exception and handling the exception in the workflow #5422

Conversation

fanslead commented May 22, 2024 • edited by sfmskywalker Loading

fanslead commented May 22, 2024

fanslead commented May 30, 2024

sfmskywalker commented May 30, 2024

fanslead commented Jun 4, 2024 • edited Loading

fanslead commented Jun 18, 2024

sfmskywalker commented Jun 18, 2024

fanslead commented Aug 13, 2024

sfmskywalker 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanslead commented Aug 14, 2024 • edited Loading

fanslead commented May 22, 2024 •

edited by sfmskywalker

Loading

fanslead commented Jun 4, 2024 •

edited

Loading

fanslead commented Aug 14, 2024 •

edited

Loading