Performance - Consider static reference and pooling per service endpoint of HttpClient

[kenakamu]

Issue: #30 Implement singleton per Uri Added missing version parameter for each method

[kenakamu]

@baywet please review the change.

[baywet]

Thanks for making the changes, great start, couple of remarks:

• Because we're using a static pool, I think we should care about thread safety, Dictionary is not thread safe as far as I know, I suggest we use this guy instead https://msdn.microsoft.com/en-us/library/dd287191(v=vs.110).aspx which should address comment line 298
• Because we're building a pool of clients, we should not dispose anything at the end of the methods (get, Post...) so I think the using block statements for clients coming from the pull should be removed.
• We also should probably subscribe to the main application domain event of shutting down the application to dispose everything we have in the pool
• minor thing but L300-301 why don't we simply leverage the optional parameters functionality instead and do something like that string version = "2" L288? It'd show up in auto-completion that we default to 2 and reduce the amount of code.

[kenakamu]

Yeah, I totally forgot about eliminating using blocks. I did change by following your advice. For the application shutdown, how do we want to implement? I just did IDisposable for now.

[baywet]

I have to say I'm split on that last one. Because of the consumer sees that the ucwaclient is disposable it's going to be wrapped up in a using statement which is going to undermine our efforts. If we choose to implement that I think it should be in the auto completion doc as well as in the wiki here that for performance considerations they should not instantiate and dispose the client right away. The other option would be not to mark it as disposable and leverage app domain unload event instead. https://docs.microsoft.com/en-us/dotnet/api/system.appdomain.domainunload?view=netframework-4.7.1 What do you think?

[baywet]

Forget what I just wrote, going disposable makes more sense. This way if something crashes or if the consumer forgets something the CLR should be able to catch up and clean things. Let's document it properly, that's it.

[kenakamu]

Okay, I still agree that user may use UCWAClient in using statement which ruins our effort, but I will document it. How should we do it? Maybe adding comment in the constructor so that VS will show users how to use it?

/// <summary>
/// Instantiate UCWAClient by specifying ErrorHandling Policy.
/// Keep using the instance while you need it, and only dispose when you don't need it anymore.
/// </summary>
/// <param name="errorHandlingPolicy"></param>
public UCWAClient(ITransientErrorHandlingPolicy errorHandlingPolicy = null)
{
    if (errorHandlingPolicy != null)
        transientErrorHandlingPolicy = errorHandlingPolicy;
    Settings.UCWAClient = this;
}```

[baywet]

I'd even create a wiki page for performance considerations and add the link into the reference

[kenakamu]

Okay let's do it. Do you have permission to write Wiki by the way? I just added a page for it as place holder.

/// <summary>
/// Instantiate UCWAClient by specifying ErrorHandling Policy.
/// See https://github.com/kenakamu/UCWA2.0-CS/wiki/Performance-Considerations for performance considerations.
/// </summary>
/// <param name="errorHandlingPolicy"></param>

How about this?

[baywet]

Everybody has permissions to edit the wiki as long as they have a github account and that you have enabled the wiki on the repo. I'm on the go right now but I can probably write it later today. I can see you created the page already.

[baywet]

I updated the documentation https://github.com/kenakamu/UCWA2.0-CS/wiki/Performance-Considerations let me know what you think (Also added a section for authentication library that I'll update later) Only thing left I guess is in the sample, App Domain unloading should be implemented instead of the exit (because you can have multiple app domains per applications) ans we should be good.