Fix HttpClient deadlocks: Addendum to #2367#2371
Fix HttpClient deadlocks: Addendum to #2367#2371deAtog wants to merge 1 commit 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}
|
|
Even with these changes, I'm still getting a deadlock here. It seems #2083 still isn't quite fixed. 
|
|
|
@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 |
|
@alexeyzimarev I squashed the commits and updated the description of this pull request. I understand the reason a lambda expression is used when calling RunSync. The lambda expression creates an anonymous function which executes the async function which returns a Task. When passed to Task.Run, the async function is executed on the thread pool and current context would be that of the thread pool, which avoids capturing the caller's context. Task.Run() has an overload that takes a return RunSync(() => client.ExecuteAsync(request));Is therefore equivalent to the following after these changes: return Task.Run(() => client.ExecuteAsync(request)).GetAwaiter().GetResult();This change resolves the deadlock in my application without any other modifications. |
…un async methods safely from the Thread Pool.
|
2 participants
Description
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 prevent deadlocks from calling async methods synchronously by executing them on a thread pool thread and capturing the result after the task completes.
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. This implementation can only run tasks that do not need to run concurrently and use locks or other synchronization mechanisms to ensure thread safety. This implementation is not suitable for wrapping methods of the HttpClient and has been removed as a result.