Azure / azure-functions-durable-extension

Durable Task Framework extension for Azure Functions
MIT License
711 stars 263 forks source link

System.InvalidOperationException: Multithreaded execution was detected. #2704

Closed yell0wfl4sh closed 6 months ago

yell0wfl4sh commented 6 months ago

I recently added Microsoft's threading analyzer to our solution and it pointed out an error in our orchestration code.

I changed couple of line of calling an async API from api.Result to await api.configureAwait(true). All orchestrations started failing with the following error:

Message: MessageCampaignOrchestratorV2.cs_Run(1525): Marking simulation as failed due to an exception.,
 Exception : System.InvalidOperationException: Multithreaded execution was detected. This can happen if the orchestrator function code awaits on a task that was not created by a DurableOrchestrationContext method. More details can be found in this article https://learn.microsoft.com/azure/azure-functions/durable/durable-functions-code-constraints.

Please help me understand why the .Result implementation works and await one doesn't.

Thanks, Yash

jviau commented 6 months ago

You cannot call external async APIs from an orchestration. This work must be performed from within an activity.

Assuming api in your example is some external API, then both the .Result and the await approaches are incorrect. By making these calls your orchestration is non-deterministic and you may see multiple calls to that API due to our replay behavior.

Writing Task Orchestrations

yell0wfl4sh commented 6 months ago

@jviau Thanks for clarifying! Although we have few external async calls in our orchestration. We will have to carefully flight or write new orchestration file to get rid of these calls. In a similar effort, I am writing new files after cleaning async calls, and also converted existing API from .Result to await approach.

I wanted to understand why my service is not throwing error for existing .Result and await calls but failed when I converted for API to await.

Thanks~!

jviau commented 6 months ago

I wanted to understand why my service is not throwing error for existing .Result and await calls but failed when I converted for API to await.

This is due to a combination of two things: how C# async/await works and how durable extension performs some limited safe-guards against invalid orchestrations.

First, .Result is a synchronous block - that thread is blocked and stays active while the dependent task works. Your code is not async at all here, you never yield the thread and remain on it before and after that .Result call. When using async/await this is an asynchronous block. Your code will be dequeued from the thread (releasing it for other tasks to use), and then posted back to the thread pool when the dependent async work is complete. Due to that yielding and posting back, you may be on a different thread before and after the await keyword - this different thread is important.

Second, is how durable performs some limited checks for invalid orchestrations. In this case, we record what thread ID your orchestration starts on and then will throw if that thread ID ever changes for the life of a given orchestration replay. This works for valid orchestrations due to the way our record-replay works (a lot of details here I won't get into). This lets us detect and throw for invalid await calls (ones not done through our approved APIs, because they will most likely return you to a different thread). This check is not absolute and there are many ways to get around it (such as using .Result). But getting around it doesn't make your orchestration valid - you have just obscured the invalid calls from us. Also keep in mind it is not the switching of thread IDs precisely we care about, but what you most likely have done as a result of switching thread IDs: you probably did mutable non-deterministic work, which must not be done directly in an orchestration, it must all be in activities.

Your code was "invalid" with both methods - we just could only detect it in the 2nd way due to the thread ID change.

Hope this answers your question!

yell0wfl4sh commented 5 months ago

@jviau thanks for detailed explanation!