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

Add engine exception handling middleware #5860

Merged
merged 3 commits into from
Aug 7, 2024
Merged

Conversation

raymonddenhaan
Copy link
Contributor

@raymonddenhaan raymonddenhaan commented Aug 6, 2024

Integrate new EngineExceptionHandlingMiddleware into the workflow execution pipeline to catch and log exceptions outside of running workflow activity.


This change is Reviewable

Integrate new EngineExceptionHandlingMiddleware into the workflow execution pipeline to catch and log exceptions outside of running workflow activity.
@@ -670,7 +530,7 @@ subStatus switch
_ => throw new ArgumentOutOfRangeException(nameof(subStatus), subStatus, null)
};

private bool ValidateStatusTransition(WorkflowSubStatus targetSubStatus)
private bool ValidateStatusTransition()
{
var currentMainStatus = GetMainStatus(SubStatus);
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the wrong fix and instead we should change this line, to:

if (!ValidateStatusTransition(subStatus))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not make a fix here, just removed code that was not used. The validate method is not so much checking if the new state can be moved to as it is checking if the current state is finished and therefor a state that can be moved from. Sending the target sub status and using it instead of the current SubStatus will prevent us from moving to the Finished state.

Copy link
Member

Choose a reason for hiding this comment

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

I understand. I'm just wondering if the original code is a mistake and whether it should have been using the parameter instead of the property. I'll add a TODO to follow up.


internal void TransitionTo(WorkflowSubStatus subStatus)
{
if (!ValidateStatusTransition(SubStatus))
if (!ValidateStatusTransition())
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 (!ValidateStatusTransition())
if (!ValidateStatusTransition(subStatus))

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 (!ValidateStatusTransition())
// TODO: Check if this method should be using `targetSubStatus` instead of `SubStatus`.
private bool ValidateStatusTransition(WorkflowSubStatus targetSubStatus)
private bool ValidateStatusTransition()
{
var currentMainStatus = GetMainStatus(SubStatus);

@@ -670,7 +530,7 @@ subStatus switch
_ => throw new ArgumentOutOfRangeException(nameof(subStatus), subStatus, null)
};

private bool ValidateStatusTransition(WorkflowSubStatus targetSubStatus)
private bool ValidateStatusTransition()
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
private bool ValidateStatusTransition()
private bool ValidateStatusTransition(WorkflowSubStatus subStatus)

@@ -670,7 +530,7 @@ subStatus switch
_ => throw new ArgumentOutOfRangeException(nameof(subStatus), subStatus, null)
};

private bool ValidateStatusTransition(WorkflowSubStatus targetSubStatus)
private bool ValidateStatusTransition()
{
var currentMainStatus = GetMainStatus(SubStatus);
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 currentMainStatus = GetMainStatus(SubStatus);
var currentMainStatus = GetMainStatus(subStatus);

Comment on lines 10 to 12
/// <summary>
/// Adds extension methods to <see cref="ExceptionHandlingMiddleware"/>.
/// </summary>
Copy link
Member

Choose a reason for hiding this comment

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

Not a big deal (at all), but it gave me a giggle to see the effort that was made in the WorkflowExecutionContext where all the ///<summary> lines were removed, but are added here 😄

@raymonddenhaan raymonddenhaan merged commit 1437635 into patch/3.2.x Aug 7, 2024
3 checks passed
@raymonddenhaan raymonddenhaan deleted the enh/engine_ex branch August 7, 2024 09:37
@Suchiman
Copy link
Contributor

Why was the <summary> removed? without that, VS does not display documentation anymore

@sfmskywalker
Copy link
Member

To reduce the amount of noise in a file. However, we weren’t aware that this would break VS.

@Suchiman
Copy link
Contributor

You can change it from

/// <summary>
/// description
/// </summary>

to

/// <summary>description</summary>

if you desire a more compact format, without breaking VS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants