Factory pattern discussion & feedback

[CodeBlanch]

The other day I was porting some code to .NET Core which consumes a WCF service one of our partners stood up. The System.ServiceModel library works great, but I was finding myself painted into a corner when it came to managing the ChannelFactory, ClientBase, and options reloading. Basically lots of code where it didn't belong newing up stuff and doing configuration. I'm a fan of how HttpClientFactory manages HttpClients, so I took a stab at porting that pattern to WCF. End result is here: Macross.ServiceModel.Extensions

My thinking was: Just because it's a port from .NET Framework doesn't mean we can't use it in a .NET Core-ish way! Anyway, this issue is to gather some feedback from the experts.

Example:

[ServiceContract]
public interface ILegacyProductProxy
{
    [OperationContract]
    Task<int> GetStatusAsync();
}

public class ProductService : ILegacyProductProxy
{
    private readonly ILogger<ProductService> _Logger;
    private readonly SoapClient<ILegacyProductProxy> _SoapClient;

    public ProductService(ILogger<ProductService> logger, SoapClient<ILegacyProductProxy> soapClient)
    {
        _Logger = logger ?? throw new ArgumentNullException(nameof(logger));
        _SoapClient = soapClient ?? throw new ArgumentNullException(nameof(soapClient));
    }

    public Task<int> GetStatusAsync() => _SoapClient.Channel.GetStatusAsync();
}

public void ConfigureServices(IServiceCollection services)
{
    services.AddSoapClient<ILegacyProductProxy, ProductService>(()
        => new ChannelFactory<ILegacyProductProxy>(
            new BasicHttpBinding(),
            new EndpointAddress("http://localhost/LegacyService/")));
}

[StephenBonikowsky]

@CodeBlanch Our expert in this area would be @mconnew We'll provide some feedback as soon as we can.

[mconnew]

I'll be honest, there are some major flaws with your design. The biggest is around lifetime management of channels and factories. A channel factory as well as a channel should be closed when you are finished with them for multiple reasons. A channel needs to be closed because with some transports such as NetTcp, the channel has a persistent socket connection to the server. They should also be explicitly opened when the code which is calling the service operations doesn't own the factory or channel. The reason being there's a behavior when multiple threads make a call on a channel which has been opened where the calls are made sequentially and will continue to be made sequentially until there are no more outstanding calls. At which point WCF will allow multiple concurrent calls.
A ChannelFactory might need to open and/or close a token provider. This is more of an issue when using TransportSecurityWithMessageCredentials, but you could have a problem with lingering resources being kept around with some configurations if you don't properly open and close your channel factory.
You have a lifetime problem with your ConfigureChannelFactory extension method. If one delegate causes the ChannelFactory to be opened (e.g. by calling CreateChannel for some reason), a subsequent delegate which tries to modify the ClientCredentials will get an exception because the credentials object has been made read-only. If you must have an extension method which passes the high level ChannelFactory to a delegate, you should validate the state after it's returned to make sure it's still in the Created state and throw an exception if a delegate has messed with it. The better option would be to have specific methods to modify specific pieces. For example, pass the behaviors collection, or pass the ClientCredentials as a ref (so it can be replaced by the delegate), and check if it has been replaced and replace it in the ChannelFactory afterwards.
One of the advanced use cases you show is to create a new ChannelFactory if you want to connect to a different endpoint. WCF has this ability built in. You can call ChannelFactory<TChannel>.CreateChannel(new EndpointAddress(urlStr));.
I think you are throwing too many ideas into the pot at once. I would suggest splitting up the adapter concept from the factory concepts. You could have an adapte similar to this:

public interface IServiceAdapter<TService, TAdaptedService> : TAdaptedService, IDisposable { }

public class ProductServiceAdapter : IServiceAdapter<ILegacyProductProxy, IProductService>
{
    public ProductServiceAdapter(ILegacyProductProxy legacy) { _legacy = legacy; }
    public async Task EnsureStatus(int status)
    {
        if (await _legacy.GetStatusAsync().ConfigureAwait(false) != status)
            throw new InvalidOperationException();
    }
    public void Dispose() { (_legacy as IDisposable)?.Dispose(); }
}

The adapter doesn't need to know anything about the SoapClient class, it's irrelevant. You add this to DI as a transient type:

public void ConfigureServices(IServiceCollection services)
{
    services.AddTransient<IServiceAdapter<ILegacyProductProxy,IProductService>,ProductServiceAdapter>();
}

You could add it to DI as the type IProductService but you might want to expand the interface such as add a method or property which returns the legacy implementation interface.
The builder pattern doesn't generally get used in the ConfigureServices method, it's used in the Configure method. ConfigureServices is for adding all the general purpose types needed to be consumed by the builder which fetches all the necessary components plumbs everything together. The fact that you are providing so much of the configuration in ConfigureServices is an indicator of having the wrong abstraction. The concrete binding, the endpoint address etc should all be in the Configure method. the problem you are running into is you want to add items to DI which are already plumbed together. You should have extension methods for use in the Configure method which take configuration delegates, bindings, endpoints etc and in the implementation requests the various services which you can provide these things to in order to create the the class that your app needs.

[CodeBlanch]

@mconnew Wow thank you for the detailed response!

• Adapter pattern: Good idea I will pull that out of the lib and use it in apps where I need it.
• Feedback about passing the behavior collection & ClientCredentials by ref: Good idea I will churn on that.
• DI feedback: One of my goals was to say as close to HttpClientFactory as I could. Feedback seems to be at odds with that API, going to stew on this a bit.

OK lifetime issues!

My main goal with this work was to make the ChannelFactory effectively a singleton and then have Channel/SoapClient be a transient service that is spun up when a service asks for it and closed/disposed automatically at the end of the transient scope when it is through. Singleton for performance, managed-channel for convenience. It sounds like you are saying that it is defective by design and I should take it out behind the barn and do the needful? :) The Channel/SoapClient & its consuming service are both transient registrations so each service instance will get its own dedicated Channel/SoapClient from the singleton-ish factory. Even then, no good?

[StephenBonikowsky]

@CodeBlanch @mconnew will get back to you on your last question, but it may be a little while before he can get to it.