Feature - Workflow Middleware #684

dflor003 · 2020-10-22T14:33:12Z

Added concept of workflow middleware and workflow step middleware
Added helpers to register these with DI

PR can close #678, #665, and possibly a few other issues related to retry policies around steps.

dflor003 · 2020-10-22T14:43:51Z

A few things that I still need to do:

Determine how we are going to handle exceptions in post workflow middleware. Just need some direction from @danielgerlag on how this OnException should work.
Update the main docs with info on middleware and links to the sample project. What area of the docs does it make the most sense to add this to?
Have a discussion about persisting workflow step state to the PersistenceProvider from within a workflow step middleware. This would be pretty useful to allow end-users of Workflow Core to stuff additional metadata into ExtensionAttributes. I'm thinking we can either add such a mechanism as part of this PR or address it in a separate PR to get this out sooner.

dflor003 · 2020-10-22T15:49:32Z

Re: Exception handling in post-workflow middleware. Here's another idea that's an offshoot of your suggestion of introducing an OnException method.

What if we add another method to IWorkflowBuilder similar to UseDefaultErrorBehavior that takes in a type that should be invoked when an exception occurs. This will get set on the WorkflowDefinition and will default to essentially a noop that catches the exception and does nothing so that there will be no issues of backwards compatibility.

Here's an example of how that could look:

public class MyWorkflow: IWorkflow<object> {
  public string Id => nameof(MyWorkflow);
  public int Version => 1;

  public void Build(IWorkflowBuilder<object> builder) =>
    builder
      .OnPostMiddlewareException<MyPostMiddlewareExceptionHandler>()
      .StartWith<SomeStep>();
}

public class MyPostMiddlewareExceptionHandler : IPostWorkflowMiddlewareExceptionHandler {
  public MyPostMiddlewareExceptionHandler(...) {
    // Will fetch from DI so you can inject whatever dependencies you
    // need in case you want to ship the error to DB or some external service
  }

  public Task Handle(Exception ex) {
    // Handle it somehow here
  }
}

danielgerlag · 2020-10-22T15:52:09Z

How would that work for JSON or YAML defined workflows?

dflor003 · 2020-10-22T15:54:53Z

Oh, I hadn't considered that... Could it work the same way that you specify StepType or DataType? Something like this:

Id: AddWorkflow
Version: 1
DataType: MyApp.MyDataClass, MyApp
OnPostMiddlewareException: MyApp.MyPostMiddlewareExceptionHandler, MyApp
Steps:
- Id: Hello
  StepType: MyApp.HelloWorld, MyApp
  NextStepId: Add
- Id: Add
  StepType: MyApp.AddNumbers, MyApp
  NextStepId: Bye
  Inputs:
    Value1: data.Value1
    Value2: data.Value2
  Outputs:
    Answer: step.Result
- Id: Bye
  StepType: MyApp.GoodbyeWorld, MyApp

dflor003 · 2020-10-22T15:57:04Z

src/WorkflowCore/Interface/IWorkflowMiddleware.cs

+    /// <summary>
+    /// Determines at which point to run the middleware.
+    /// </summary>
+    public enum WorkflowMiddlewarePhase


I had considered also having a WorkflowMiddlewarePhase.Both to enable middleware that runs both pre/post. Do you think that would be a good idea?

dflor003 · 2020-10-22T16:02:30Z

src/samples/WorkflowCore.Sample19/Middleware/PollyRetryMiddleware.cs

+                MaxRetries
+            );
+
+            // TODO: Come up with way to persist workflow


I left a todo here. We should figure out if it makes sense to allow you to persist workflow steps.

So, re: persistence of steps. In our app that uses Workflow Core, we already have a way of persisting steps so we have a work around for now. In the interest of getting this PR out sooner, I'm thinking of not tackling step persistence as part of this PR and tackling it as a separate issue. Sound good?

dflor003 · 2020-10-22T16:04:08Z

test/WorkflowCore.TestAssets/LockProvider/DistributedLockProviderTests.cs


        [Test]
-        public async void AcquiresLock()
+        public async Task AcquiresLock()


When I was running the tests, NUnit was complaining that these tests were invalid because they were async void instead of async Task. When you have async void, it does not give NUnit the hooks it needs to wait for the test to complete.

dflor003 · 2020-10-22T17:20:20Z

Seems like its having trouble running some of the integration tests in appveyor and I'm not sure why. I tried to follow the same pattern as all of the other integration tests. Is there any reason you can see that these would stall?

This is the offending test:
https://github.com/danielgerlag/workflow-core/pull/684/files#diff-fb926f25cd2306c89a5c425ebd0e886c8d4f8a8f35f665b34df3d3047adbde95R130

