Accepting Timeout value from the Configuration

[TKul6]

Hi,

In my application I try to join to couple of server nodes (so if the one hard coded server is temporary down, I'm covered :))

However I've seen in Client.cs that you initializing the ConsulClient.HttpClient.Timeout with a hard coded value of 15 minutes:
HttpClient.Timeout = TimeSpan.FromMinutes(15);

(Which mean for me if one Consul node is down or have difficulties responding I need to wait 15 minutes for the result)

I was wondering why this is the value and if I can refactor this to be a configurable value from the ConsulClientConfiguration

By the way, I saw there is a Wait property in the ConsulClientCofiguration is it for consul usage?

Thank you very much.

[highlyunavailable]

Due to a limitation of how HttpClient works, the Timeout property on the HttpClient must be set to at least as long as any long poll (blocking queries) requests that you want to do, or the request will fail with an error while waiting for the long poll to complete because the request is timing out waiting for data. This means that the timeout must be set to a fairly long interval by default to support blocking queries in the general use case.

If you know that your application will not be using any blocking queries during the lifetime of a specific ConsulClient (this includes use of the Lock and Semaphore classes since they use blocking queries under the hood) there is a constructor overload that accepts a user-provided HttpClient.

You'd use it like so:

using (var hc = new HttpClient()) {
    hc.Timeout = Timespan.FromSeconds(5);
    hc.DefaultRequestHeaders.Accept.Add(
        new MediaTypeWithQualityHeaderValue("application/json"));
    // You should reuse a Client many times since it adds
    // overhead/garbage to make one per request
    using (var c = new ConsulClient(new ConsulClientConfiguration(), hc) {
        // Do your tasks
    }
}

If you provide your own HttpClient/WebRequestHandlers (e.g. for SSL validation skipping), you are responsible for disposing of them, but you can reuse a single HttpClient between multiple ConsulClients with no problem as well, if needed.

The WaitTime configuration property provides an upper bound on how long a blocking query will wait before returning data if nothing has changed. It is part of the "blocking query" functionality and maps directly to the wait parameter on the Consul HTTP API.

[highlyunavailable]

Oh, and I'm adding CancellationToken arguments to everything when I get some time to do it (see #55 and #56), so with that you could "fake" your own timeouts by awaiting and cancelling after a delay if needed, while keeping a long Timeout value. This is not possible today, though.

[TKul6]

Thank you for your answer.

I really prefer if I wouldn't have to create my own HttpClient because of several reasons:

1. I don't want to be coupled to the HttpClient because maybe ,someday, you would find it necessary to replace the implementation with other library, and I will have to change my code, which make the upgrade process a little bit nasty.
2. If I provide my own HttpClient, then somewhere in the future, Consul will ask for another header, or change the existing one, and I won't be able to just update the package, I will have to make the changes manually (copy from you 😁).

I can offer the following solution: Create another constructor which will look like this:

ConsulClient(ConsulClientConfiguration config, Action<HttpClient> overrideClient):this(config)
{
     overrideClient(HttpClient);
}

This way, you are still the owner and creator of the HttpClient and You can add / remove / change the headers and change the properties as necessary (by the way if you use this approach the if in Client.cs line 467 is redundant),and still give me the opportunity to override the default values.
And if I mess something up is my problem :)

what do you say?

Thanks a lot.

[highlyunavailable]

The approach of "pass your own HTTPClient" came from #42 because someone else wanted to do the same thing.

Your approach is very interesting and I didn't know you could even do that with Actions. It's a much cleaner way, I agree, though it does not allow one to add Handlers (e.g. for proxied requests or custom cert validation) since handlers must be provided at construction time.

I have no problem adding such a constructor, but it does not seem to cover all use cases around modifying the HttpClient due to how Handlers are added to the client. Any thoughts on those, or am I stuck with a custom HttpClient constructor?

[highlyunavailable]

Now that I look through it, I can just do the same thing with the handler, since proxy settings and such are just properties of the handler.

[highlyunavailable]

I think I managed to implement something that covers all the use cases that I've seen in 6db3fec - any feedback is nice, but it seems to be what is needed. I'd like to deprecate config object sharing in 0.7.0 as well since it's thread-unsafe to share a config and definitely unsafe to edit it after it's been "consumed" by a ConsulClient.

[TKul6]

Well I thought about only one Action but you have three, which make the code a little bit harder to test and maintain.

I consulted with a friend of mine, and I make another implementation

The solution is basically to unify all actions under one class (IConsulClientFactory) and create a base class which hold the basic implementation (BaseConsulClientFactoty).

This way the user is not aware of the internal design (which makes it really ease for you to change in time).
And of course the user can override your implementation if they wish to.

This solution is more oop and encapsulated, The actual ConsulClient does not know how the object are created and does not need to update them, it just uses them where needed.

By the way, nor the ConsulClient nor the IConsulClientFactory know about the HttpHandler, because neither of them actually uses it (So they doesn't suppose to know it exists). The only entity that known about the CreateHttpHandler action is the BaseConsulClientFactory which actually uses it in the CreateHttpClient method.

It makes the code a little bit better.

What do you think?

[highlyunavailable]

I don't like the Factory pattern here since this is going to effectively turn it back into "make your own HttpClient/HttpHandler, just by a long proxy route via a Factory class." I'm not sure how making it an Action makes it harder to test - you get type safety to tell you if you've incorrectly specified a property name or value, you still get exceptions based on setters if needed, and you can store Actions in a variable and pass them to multiple constructors, e.g.:

Action<ConsulClientConfiguration> cfgAction = (cfg) => { cfg.Datacenter = "foo"; };
using (var c = new ConsulClient(cfgAction)) {}
using (var otherc = new ConsulClient(cfgAction,
    (client) => { client.Timeout = TimeSpan.FromSeconds(30); })) {}

You could even compose them (which would be more useful in other contexts but it is possible):

Action<ConsulClientConfiguration> cfgAction1 = (cfg) => { cfg.Token = "yep"; };
Action<ConsulClientConfiguration> cfgAction2 = (cfg) => { cfg.Datacenter = "foo"; cfgAction1(cfg); };

If anything it could be easier to test/maintain because it would be more simple to mock, at least in my mind. I'd gain the ability to guarantee that the configs and clients/handlers were safe because they would not be shared between multiple objects and could not be modified after construction.

[TKul6]

I don't like the Factory pattern here since this is going to effectively turn it back into "make your own HttpClient/HttpHandler, just by a long proxy route via a Factory class."

I'm not sure why you feel that way, I'm not asking for the user to implement the IConsulClientFactory on his own. Only "special users" who wish to override the default behavior (which won't be so many) will inherit from the base class and other won't know it even exists (and they won't have to supply null on Actions they are not sure what to write in).

Moreover C# in his heart consist of OOP and object approach, we should not afraid to use objects, and if we want to change the default behavior of things inheritance and polymorphisem is the best way to go (as oppose to other languages such as javascript), is the way C# and the .Net environment support it.

And ... my approach give you the ability to expand the classes and add more feature without the user knowing that, in your approach you will have to add more and more constructors and handle a lot of Action and null check over time to expand the ConsulClient abilities.

[highlyunavailable]

My problem with the Factory approach is that effectively you'd need to create your own Factory if you wanted to make a differently configured Config object, or create a Factory that accepted and copied a ConsulClientConfiguration.

It is a very common use case to set a bunch of different stuff in the ConsulClientConfiguration class and pass it to the constructor, and the only reason it's a separate class as compared to a bunch of properties on the ConsulClient class is that I couldn't think of a way to construct multiple clients with the same properties easily. The Action method solves that and I might end up axing the entire ConsulClientConfiguration class to move all the properties into ConsulClient, and make setting them part of the constructor action for the ConsulClient to make them "readonly-esque".

[highlyunavailable]

Closing since you've got a workaround until 0.7.0 (you'll have to make your own HTTPClient for now) and we've figured out a method to allow changes post 0.7.0.

[TKul6]

Sorry for the late response, but I had to consult with my colleges to regard your answer.

The factory approach forces to create a Factory class only if you want to change something (it is NOT the common case).
Moreover, if you want to change one thing you don't need to override 3 functions but only one, so you should be aware only to the functions you want to override (as C# intended to ).

To achieve this with Actions you will have to overload the constructors in various format which will make the code messy.

Using class is more OOP solution than invoking Actions.

Actions are meant to answer the question of

"How to execute a business logic unit".

this is not your question in this case, because you don't have a business logic to execute, it does not belong to how to retrieve KV data or how to connect to agents. in those cases you really have a process that you ask the user something and the user should know and care about this.
A better question is

"How to configure a middle-ware in my system"

here you can get a Model to answer the question, be ware that you are in control of what this Model exposes and how it configures your middle-ware.

If you use an action you don't have those restrictions, The user can change a value that you do not know about and mess up your HttpClient.

It is not an encapsulated solution, and C# (as oppose to js or slimier languages) uses the power of the Objects to encapsulate data.

Please try to discuss with your colleges and ask for their opinions the matter (and if they will find me wrong please do share).

Thank you for your time.