Actual implementation of IDisposable

Question

Note: Title is for one question but I actually have 3 questions to ask.
I am using Autofac DI container to resolve dependencies.
If there are any other improvements that can be made to this code, please suggest.
I have already went through the following links:
Implementing IDisposable correctly
Implementing a Dispose method
IDisposable Part 1

Consider the following code:
Interface: IHttpClient

public interface IHttpClient : IDisposable
{
    HttpRequestHeaders DefaultRequestHeaders { get; set; }    // Question 1

    Uri BaseAddress { get; set; }

    Task<HttpResponseMessage> PostAsync(string uri, HttpContent httpContent);
}

Class: HttpClientAdaptor

internal HttpClientAdaptor : IHttpClient
{
    private HttpRequestHeaders _defaultRequestHeaders;
    private bool _disposed = false;

    public HttpRequestHeaders DefaultRequestHeaders
    {
        get
        {
            if (_defaultRequestHeaders == null)
                _defaultRequestHeaders = new HttpClient().DefaultRequestHeaders;

                return _defaultRequestHeaders;
        }    
        set
        {
            if (value != null)
                _defaultRequestHeaders = value;
        }
    }

    public Uri BaseAddress { get; set; }

    private async Task<HttpResponseMessage> PostAsync(Uri uri, HttpContent httpContent, CancellationToken cancellationToken)
    {
        HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Post, uri)
        {
            Content = httpContent
        };

        return await new HttpMessageInvoker(new HttpClientHandler()).SendAsync(request, cancellationToken);     // Question 2
    }

    protected virtual void Dispose(bool disposing)
    {
        if (!_disposed)
        {
            if (disposing)
            {
                _defaultRequestHeaders = null;
                BaseAddress = new Uri();      // Question 3
            }

            _disposed = true;
        }
    }

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

Consider the following class, interface IHttpClient and // Question 1:

class DS
{
    private IHttpClient _apiClient;

    public class DS(/*other dependencies*/, IHttpClient apiClient)
    {
        _apiClient = apiClient;
    }

    // At somepoint I need to perform the following operation
    private void SomeMethod(string hostname, /*other params*/)
    {
        _apiClient.BaseAddress = new Uri(hostName);
        _apiClient.DefaultRequestHeaders.Accept.Clear();   // this line throws exception as "DefaultRequestHeaders" is null
        _apiClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
    }
}

Question 1: Why does it throws a null exception, when I've already resolved the dependencies. To avoid this I have to add the following line:

_apiClient.DefaultRequestHeaders = new HttpClientAdaptor().DefaultRequestHeaders;

Is there a better way to do it, rather than using above mention code everywhere I need DefaultRequestHeader?

Question 2: Consider the class HttpClientAdaptor and comment // Question 2. I need to make call to the SendAsync(). Is this the correct way or should I inherit the class HttpMessageInvoker. Or Is there a third way to do it?

Question 3: Consider the class HttpClientAdaptor and comment // Question 3. Is this the right way to dispose managed code. If it is then how should I dispose property BaseAddress? And if not, please suggest the right way. And of course current code is giving compile time error for obvious reasons.

Autofac will only resolve what you register with it, and it won't do anything to register the internals of a class you do register. Autofac--and other DI containers--won't instantiate every property of a object just because they exist; that's the constructor's job. — Kenneth K., Jul 23 '18 at 12:45
A3: You can safely dispose your `Dispose()`. Your class does not have to be `IDisposable`. — bommelding, Jul 23 '18 at 12:55
@bommelding Could you pleas elaborate the **how** of your answer? — phougatv, Jul 23 '18 at 12:59
The __how__ is simply to delete `: IDisposable` and both `Dispose()` methods. — bommelding, Jul 23 '18 at 13:12
The __why__ is that you do not have any managed or unmanaged resources. — bommelding, Jul 23 '18 at 13:13
Not possible to delete because it's an implementation of system interface. It can just be left blank. — Ilya Chernomordik, Jul 23 '18 at 13:19
@IlyaChernomordik - that interface is the first thing to remove here. — bommelding, Jul 23 '18 at 14:43

