Azure / azure-functions-dotnet-worker

Azure Functions out-of-process .NET language worker
MIT License
425 stars 182 forks source link

Allow for extensions to wrap function invocation delegate #1666

Open jviau opened 1 year ago

jviau commented 1 year ago

Background

Durable functions for dotnet isolated has a need for wrapping all [OrchestrationTrigger] function invocations with additional logic. Today we perform this wrapping via middleware. However, it was discovered this is extremely fragile for durables case because we need to tightly control the precise async behavior in our logic. This has led to non-determinism issues in durable, which arise from the customer introducing any custom middleware between the Durable middleware and the function invocation. This makes orchestrations fragile and is a less than ideal experience for customers to put it lightly.

Proposed Solution

Provide an API where an extension can wrap a function invocation's delegate with a custom delegate. This would most make sense to be doable as part of the middleware. So durable could retain its middleware, but instead of wrapping the rest of the pipeline it performs some action on the FunctionContext (maybe a new IInvocationFeatures from .Features)?

Alternative Solutions

Controlling middleware order

If durable could control its middleware order, it could set its middleware to be the last. However, this is fragile as any other extension could come along and also declare its middleware to be last, and then we are back to square one with this issue.

Replacing function definition invocation

This would rely on introducing an extensibility model to function definition metadata generation and is most likely overkill for this scenario.

horatiu-stoianovici commented 11 months ago

Incompatibility with Azure App Configuration Refresh

Adding a bit more to this issue, working on an internal project in Microsoft, we are using Azure App Configuration for our application and are trying to migrate the durable function projects to the isolated model.

What I found is that it looks like you cannot use the documented way of achieving dynamic config by refreshing your configuration values for Azure App Configuration, since that injects its own middleware to refresh config behind the scenes.

This is a very tough issue to identify as I was getting very weird unexplained behavior when an eternal orchestration just freezes indefinitely when doing context.ContinueAsNew(). Eventually I somehow also got the The orchestrator function completed on a non - orchestrator thread! error, which lead me all the way here. Removing the App Configuration middleware got rid of all the weird behavior.

Are you aware of the issue and any potential workarounds here for Azure App Configuration users?

jviau commented 10 months ago

@horatiu-stoianovici you can add your own middleware and manually use the app configs refresher and refresh only when it is not an orchestration trigger.

max-huenink commented 8 months ago

I have a potential alternative solution that feels more like a band-aid than a permanent solution, but would solve this problem for anyone using custom middleware. I'm open to feedback on this approach.

My solution is to change the UseMiddleware and UseWhen extension methods from WorkerMiddlewareWorkerApplicationBuilderExtensions.c to only invoke middleware when the function context doesn't have an orchestration trigger, using the IsOrchestration method mentioned above. However, this would break the existing durable functions middleware added here DurableTaskExtensionStartup.cs. I've thought of two solutions to this new problem, but I don't know which is a better approach.

  1. Add a new method UseDurableMiddleware that doesn't have the check mentioned above. Then DurableTaskExtensionStartup.cs would use this new method. Existing consumers wouldn't need to change.
  2. Add a parameter to the existing UseMiddleware and UseWhen functions, bool useInOrchestrations = false. Then DurableTaskExtensionStartup.cs would specify true for this parameter. Existing consumers wouldn't need to change.
LockTar commented 3 months ago

@horatiu-stoianovici you can add your own middleware and manually use the app configs refresher and refresh only when it is not an orchestration trigger.

@jviau I'm using an injected EF DbContext in my middleware. I need to read and write some data from and to a database for an Activity and an (sub)Orchestration. I did a .Wait() on my call to fix the issue. Not ideal but I thought it worked. Now, I'm trying to do a fan out by creating multiple sub orchestrations and I get this DbContext threading issue. Any suggestions?