7

I have an old version of ASP.NET MVC app that doesn't have a Startup.cs. I wanted to implement a clean way to have an HttpClient that I would use for my API calls to third parties.

Here's what I've done so far based on some ideas/recommendations I've received for this question. The problem is that when I make the API call, it goes nowhere. I put it in a try catch but I'm not even getting an exception. The API provider tells me that they're not seeing the search parameter.

First, I created this HttpClientAccessor for lazy loading.

public static class HttpClientAccessor
{
   public static Func<HttpClient> ValueFactory = () =>
   {
      var client = new HttpClient();

      client.BaseAddress = new Uri("https://apiUrl.com");
      client.DefaultRequestHeaders.Accept.Clear();
      client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
      client.DefaultRequestHeaders.TryAddWithoutValidation("APIAccessToken", "token1");
      client.DefaultRequestHeaders.TryAddWithoutValidation("UserToken", "token2");

       return client;
   };

   private static Lazy<HttpClient> client = new Lazy<HttpClient>(ValueFactory);

   public static HttpClient HttpClient
   {
      get { return client.Value; }
   }
}

I then created an API client of my own so that I can have the API call functions in one place which looks like this:

public class MyApiClient
{

   public async Task GetSomeData()
   {
       var client = HttpClientAccessor.HttpClient;
       try
       {
           var result = await client.GetStringAsync("somedata/search?text=test");
           var output = JObject.Parse(result);
       }
       catch(Exception e)
       {
           var error = e.Message;
       }
    }
}

Then in my ASP.NET Controller action, I do this:

public class MyController : Controller
{
   private static readonly MyApiClient _apiClient = new MyApiClient ();

   public ActionResult ApiTest()
   {
       var data = _apiClient.GetSomeData().Wait();
   }
}

Any idea where my mistake is?

UPDATE: This simple approach works fine:

public class MyController : Controller
{
   private static readonly HttpClient _client = new HttpClient();

   public ActionResult ApiTest()
   {
       _client.BaseAddress = new Uri("https://apiUrl.com");
       _client.DefaultRequestHeaders.Accept.Clear();
       _client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
       _client.DefaultRequestHeaders.TryAddWithoutValidation("APIAccessToken", "token1");
       _client.DefaultRequestHeaders.TryAddWithoutValidation("UserToken", "token2");

       var response = _client.GetStringAsync("somedata/search?text=test").Result;
   }
}
Sam
  • 26,817
  • 58
  • 206
  • 383

4 Answers4

6

As mentioned, dependency injection is not being utilized so technically there is no need for a composition root where these things would have been initialized.

If there is no need to actually initialize the client on start up you could consider using a Lazy singleton approach.

An example

public static class HttpClientAccessor {
   public static Func<HttpClient> ValueFactory = () => {
      var client = new HttpClient();

      client.BaseAddress = new Uri("https://apiUrl.com");
      client.DefaultRequestHeaders.Accept.Clear();
      client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
      client.DefaultRequestHeaders.TryAddWithoutValidation("APIAccessToken", "token1");
      client.DefaultRequestHeaders.TryAddWithoutValidation("UserToken", "token2");

       return client;
   };

   private static Lazy<HttpClient> client = new Lazy<HttpClient>(ValueFactory);

   public static HttpClient HttpClient {
      get { return client.Value; }
   }
}

The factory delegate of the Lazy<HttpClient> can be made more complex if additional settings are needed on the client.

And where ever the client is needed you call the service

var client = HttpClientAccessor.HttpClient;

var response = await client.GetStringAsync("{url}");

the client will be initialized on first use and you will get the same instance on subsequent calls for the instance.

As used in your controller, you are mixing async calls with blocking calls line .Wait() or .Result. This can lead to deadlocks and should be avoided.

public class MyController : Controller {
    private static readonly MyApiClient _apiClient = new MyApiClient ();

    public async Task<ActionResult> ApiTest() {
        var data = await _apiClient.GetSomeData();

        //...
    }
}

Code should be async all the way through.

Reference Async/Await - Best Practices in Asynchronous Programming

