From a9800223514874d0aece504b871a2d3fc40b8e97 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 20 Jan 2025 13:58:11 +0100 Subject: [PATCH] Refactor exception handling and activity status logic Refactored `ExceptionHandlingMiddleware` to improve readability and modularity by splitting responsibilities into smaller methods. Replaced `GetAggregateStatus` with direct use of `source.Status` for significant performance improvement when a large number of activity instances are involved. Updated null-checks for clarity and fixed inconsistent usage of default values. --- .../Activities/ExceptionHandlingMiddleware.cs | 63 ++++++++++--------- .../DefaultActivityExecutionMapper.cs | 19 ++---- 2 files changed, 37 insertions(+), 45 deletions(-) diff --git a/src/modules/Elsa.Workflows.Core/Middleware/Activities/ExceptionHandlingMiddleware.cs b/src/modules/Elsa.Workflows.Core/Middleware/Activities/ExceptionHandlingMiddleware.cs index b26210782..38d5963a2 100644 --- a/src/modules/Elsa.Workflows.Core/Middleware/Activities/ExceptionHandlingMiddleware.cs +++ b/src/modules/Elsa.Workflows.Core/Middleware/Activities/ExceptionHandlingMiddleware.cs @@ -1,4 +1,5 @@ using Elsa.Common; +using Elsa.Extensions; using Elsa.Workflows.Models; using Elsa.Workflows.Pipelines.ActivityExecution; using Elsa.Workflows.State; @@ -20,45 +21,47 @@ public static class ExceptionHandlingMiddlewareExtensions /// /// Catches any exceptions thrown by downstream components and transitions the workflow into the faulted state. /// -public class ExceptionHandlingMiddleware : IActivityExecutionMiddleware +public class ExceptionHandlingMiddleware(ActivityMiddlewareDelegate next, IIncidentStrategyResolver incidentStrategyResolver, ISystemClock systemClock, ILogger logger) + : IActivityExecutionMiddleware { - private readonly ActivityMiddlewareDelegate _next; - private readonly IIncidentStrategyResolver _incidentStrategyResolver; - private readonly ISystemClock _systemClock; - private readonly ILogger _logger; - - /// - /// Constructor. - /// - public ExceptionHandlingMiddleware(ActivityMiddlewareDelegate next, IIncidentStrategyResolver incidentStrategyResolver, ISystemClock systemClock, ILogger logger) - { - _next = next; - _incidentStrategyResolver = incidentStrategyResolver; - _systemClock = systemClock; - _logger = logger; - } - /// public async ValueTask InvokeAsync(ActivityExecutionContext context) { try { - await _next(context); + await next(context); } catch (Exception e) { - _logger.LogWarning(e, "An exception was caught from a downstream middleware component"); - context.Exception = e; - context.TransitionTo(ActivityStatus.Faulted); - - var activity = context.Activity; - var exceptionState = ExceptionState.FromException(e); - var now = _systemClock.UtcNow; - var incident = new ActivityIncident(activity.Id, activity.Type, e.Message, exceptionState, now); - context.WorkflowExecutionContext.Incidents.Add(incident); - - var strategy = await _incidentStrategyResolver.ResolveStrategyAsync(context); - strategy.HandleIncident(context); + logger.LogWarning(e, "An exception was caught from a downstream middleware component"); + LogExceptionAndTransition(context, e); + FaultAncestors(context); + await HandleIncidentAsync(context); } } + + private void LogExceptionAndTransition(ActivityExecutionContext context, Exception e) + { + context.Exception = e; + context.TransitionTo(ActivityStatus.Faulted); + var activity = context.Activity; + var exceptionState = ExceptionState.FromException(e); + var now = systemClock.UtcNow; + var incident = new ActivityIncident(activity.Id, activity.Type, e.Message, exceptionState, now); + context.WorkflowExecutionContext.Incidents.Add(incident); + } + + private async Task HandleIncidentAsync(ActivityExecutionContext context) + { + var strategy = await incidentStrategyResolver.ResolveStrategyAsync(context); + strategy.HandleIncident(context); + } + + private static void FaultAncestors(ActivityExecutionContext context) + { + var ancestors = context.GetAncestors(); + + foreach (var ancestor in ancestors) + ancestor.TransitionTo(ActivityStatus.Faulted); + } } \ No newline at end of file diff --git a/src/modules/Elsa.Workflows.Runtime/Services/DefaultActivityExecutionMapper.cs b/src/modules/Elsa.Workflows.Runtime/Services/DefaultActivityExecutionMapper.cs index 0e1dd85de..7278e4fa1 100644 --- a/src/modules/Elsa.Workflows.Runtime/Services/DefaultActivityExecutionMapper.cs +++ b/src/modules/Elsa.Workflows.Runtime/Services/DefaultActivityExecutionMapper.cs @@ -115,7 +115,7 @@ public class DefaultActivityExecutionMapper : IActivityExecutionMapper ActivityTypeVersion = source.Activity.Version, StartedAt = source.StartedAt, HasBookmarks = source.Bookmarks.Any(), - Status = GetAggregateStatus(source), + Status = source.Status, CompletedAt = source.CompletedAt }; } @@ -220,20 +220,9 @@ public class DefaultActivityExecutionMapper : IActivityExecutionMapper } } - private static ActivityStatus GetAggregateStatus(ActivityExecutionContext context) - { - // If any child activity is faulted, the aggregate status is faulted. - var descendantContexts = context.GetDescendants().ToList(); - - if (descendantContexts.Any(x => x.Status == ActivityStatus.Faulted)) - return ActivityStatus.Faulted; - - return context.Status; - } - private static IDictionary GetPayload(ActivityExecutionContext source) { - var outcomes = source.JournalData.TryGetValue("Outcomes", out var resultValue) ? resultValue as string[] : default; + var outcomes = source.JournalData.TryGetValue("Outcomes", out var resultValue) ? resultValue as string[] : null; var payload = new Dictionary(); if (outcomes != null) @@ -256,13 +245,13 @@ public class DefaultActivityExecutionMapper : IActivityExecutionMapper var cachedValue = activity.GetOutput(expressionExecutionContext, x.Name); - if (cachedValue != default) + if (cachedValue != null) return cachedValue; if (x.ValueGetter(activity) is Output output && source.TryGet(output.MemoryBlockReference(), out var outputValue)) return outputValue; - return default; + return null; }); return outputs;