alphagov / notifications-net-client

.NET client for the GOV.UK Notify API
https://www.nuget.org/packages/GovukNotify/
MIT License
25 stars 20 forks source link

Task result obtained synchronously inside an asynchronous method #130

Open ilyalukyanov opened 4 years ago

ilyalukyanov commented 4 years ago

At this line https://github.com/alphagov/notifications-net-client/blob/6931da038ae684879c39f3c8150d25ff873cea2d/src/Notify/Client/BaseClient.cs#L74 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.