CA1001 Types that own disposable fields should be disposable over HttpClient

Question

I have a HttpClientWrapper class to bring unit testability over my classes which are making HttpCalls.

This class is injected via IoC, and acts as a singleton. Because I would like to use same HttpClient instance over and over again without disposing it.

Do HttpClient and HttpClientHandler have to be disposed?

public class HttpClientWrapper : IHttpClientWrapper
    {
        readonly HttpClient _client;

        public HttpClientWrapper()
        {
            _client = new HttpClient();
        }
        public Uri BaseAddress
        {
            get { return _client.BaseAddress; }
        }
        public Task<HttpResponseMessage> SendAsync(HttpRequestMessage message)
        {
            Task<HttpResponseMessage> sendAsync = _client.SendAsync(message);
            return sendAsync;
        }


    }

Following clears the warning naturally.

  public class HttpClientWrapper : IHttpClientWrapper, IDisposable
{
    readonly HttpClient _client;

    public HttpClientWrapper()
    {
        _client = new HttpClient();
    }
    public Uri BaseAddress
    {
        get { return _client.BaseAddress; }
    }
    public Task<HttpResponseMessage> SendAsync(HttpRequestMessage message)
    {
        Task<HttpResponseMessage> sendAsync = _client.SendAsync(message);
        return sendAsync;
    }


    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {

            if (_client != null)
            {
                _client.Dispose();
            }
        }

    }

When I run code analysis, I am receiving following warning from IDE.

CA1001 Types that own disposable fields should be disposable Implement IDisposable on 'HttpClientWrapper' because it creates members of the following IDisposable types: 'HttpClient'. If 'HttpClientWrapper' has previously shipped, adding new members that implement IDisposable to this type is considered a breaking change to existing consumers.

So am I missing something here? Since I would like to keep only one instance of HttpClient and would like not to dispose it, can I safely surpress the warning?

Does second version of class implementation makes sense?

Can you suppress it? Sure. Should you? Probably not. You have to assume that at SOME point HttpClient will be disposed and it only makes sense to likewise add `Dispose()` for your wrapper as well. It is consistent, a best practice, and it takes care of those pesky warnings without suppression. — David L, Jun 21 '16 at 21:40
Also, keep in mind that this does NOT create one instance of HttpClient for the lifetime of the app. It creates one instance for the lifetime of the wrapper. There's nothing here that indicates that the HttpClient instance is a singleton...that is only implied via your IoC container. That further reinforces the properness of implementing the Disposable pattern. — David L, Jun 21 '16 at 21:41
@DavidL hmm, since constructor runs once, and wrapper keeps the reference to HttpClient object, I was thinking there will be only one instance in this case? — Teoman shipahi, Jun 21 '16 at 21:43
In this case yes, but the code still doesn't know that from a code analysis perspective. The "proper" solution is to implement the Disposable pattern. As the state of your app exists now, yes, it is a singleton, but that could change down the road and proper implementation now can save you from leakiness later. — David L, Jun 21 '16 at 21:45
@DavidL thanks. Can you please check the updated question? I included the way how I implemented IDisposable. Not sure 100% I am on correct path. — Teoman shipahi, Jun 21 '16 at 21:49
That looks like an acceptable disposable pattern. Seems fine :) — David L, Jun 22 '16 at 01:04

CA1001 Types that own disposable fields should be disposable over HttpClient

0 Answers0