Make TezosRpc mockable by injecting HttpMessageHandler

[sshine]

This closes #9 and closes #11.

Following the discussion of #11, this PR injects HttpMessageHandler in through TezosRpc and RpcClient. It adds ctors that use Uri and TimeSpan instead of string and int and marks existing ctors as obsolete.

Constants like "2 hours", "10 seconds" and "main" are exported to the top.

---

This was made in collaboration with @Sword-Smith.

Please let me know if something needs to be fixed for this PR to be accepted.

[Groxan]

Thank you for the PR and sorry for confusing in the discussion of #11. I realized that passing HttpClient is still better, as it allows not only to mock HTTP requests, but also advanced configuration such as custom auth headers, etc.

Also I have a few comments on your PR:

1. Do not mix multiple changes in one PR. Adding more ctors, reorganizing code and other changes you have made are out of #9 scope.
2. Do not mark existing API as "obsolete" unnecessarily. If you need specific ctors, add it, but do not "remove" existing ones.
3. Do not change/reorganize existing code unnecessarily. For example, you have changed this code:

   if (DateTime.UtcNow > _Expiration)
   {
   ...

   to this:

   DateTime now = DateTime.UtcNow;
   TimeSpan lifetime = now.Subtract(_HttpClientCreated);
   if (lifetime > HTTP_CLIENT_LIFETIME)
   {
   ...

   and I don't see any reason why it was necessary.

[sshine]

Thank you for feedback. You are right, this PR does too many things.

Considering your latest pushes to master, I am happy that my PRs motivated.

I realized that passing HttpClient is still better,

I trust that you've read How to mock HttpClient in your .NET / C# unit tests. I went down this rabbit hole once this year and my assessment of the two approaches was that injecting HttpMessageHandler was the better option.

In the meantime, however, an even better option has emerged, which is to inject an IHttpClientFactory, which admittedly does depend on Microsoft.Extensions.Http.

Why is it better? Keep an HttpClient around too short and you exhaust the socket pool. Keep an HttpClient around too long and you get DNS cache problems. Right now a computed property is being used as a specialized HttpClient factory; you are probably also aware that computed properties should be state-preserving and not compute heavy; injecting an IHttpClientFactory instead of doing the work here would both improve the HttpMessageHandler re-use by licensing it to a factory that only does this, and it would move the particular configuration of the HttpClient out of this class.

Injecting an IHttpClientFactory is more mockable and does not depend upon a public HttpClient property or method.

It appears that the secondary constructor introduced in 2536ca62fbd3f503940f18cf1f2c955a5a4c7c62 causes the potential problem that one constructor is used in testing (so that mocking HttpClient is possible) and the other is used in production. My aim with a second set of constructors was to make sure that they all derive the same code path eventually. I still think this is warranted, even though it may be outside the scope of this PR.

and I don't see any reason why it was necessary.

In one code path, _Expiration is compared before it is set.

This still appears to be the case.

[Groxan]

Yeah, I'm aware of the HttpClient specifics you mentioned and that's why RpcClient auto refreshes the HttpClient instanse.

Recently I've also read about IHttpClientFactory and under the hood it works in a similar way: auto refreshes client handler every two minutes (by default, or you can set different lifetime). I like the new approach and that it is originally designed for DI.

By the way, nothing stops you from following that approach and using TezosRpc as a typed client:

public void ConfigureServices(IServiceCollection services)
{
    services.AddHttpClient<TezosRpc>(client =>
    {
        client.BaseAddress = new Uri("https://rpc.tzkt.io/mainnet/");
        client.Timeout = TimeSpan.FromSeconds(30);
    });

    ...
}

...

public class IndexModel : PageModel
{
    readonly TezosRpc Rpc;

    public IndexModel(TezosRpc rpc)
    {
        Rpc = rpc;
    }

    public async Task OnGet()
    {
        var head = await Rpc.Blocks.Head.Hash.GetAsync<string>();
        Console.WriteLine(head);
    }
}

In one code path, _Expiration is compared before it is set.

_Expiration is initialized with default value (0000-01-01 00:00:00) when the instanse of RpcClient is created and when we access Client property first time, if (DateTime.UtcNow > _Expiration) obviously returns true, and that is exactly what we want. So, everything's fine.

[sshine]

that's why RpcClient auto refreshes the HttpClient instanse.

OK; although there is a lock keyword, this does not really guarantee against one thread disposing an HttpClient used by another thread. I would probably either prefer to know that my resource is thread-isolated, or that it isn't, rather than think that it is with the presence of lock.

nothing stops you from following that approach and using TezosRpc as a typed client

Thanks for pointing that out! This pattern is new to me, and I only saw it when discovering IHttpClientFactory.

_Expiration is initialized with default value (0000-01-01 00:00:00) [...] So, everything's fine.

If you think so, I will not argue. :-)

[Groxan]

one thread disposing an HttpClient used by another thread

Hmm, you are right... That's quite possible, especially for long-running requests. Many thanks for pointing that out! We need to think on how to fix it without removing TezosRpc(string url) ctor.