Fix HttpClient deadlocks: Addendum to #2367#2371
Fix HttpClient deadlocks: Addendum to #2367#2371deAtog wants to merge 5 commits intorestsharp:devfrom
Conversation
|
@dotnet-policy-service agree company="The Davey Tree Expert Company" |
Review Summary by QodoRefactor CustomSynchronizationContext to prevent deadlocks
WalkthroughsDescription• Refactored CustomSynchronizationContext to manage context setup internally • Removed ConfigureAwait(false) from task execution to ensure continuations stay on custom context • Added ConfigureAwait(false) to RunSync<T> method's await call • Simplified RunSync method by moving context management into CustomSynchronizationContext.Run() Diagramflowchart LR
A["RunSync calls<br/>CustomSynchronizationContext"] -->|"simplified"| B["CustomSynchronizationContext.Run<br/>manages context"]
B -->|"sets context"| C["SynchronizationContext<br/>set to custom"]
C -->|"executes"| D["PostCallback<br/>without ConfigureAwait"]
D -->|"ensures continuations<br/>stay on context"| E["Prevents deadlocks<br/>and thread pool jumps"]
File Changes1. src/RestSharp/AsyncHelpers.cs
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
…chronizationContext. This simplifies the RunSync method and clarifies the logic of the CustomSynchronizationContext.
As such, the Run method should never be called more than once. Make the constructor and Run method private and add a public static Run method to run a single task on an instance of the context.
|
|
Even with these changes, I'm still getting a deadlock here. It seems #2083 still isn't quite fixed. 
|
…un async methods safely from the Thread Pool.
|
|
@alexeyzimarev With commit 113594e, my application no longer deadlocks upon calling the synchronous methods of the RestClient. This commit also makes my prior changes irrelevant. Based upon my analysis and testing, this should permanently resolve any deadlocks that people have been experiencing. Let me know if you want me to flatten this into the single final commit which actually solves this issue. |
|
There's no need for |
|
Suggestions:
Why the delegate is required: With Func<Task>: RunSync(() => client.GetAsync(request));
// inside: Task.Run(task) → invokes the delegate on a pool thread The call to With Task directly: RunSync(client.GetAsync(request));
// client.GetAsync(...) is evaluated on the caller's thread BEFORE RunSync is even enteredBy the time the And wrapping it after the fact doesn't help: Task.Run(() => task).GetAwaiter().GetResult(); // task is already running!
Side note: this is also why every call site in RestSharp's extensions uses |
2 participants
Description
Adds missingConfigureAwait(false)to await call in RunSync method.Refactors the CustomSynchronizationContext and associated RunSync method for code clarity.Drops theConfigureAwait(false)method call on the task executed from the CustomSynchronizationContext to ensure continuations occur on the custom context, not the thread pool.This https://learn.microsoft.com/en-us/archive/blogs/jpsanders/asp-net-do-not-use-task-result-in-main-context describes the approach taken here to resolve the deadlock. Instead of using Task.Wait, we use Task.GetAwaiter().GetResult() as it will unwrap all exceptions instead of throwing an AggregateException.
Purpose
These changes should prevent deadlocks from calling async methods and ensure that continuations happen synchronously on the CustomSynchronizationContext instead of the thread pool.These changes prevent deadlocks from calling async methods synchronously by executing them on a thread pool thread and capturing the result after the task completes. ConfigureAwait(false) is used to ensure that the task does not capture the context and cause a deadlock.
History
The CustomSychronizationContext implementation was originally pulled from https://github.com/rebus-org/Rebus/blob/27b212a2380d55edc16a0036dfefd8e6b3ad9c2c/Rebus/Bus/Advanced/RebusAsyncHelpers.cs
Their implementation does not call
ConfigureAwait(false)on the task executed from within the CustomSynchronizationContext to ensure any continuations are queued on the CustomSynchronnizationContext, not the thread pool. While this should work it still causes a deadlock when methods of the HttpClient are called synchronously and has been removed as a result.