Nkosi
  • 235,767
  • 35
  • 427
  • 472
  • Thank you. I was just reading up about making the singleton thread safe. The `lazy` approach will create the instance only when it's necessary, correct? What about making it thread safe? I read somewhere that using a single instance would actually create performance issues. The correct way as it's suggested is to have a single instance per thread. – Sam Apr 03 '18 at 03:44
  • @Sam it is advised for `HttpClient` to use one instance for the life cycle of an application. having multiple instance of it and disposing of them is what will have performance issues. – Nkosi Apr 03 '18 at 03:47
  • I was actually planning on creating an `ApiClient` that would create the `HttpClient` and use your approach to create my `ApiClient`. Or is it a better idea to create a "general purpose" `HttpClient` that I can access from my `ApiClient`? – Sam Apr 03 '18 at 03:50
  • OK. You nailed it. When I made my controller action `async`, the code started working. Thank you for sticking with this and helping me out. I really appreciate it. – Sam Apr 09 '18 at 17:42
  • There is no need for a trailing slash in `BaseAddress` because it's path is empty. See [Why is HttpClient BaseAddress not working?](https://stackoverflow.com/questions/23438416/why-is-httpclient-baseaddress-not-working) discussion on StackOverflow. – Leonid Vasilev Apr 12 '18 at 07:17
0

The Application_Start() method is the right place. But I would have to ask: why you have to create the HttpClient instance when the "application starts"? In general, HttpClient is some "resource" and you can just create it when you want to use it. And also it's no need to set it as "Singleton". Just wrap it in the using block. (Maybe you want to make the API wrapper as Singleton?)

public class APICaller
{

    //make the APICaller singleton in some way here
    //...


    // the api calling method:
    public string CallAPI(string someParameter)
    {
        var response = "";
        using (var client = new HttpClient())
        {
            //calling the API
        }
        return response;
    }
}
biz.tim
  • 60
  • 1
  • 6
  • HttpClient is meant to be a static resource that is reused for the duration of the application. – Alex Terry Apr 13 '18 at 19:55
  • @ATerry include a link to something which supports your claim plz – Seabizkit Apr 13 '18 at 21:06
  • @Seabizkit read the remarks, https://msdn.microsoft.com/en-us/library/system.net.http.httpclient(v=vs.110).aspx – Alex Terry Apr 15 '18 at 14:44
  • @ATerry no where in that link does it back what you said `HttpClient is meant to be a static resource` I don't think that statement is true. You are welcome to correct me. Even in their own example they are overriding the static instance each time. I believe HttpClient should have a life span. – Seabizkit Apr 15 '18 at 17:23
  • 2
    @Seabizkit this is directly from the remarks `HttpClient is intended to be instantiated once and re-used throughout the life of an application. Instantiating an HttpClient class for every request will exhaust the number of sockets available under heavy loads. This will result in SocketException errors. Below is an example using HttpClient correctly.` The example shows a static HttpClient, not per instance. If you don't believe me or MSDN just google "HttpClient Static"; there are hundreds of articles on the issue. – Alex Terry Apr 16 '18 at 13:27
  • Kinda late but here's a clear description. Sorry for the link: https://learn.microsoft.com/en-us/dotnet/architecture/microservices/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests. Notable excerpt: For more information about this issue, see the blog post `You're using HttpClient wrong and it's destabilizing your software.` – No Refunds No Returns Apr 01 '21 at 15:37
0

The main issue is incorrect asynchronous code.

You are using Task.Wait() which alongside asynchronous MyApiClient.GetSomeData() causes a deadlock on ASP.NET request context. That is a very common issue, see An async/await example that causes a deadlock on StackOverflow. Code with Task.Result property call is working because HttpClient.GetStringAsync() probably takes preventative measures against deadlocks. See Task.ConfigureAwait() page on MSDN and Best practice to call ConfigureAwait for all server-side code discussion on StackOverflow.

There are multiple options to write a singleton using C#. See Implementing the Singleton Pattern in C# article by Jon Skeet for a detailed overview.

Leonid Vasilev
  • 11,910
  • 4
  • 36
  • 50
0

As you mentioned, you can just use a static class member on the controller. HttpClient only needs to be setup once; so do this in the static constructor of the controller. Also, make sure that you use async/await for async methods, especially with long running http requests. IOC and an abstraction layer would make sense depending on your needs.

using System;
using System.Net.Http;
using System.Threading.Tasks;

namespace TestApi
{
    public class MyController : Controller
    {
        private const string ApiUrlString = "https://apiUrl.com";
        private static readonly Uri ApiUri = new Uri(ApiUrlString);
        private static readonly HttpClient RestClient;

        static MyController()
        {
            this.RestClient = new HttpClient{
                BaseAddress = ApiUri
            }
            this.RestClient.DefaultRequestHeaders.Accept.Clear();
            this.RestClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
            RestClient.DefaultRequestHeaders.TryAddWithoutValidation("APIAccessToken", "token1");
            RestClient.DefaultRequestHeaders.TryAddWithoutValidation("UserToken", "token2");
        }

        public async Task<IActionResult> ApiTest()
        {
            return this.Ok(await this.RestClient.GetStringAsync("somedata/search?text=test"));
        }
    }
}
Alex Terry
  • 1,992
  • 1
  • 11
  • 17