Ilya Chernomordik · Accepted Answer · 2018-07-23T13:07:11.440

2

Question 1: As noted in the comments, no DI initializes your fields by calling get properties, so what you need to do is to move the initialization in the constructor.

Question 2: It is a bit unclear what you want to achieve, as you noted in comments you use it for unit testing. One don't do actual calls in the mocks. So maybe you need something else in there? E.g. to remember that the the call happened and possibly the parameters it was called with.

Question 3: No, it does not seem the right way to dispose the way you implemented it. One need to implement IDisposable in these 2 cases:

You have a member that is IDisposable that you will need to call Dispose on
You have some unmanaged resources directly

It does not seem that any of those is the case for you so you don't really need to do anything inside dispose at all as it seems only you have it at all due to the fact that you are re-implementing system interface.

That still means though that those who will use your HttpClientAdaptor class would need to do dispose because they are unaware that it actually does nothing. Though of course if you use it yourself you can skip calling dispose without any consequences.

P.S. If you use your class for unit testing, you can use one of the mocking libraries that are intended for this purpose and will do the job dynamically. Moq or NSubstitute are good candidates.

edited Jul 23 '18 at 13:07

answered Jul 23 '18 at 12:43

Ilya Chernomordik

27,817
27
121
207

`HttpClientAdaptor` is sort of replica of `HttpClient`. Created to unit test the `PostAsync()`. Since `HttpClient` implements `Dispose(bool disposing)`, so I also wanted to do the same. Just couldn't figure out the right way. – phougatv Jul 23 '18 at 12:47
It does that because it itself has either unmanaged resources of references to `IDisposable`. Since you don't have it, no need for you to do anything but to comply to the interface and leave it blank :) – Ilya Chernomordik Jul 23 '18 at 12:50
So, according to you I should be writting the `Dispose()` in the following manner: https://stackoverflow.com/questions/18336856/implementing-idisposable-correctly#answer-18336995 – phougatv Jul 23 '18 at 12:54
2

No, not really. You wrote dispose just fine, but the thing is: you don't need it at all for this case. So you can just have an empty `Dispose() {}` call. There is just nothing in your class to dispose of. – Ilya Chernomordik Jul 23 '18 at 12:56
I am already using the `NSubstitute`. But could not mock the `PostAsync()` of `HttpClient` since it is not virtual. Because of this I created my own. Thanks for your time. – phougatv Jul 23 '18 at 13:01
"So maybe you need something else in there?" When you´re up to write this ythe question is - as you´ve stated - unclear. Yiu shouldn´t answer questions that can party be answered. They should be closed as too broad, though. – MakePeaceGreatAgain Jul 23 '18 at 13:04
Why do you try to do the actual post in a unit test? I would expect that you need just to remember that it was called and probably with which parameters to do a verify later? – Ilya Chernomordik Jul 23 '18 at 13:06
I never tried to do the actual post in a unit test? I am saying I could not have unit tested the `HttpClient` since `PostAsync()` is **not virtual**. So I implemented my own interface to replicate the `PostAsync()` of `HttpClient`. – phougatv Jul 23 '18 at 13:09
Yes, I have understood why you do this. Maybe I have misunderstood this line: `return await new HttpMessageInvoker(new HttpClientHandler()).SendAsync(request, cancellationToken); ` Won't it do an actual operation and send the request? – Ilya Chernomordik Jul 23 '18 at 13:18
Yes, I will make an actual request. But this is a `private` method and not exposed in interface. An overload `PostAsync()` of this method is exposed and thus will be mocked. Just to clear your doubt this is the code in my test case: `_fakeHttpClient.PostAsync(Arg.Any(), Arg.Any()).Returns(Task.FromResult(httpResponseMessage));` where `_fakeHttpClient` is defined as follows: `IHttpClient _fakeHttpClient = Substitute.For();` and `httpResponseMessage` is defined as follows: `var httpResponseMessage = new HttpResponseMessage { StatusCode = (HttpStatusCode)200 };`. – phougatv Jul 23 '18 at 13:30

Actual implementation of IDisposable

1 Answers1