Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/modules/Elsa.Workflows.Core/Activities/Catch.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
using System.Runtime.CompilerServices;
using Elsa.Workflows.Attributes;
using Elsa.Workflows.Models;
using Elsa.Workflows.UIHints;

namespace Elsa.Workflows.Activities;

/// <summary>
/// Catch the Exception.
/// </summary>
[Activity("Elsa", "Primitives", "Catch the Exception.")]
public class Catch : Activity
{
/// <inheritdoc />
public Catch([CallerFilePath] string? source = default, [CallerLineNumber] int? line = default) : base(source, line)
{
}

/// <summary>
/// Catch all the Exception.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// </summary>
[Input(Description = "A value that is the tag a global catcher")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.")]

public Input<bool> CatchAll { get; set; } = default!;

/// <summary>
/// Catch the Activities.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// </summary>
[Input(Description = "The Activities will be catch",
UIHint = InputUIHints.MultiText)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)]

public Input<ICollection<string>?> CatchActivities { get; set; } = default!;

/// <inheritdoc />
protected override ValueTask ExecuteAsync(ActivityExecutionContext context)
{
return context.CompleteActivityAsync();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ public Flowchart([CallerFilePath] string? source = default, [CallerLineNumber] i
OnSignalReceived<ScheduleActivityOutcomes>(OnScheduleOutcomesAsync);
OnSignalReceived<ScheduleChildActivity>(OnScheduleChildActivityAsync);
OnSignalReceived<CancelSignal>(OnActivityCanceledAsync);
OnSignalCaptured<ExceptionSignal>(OnActivityExceptionAsync);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/// <summary>
Expand Down Expand Up @@ -114,7 +115,7 @@ protected override async ValueTask ScheduleChildrenAsync(ActivityExecutionContex

// If no start activity found, return the first activity.
logger.LogDebug("No start activity found. Using the first activity");
return Activities.FirstOrDefault();
return Activities.Where(activity => activity is not Catch).FirstOrDefault();
}

/// <summary>
Expand Down Expand Up @@ -325,4 +326,55 @@ private async ValueTask OnActivityCanceledAsync(CancelSignal signal, SignalConte
{
await CompleteIfNoPendingWorkAsync(context.ReceiverActivityExecutionContext);
}

private async ValueTask OnActivityExceptionAsync(ExceptionSignal signal, SignalContext context)
{
var avtivities = Activities.Where(a => a is Catch).Select(a => a as Catch).ToList();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var avtivities = Activities.Where(a => a is Catch).Select(a => a as Catch).ToList();
var activities = Activities.Where(a => a is Catch).Cast<Catch>().ToList();


if (avtivities.Count == 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (avtivities.Count == 0)
if (activities.Count == 0)

return;

var flowchartContext = context.ReceiverActivityExecutionContext;
var workflowExecutionContext = flowchartContext.WorkflowExecutionContext;

var exceptionActivity = signal.Activity;

var options = new ActivityInvocationOptions
{
Owner = flowchartContext
};

foreach (var catchAvtivity in avtivities)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
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);
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
using Elsa.Common.Contracts;
using Elsa.Extensions;
using Elsa.Workflows.Contracts;
using Elsa.Workflows.Models;
using Elsa.Workflows.Pipelines.ActivityExecution;
using Elsa.Workflows.Signals;
using Elsa.Workflows.State;
using Microsoft.Extensions.Logging;

Expand Down Expand Up @@ -60,6 +62,8 @@ public async ValueTask InvokeAsync(ActivityExecutionContext context)

var strategy = await _incidentStrategyResolver.ResolveStrategyAsync(context);
strategy.HandleIncident(context);

await context.SendSignalAsync(new ExceptionSignal(activity, e));
Comment on lines 64 to +66
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}
10 changes: 10 additions & 0 deletions src/modules/Elsa.Workflows.Core/Signals/ExceptionSignal.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
using Elsa.Workflows.Contracts;

namespace Elsa.Workflows.Signals;

/// <summary>
/// /// Sent by ExceptionHandlingMiddleware to notify their composite container that it has exception.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// </summary>
/// <param name="Activity">The exception activity</param>
/// <param name="Exception">The exception</param>
public record ExceptionSignal(IActivity Activity, Exception Exception);