Cannot reuse ConsulClientConfiguration for multiple clients

[playfab]

Cannot reuse ConsulClientConfiguration for multiple clients

[highlyunavailable]

This issue has been resolved and is available on Nuget as 0.6.4.3. Thanks again for reporting it!

[highlyunavailable]

Thanks for reporting this!

The reason the handler is in the ConsulClientConfiguration is because it's using the handler to do SSL certificate check ignoring and/or credentials passing based on the config values set.

The following statement does not excuse this bug, but: ideally you'd only make a single Client per configuration and reuse it for multiple requests all over your program since each request is self-contained/thread safe. That said, it is entirely expected that you should be able to reuse the config object between multiple clients.

I'll take a look once I get home tonight and see if I can figure out a way around this that makes sense. I don't really want to move the "create handler" code into the ConsulClient because the behavior right now is to update the handler directly if you edit the Configuration, though technically that would be unsafe now that I think about it since there's no locks around it and it's supposed to be sort of a create-only kind of object. I don't think recreating the handler if the config is disposed is a safe enough way to do it, because I want to make the ConsulClient object thread-safe enough that it can be reused globally, and technically you could dispose of a Client (which disposes of the Config's handler) on thread A while a Client on thread B tried to use the handler in the config - who recreates the handler then? B would try, probably, but it would be racing to see if the handler is already disposed in the first place.

I'm thinking the cleanest way around this would be to save the variables when set instead of directly updating the Handler, then create a new HttpHandler for each Client every time the config is passed to a ConsulClient's constructor. This has the side effect of "freezing" the config per client, so if you change the Config object's values, it won't change the value of the Client that's using it (e.g. changing authentication means you have to recreate all clients using that Config object) but that's a "safer" side effect than trying to manage shared state between clients, which would actually require locking of every config value - something that I don't particularly want to do, since it adds overhead to accessing the Config object.

Alternatively, I could implement the "freezing" step, and add a "changed" event and make each client listen to its associated config's change event, updating the handler on that in each Client.

If you have any further thoughts on what would be the "least surprising" way around this, given what I wrote above, I'll happily read them.

[Eibwen]

The second idea (first listed here) I alluded to would look like this, MassTransit is the main project where i've encountered this pattern, which allows for more flexibility for that since it supports multiple "wrapped" message queue services.

I think both of these are different than the ideas you suggest.

//Changes to consul client:
public ConsulClient()
{
    //Normal
}
public ConsulClient(Action<ConsulClientConfiguration> configurator)
{
    //The client controls the actual instanciation, not the consuming code
    Config = configurator(new ConsulClientConfiguration());
}

//Usage of this:
using (var client = new ConsulClient(config => config.Addres = new Uri("http://mydockermachineip:8500"))
{
    Console.WriteLine(client.Catalog.Services().Result.StatusCode);
}

The other main idea i can think of is more or less:

public class ConsulClientConfiguration
{
    WebRequestHandler _handler;
    public WebRequestHandler Handler
    {
        get
        {
            if (_handler == null || _handler.IsDisposed()) //Can you check if its disposed easily, idk?
            {
                //Actually probably a private method of CreateHandler()
                _handler = new WebRequestHandler();
                AddClientCertificateToHandler();
            }
            return _handler;
        }
    }
}

The second method here shouldn't change the public interface, but does make a lot more things you need to keep track of.

The first method here might not be familiar for some people (likely requiring some docs around it).

Those were the ideas I thought of, I definitely haven't considered all the ramifications... totally up to you of course :)

[highlyunavailable]

You can't easily tell if a handler is disposed - you'd have to wrap try {} catch (ObjectDisposedException) {RebuildHandlerAndHttpClient();} around every use of it (you have to rebuild the HttpClient that uses a disposed handler too, because the Handler is passed in during the HttpClient's constructor, it can't be edited at runtime), which means every use of the HttpClient in the ConsulClient class. The Action version looks cool but I really do not want to change the public interface at this point. I'll give it some thought - thanks for the examples.

[Eibwen]

Yeah, I believe with the Action<Config> method, you need to focus on only allowing objects that are threadsafe and can be shared being able to be set from it due to that issue.

[highlyunavailable]

@Eibwen I believe I implemented a version of what you were describing in your first implementation in 6db3fec and will cut a release next week. I do want to obsolete config object sharing in 0.7.0 since it's thread-unsafe to modify the object after the ConsulClient consumes it. You'll still be able to share configs if needed via sharing the "setup Action" delegate and calling it against multiple clients, but this is just a heads-up since you so recently requested this so you don't end up getting locked into an "old" style of doing it.