Edit: I have a feeling that it is due to threading mixing with async tasks. I've created async overloads of StartWorkflow and WaitForWorkflowToComplete in the integration tests and will see if that fixes it.

danielgerlag

Fantastic contribution! Thank you so much!

danielgerlag · 2020-10-24T17:55:47Z

src/WorkflowCore/Services/WorkflowController.cs

@@ -83,6 +85,8 @@ public async Task<string> StartWorkflow<TData>(string workflowId, int? version,

            wf.ExecutionPointers.Add(_pointerFactory.BuildGenesisPointer(def));

+            await _middlewareRunner.RunPreMiddleware(wf);


Do you think there would be use cases where the pre-workflow middleware would want access to the ID of the instance?

The ID of the WorkflowInstance? Yes. And with the way that the interface looks, they should have access to every property of the WorkflowInstance since it gets passed down to the Handle method.

Yes, but the ID is generated when you first persist the workflow, which hasn't happened at this point?

Ah, it is? Is the ID always a Guid? If so, we can rely on that. I was hoping to have it run before the persistence step so that any changes to the workflow (i.e. setting the description) would be persisted along with it.

Would it make sense to persist it once before and once after?

It's not strictly a Guid... it's up to the implementation of the persistence provider...
If we are going to persist it and then do more work... we'd also probably need to hold a lock on the workflow ID, so that none of the workers pick it up and try to process it before we've finished. There is potential for a race condition here.

Hmm... that would add a bit of complexity to it. So how about we just document that the workflow does not have an id in the pre-workflow middleware and keep it prior to the initial persistence?

Yeah, we can revisit this in future versions

danielgerlag · 2020-10-24T17:58:52Z

src/WorkflowCore/Services/WorkflowMiddlewareRunner.cs

+    /// <summary>
+    /// Runner with the ability to apply middleware to steps and workflows.
+    /// </summary>
+    public class WorkflowMiddlewareRunner : IWorkflowMiddlewareRunner


Do you think this class should also execute the actual step?
If we go that route... should we maybe name it as to indicate that it also has this responsibility? (but now it'd have 2 responsibilities)

As it stands now the workflow middleware runner does execute the step as the last "next" in the step middleware chain. This is deliberate to allow step middleware to add logic around the execution of a step and even potentially change the step's result.

Are you suggesting to not have it run the step?

No, I think we it might help if the name of the class indicated that it also ran the step?

Ah gotcha. Sounds good. Will rename to reflect that. Something like WorkflowMiddlewareAndStepRunner? Or perhaps WorkflowStepRunner and the middleware portion is implied?

Hmm... Thinking about this some more. WorkflowMiddlewareAndStepRunner doesn't exactly sound well. Then I tried changing it to WorkflowStepRunner but that doesn't imply that it also runs pre/post workflow middleware. So regarding naming, I think we have these options:

Keep it as WorkflowMiddlewareRunner and document that it also runs the step in the method name and comments. We could always rename RunStep -> RunStepWithMiddleware to emphasize that it runs the step and also runs middleware around it.

Split the class into two classes, one for running pre/post middleware and the other for running steps with middleware. I'd probably call them WorkflowMiddlewareRunner and WorkflowStepRunner respectively. Although the major con here is that there will be two dependencies needed to be added to the executor constructor instead of one.

I'm in favor of option 1. What are your thoughts?

I feel like the main objective is to run the step, and running the middleware is a secondary part of that, so in that case I would opt to name it something like StepExecutor and keep the functionality as you have it?

Sounds good. I like StepExecutor

With this being called StepExecutor now, I think it makes even more sense to split it into two classes, one for steps and one for workflow pre/post middleware. Do you agree?

Take a look at my latest changes, I split it out and it made testing much easier and follows the single responsibility principle quite nicely. Now there's StepExecutor which executes the step with middleware and WorkflowMiddlewareRunner ONLY runs pre/post workflow. This felt the most natural.

danielgerlag · 2020-10-24T18:00:16Z

src/samples/WorkflowCore.Sample19/Program.cs

+
+            // Add some pre workflow middleware
+            // This middleware will run before the workflow starts
+            services.AddWorkflowMiddleware<AddDescriptionWorkflowMiddleware>();


Should we have some mechanism for defining the priority / order the middleware executes?

I was thinking about that as well, but it would introduce some additional complexity and also probably add some additional fields to the interface. In theory, I could have some kind of field like the following on the interface:

int? Order { get; }

However, then every consumer would be forced to implement it even in cases where they don't care about the order. Do you feel the additional complexity is warranted? Are there any use-cases where just changing the order in which the middleware are defined is not sufficient?

Perhaps we leave it as a possibility for future versions?

Yeah, sounds good. So leave it as is for now?

dflor003 · 2020-10-27T16:01:30Z

Checked in the new exception handling for post workflow middleware. Take a look and tell me what you think. I also retrofitted the code to be able to load it from yaml as well. Updated the sample 19 with example usage as well.

dflor003 · 2020-10-27T22:32:02Z

Docs fresh off the press! Let me know what you think:

https://github.com/danielgerlag/workflow-core/blob/8e0a5c2a8a7785d818523f7cc781d3dc8b1b656c/docs/workflow-middleware.md

danielgerlag · 2020-10-31T02:08:49Z

src/WorkflowCore/Models/WorkflowDefinition.cs

@@ -18,14 +18,16 @@ public class WorkflowDefinition

        public WorkflowErrorHandling DefaultErrorBehavior { get; set; }

-        public TimeSpan? DefaultErrorRetryInterval { get; set; }                
+        public Type OnPostMiddlewareError { get; set; }


Does it make sense to define how middleware fails on the workflow level or the global level?
What were the uses cases you had in mind?

Basically, as it stands now, you can do either. You can define it at the global level by implementing your own IWorkflowMiddlewareErrorHandler or overriding it on the individual workflow level by using OnPostMiddlewareError.

One potential use case I can think of is if your workflow middleware does something like shipping workflow metrics to a timeseries DB like InfluxDB. If for whatever reason the connection to influx is down, you may want to add some special handling for the workflows like queueing up the metrics to ship later on when it comes back up.

I was just wondering if it makes sense to be able to define it on the individual workflow level? What uses cases does that enable?

Ah good point. None off the top of my head that couldn't be handled with a global handler. Should I remove it?

Yeah, I think so... if we find use cases for it we can always add it but let's try manage the complexity for now.

Sounds good. Removed it and updated docs, examples, and tests.

…round workflow steps and before/after workflow Add sample with a sample middleware for retrying and log correlation as well as workflow pre/post samples Add async overloads for StartWorkflow and WaitForWorkflowToComplete in integration tests Add error handling of post workflow middleware Add docs for workflow middleware

danielgerlag · 2020-11-02T01:32:02Z

@dflor003 The middleware scenario test is failing on my local machine for some reason.
As soon as I can get this working, I can publish a new version.

dflor003 · 2020-11-02T01:59:12Z

@danielgerlag Hey! Just saw this. Hmm... This is the integration test? I was seeing that some of the tests will sporadically fail to wait for the workflow to complete. After some re-runs, it usually passes. I have a feeling that it may be due to threading and async mixing together which is why I introduced StartWorkflowAsync and WaitForWorkflowToCompleteAsync. Did it pass when you re-ran it?

dflor003 · 2020-11-02T02:01:51Z

Oh damn... Wait. I think there's a race condition somewhere. Never mind. It may be unrelated to that. It's an issue with the test itself.

dflor003 · 2020-11-02T02:07:19Z

I'll have a PR out shortly fixing it. Just need to await a few things.

dflor003 · 2020-11-02T02:13:31Z

@danielgerlag Take a look at #690. That should fix the issue.

dflor003 mentioned this pull request Oct 22, 2020

Feature Proposal - Workflow and Step Middleware #678

Closed

dflor003 commented Oct 22, 2020

View reviewed changes

danielgerlag reviewed Oct 24, 2020

View reviewed changes

danielgerlag approved these changes Oct 31, 2020

View reviewed changes

danielgerlag merged commit 95f99e9 into danielgerlag:master Nov 2, 2020

dflor003 deleted the feature/WorkflowMiddleware branch November 2, 2020 01:59

		@@ -83,6 +85,8 @@ public async Task<string> StartWorkflow<TData>(string workflowId, int? version,

		wf.ExecutionPointers.Add(_pointerFactory.BuildGenesisPointer(def));

		await _middlewareRunner.RunPreMiddleware(wf);

Feature - Workflow Middleware #684

Feature - Workflow Middleware #684

Conversation

dflor003 commented Oct 22, 2020 • edited

dflor003 commented Oct 22, 2020 • edited

dflor003 commented Oct 22, 2020

danielgerlag commented Oct 22, 2020

dflor003 commented Oct 22, 2020

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

dflor003 commented Oct 22, 2020 • edited

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

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

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

dflor003 commented Oct 27, 2020

dflor003 commented Oct 27, 2020 • edited

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

danielgerlag commented Nov 2, 2020

dflor003 commented Nov 2, 2020 • edited

dflor003 commented Nov 2, 2020

dflor003 commented Nov 2, 2020

dflor003 commented Nov 2, 2020

dflor003 commented Oct 22, 2020 •

edited

dflor003 commented Oct 22, 2020 •

edited

dflor003 commented Oct 22, 2020 •

edited

dflor003 commented Oct 27, 2020 •

edited

dflor003 commented Nov 2, 2020 •

edited