You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
inside an asynchronous method a result of SendRequest is taken synchronously.
public async Task<string> MakeRequest(string url, HttpMethod method, HttpContent content = null)
{
var response = SendRequest(url, method, content).Result;
var responseContent = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
if (!response.IsSuccessStatusCode)
{
HandleHTTPErrors(response, responseContent);
}
return responseContent;
}
I'm wondering if there was a specific reason for that, or can it become just var response = await SendRequest(url, method, content)?
There's a very minor issue with that - if SendRequest throws an exception (e.g. TaskCancelled upon timeout), then caller would get an AggregateException, which is a bit counter-intuitive and less convenient to process.
Also, such a pattern seem a bit dangerous as theoretically it could lead to deadlocks depending on circumstances. In our application we haven't seen any and there are ConfigureAwait-s, so that mightn't be a real issue.
The text was updated successfully, but these errors were encountered:
euantorano
added a commit
to euantorano/notifications-net-client
that referenced
this issue
Sep 27, 2023
At this line
notifications-net-client/src/Notify/Client/BaseClient.cs
Line 74 in 6931da0
SendRequest
is taken synchronously.I'm wondering if there was a specific reason for that, or can it become just
var response = await SendRequest(url, method, content)
?There's a very minor issue with that - if
SendRequest
throws an exception (e.g.TaskCancelled
upon timeout), then caller would get anAggregateException
, which is a bit counter-intuitive and less convenient to process.Also, such a pattern seem a bit dangerous as theoretically it could lead to deadlocks depending on circumstances. In our application we haven't seen any and there are
ConfigureAwait
-s, so that mightn't be a real issue.The text was updated successfully, but these errors were encountered: