8
\$\begingroup\$

I am creating a service (FaultTolerantCommunication) that wraps a call to a (web) service. Its responsibility is to switch the endpoint URL in the event of a TimeoutException.

Currently the web service is a soap web service but I am attempting to make it reusable for other remote services such as a JSON based web service, for example.

I am looking for a review on the design. I have not really done any defensive programming yet in terms of argument validation and null checking but please feel free to suggest improvements on this too.

The idea is for the client code to call the service as below:

I would probably extract an interface from FaultTolerantCommunication and inject an instance using an IoC container.

Client Code:

var faultTolerantCommunication = new FaultTolerantCommunication<FooRequest, FooResponse, ISoapClient>(this.serviceCaller);
FooResponse response = faultTolerantCommunication.Request(request, this.soapClient);

Fault Tolerant Communication

    /// <summary>
    /// Fault Tolerant Communication. Recurses to fall over web service endpoints in event of timeout.
    /// </summary>
    public class FaultTolerantCommunication<TRequest, TResponse, TClient> 
        where TRequest : FooRequest, new()
        where TResponse : FooResponse, new()
        where TClient : ISoapClient
    {
        private readonly Configuration config;
        private readonly FooIntegration FooIntegration;
        private readonly ExceptionLogger exceptionLogger;
        private TClient client;
        private readonly SoapClientSwitcher<TClient> switcher;

        public FaultTolerantCommunication(FooIntegration FooIntegration)
        {
            // Read the config during construction.
            config = new ConfigReader().Read();

            // Constructor inject FooIntegration
            this.FooIntegration = FooIntegration;

            // Exception Logger
            this.exceptionLogger = new ExceptionLogger("FaultTolerantCommunication");

            // Generic SoapClient
            this.client = default(TClient);

            // Soap Client Switcher
            switcher = new SoapClientSwitcher<TClient>(config);
        }

        /// <summary>
        /// Do a single request.
        /// </summary>
        /// <param name="request">The Request</param>
        /// <param name="client">The Client</param>
        /// <returns>The Response</returns>
        private TResponse DoRequest(TRequest request, TClient client)
        {
            // Set the endpoint soap client.
            FooIntegration.ChangeEndPoint(client);

            // Call the service
            FooResponse response = this.FooIntegration.DoTransaction(request);

            // Return the response
            return (TResponse)response;
        }

        /// <summary>
        /// Recursively fall back to secondary and stand-in endpoints on TimeoutException
        /// </summary>
        /// <param name="request">The Request</param>
        /// <param name="client">The Client</param>
        /// <returns>The Response</returns>
        public TResponse Request(TRequest request, TClient client)
        {
            try
            {
                return DoRequest(request, client);
            }
            catch (TimeoutException timeoutException)
            {
                // Log the time out
                exceptionLogger.DbLogException(timeoutException);

                // Change endpoint
                this.client = switcher.Switch(this.client);
                FooIntegration.ChangeEndPoint(client);

                // Recurse
                return Request(request, client);
            }
        }
    }

Soap Client Switcher

internal class SoapClientSwitcher<TClient> where TClient : ISoapClient
{
    private readonly Configuration config;

    internal SoapClientSwitcher(Configuration config)
    {
        this.config = config;
    }

    internal TClient Switch(TClient client)
    {
        if (client is SoapClient)
        {
            client = (TClient)new SoapClientFactory().GetSecondarySoapClient(config);
            return client;
        }

        if (client is SecondarySoapClient)
        {
            client = (TClient)new SoapClientFactory().GetStandInSoapClient(config);
            return client;
        }

        if (client is StandInSoapClient)
        {
            client = (TClient)new SoapClientFactory().GetPrimarySoapClient(config);
            return client;
        }

        return client;
    }
}

ISoapClient

public interface ISoapClient : IClient<string, string>
{
    string EndPointUrl { get; set; }
}

IClient

public interface IClient<TRequest, TResponse>
{
    TResponse Transaction(TRequest transaction);
}

Soap Client Factory

public class SoapClientFactory
{
    private ISoapClient soapCient;

    public ISoapClient GetPrimarySoapClient(Configuration configuration)
    {
        soapCient = new SoapClient(configuration);
        return soapCient;
    }

    public ISoapClient GetSecondarySoapClient(Configuration configuration)
    {
        soapCient = new SecondarySoapClient(configuration);
        return soapCient;
    }

    public ISoapClient GetStandInSoapClient(Configuration configuration)
    {
        soapCient = new StandInSoapClient(configuration);
        return soapCient;
    }
}

Soap Client

public class SoapClient : ISoapClient
{
    private wsSoapClient client;
    private string password;

    public SoapClient(Configuration configuration)
    {
        // Some Config reading which is omitted for brevity

        EndpointAddress address = SetEndPoint(settings);
        client = new wsSoapClient(binding, address);
    }

    protected virtual EndpointAddress SetEndPoint(AppSettingsSection settings)
    {
        EndpointAddress address = new EndpointAddress(settings.Settings["EndpointUrl"].Value);
        return address;
    }

    public string Transaction(string transaction)
    {
        Console.WriteLine("Calling real soapClient");
        return client.Transaction(transaction, password);
    }

    public string EndPointUrl { get; set; }
}
\$\endgroup\$
3
  • \$\begingroup\$ What happens with Request if all endpoints fail? You should at least limit the recursion level. Timeouts can occur even if the endpoint is still 'up' due to heavy load - switching every new request to the next endpoint will most likely bring that one down too and so on....Some sort of load-balancing, maybe detect if the time it takes to get a reply degrades, might help remove some of the timeouts. Do all endpoint have the same 'capacity'/ability to handle the same number of clients? \$\endgroup\$
    – bkdc
    Mar 12 '14 at 11:28
  • \$\begingroup\$ If all endpoints fail it keeps switching until one of them works. In theory, one of the endpoints should be always up. They are endpoints I do not control so I cannot comment on their ability to handle clients. I like your idea about timing replies. Thanks. \$\endgroup\$
    – Sam Leach
    Mar 13 '14 at 10:32
  • 1
    \$\begingroup\$ I get your point about "keep trying" but I don't think that recursion is the way to go - think how "nice" a 60 levels deep call-stack looks; I'd use a do-while loop instead. Also, retrying until "one endpoint works" is OK only for small/infrequent timeouts but if your machine loses network connectivity, you might retry for a (very) long time and, potentially worse, block the calling code while you do that - you should at least build-in the option to retry for up to X seconds. Or implement some async mechanism and give the calling code the chance to abort the call. \$\endgroup\$
    – bkdc
    Mar 13 '14 at 17:30
5
+50
\$\begingroup\$
  1. For this:

    public TResponse Request(TRequest request, TClient client)
    {
        try
        {
            return DoRequest(request, client);
        }
        catch (TimeoutException timeoutException)
        {
            // Log the time out
            exceptionLogger.DbLogException(timeoutException);
    
            // Change endpoint
            this.client = switcher.Switch(this.client);
            FooIntegration.ChangeEndPoint(client);
    
            // Recurse
            return Request(request, client);
        }
    }
    

    If I'm right

    return Request(request, client);
    

    should be

    return Request(request, this.client);
    

    Are you sure that the Request method needs the TClient parameter at all? The class has a client field but it is never used. I guess you should use the field here and remove the method parameter.

    Furthermore, FooIntegration.ChangeEndPoint(client) looks unnecessary here, since the called Request method also calls FooIntegration.ChangeEndPoint(client).

  2. If you're planning to unit-test your code, dependencies, like new ConfigReader() and new ExceptionLogger() will make it harder because you can't use mocked/dummy implementations with predefined values/behaviour. I'd have a Configuration and an ExceptionLogger etc. constructor parameter instead and create these objects in a factory method.

  3. This method actually doesn't switch anything:

    internal TClient Switch(TClient client)
    

    It just returns the next client. The client parameter could be called currentClient while the return value is the nextClient. I would name the method getNextClient.

  4. FaultTolerantCommunication does not seem thread-safe. (If you are planning to use it from multiple threads you should handle that.)

  5. client = (TClient)new SoapClientFactory().GetSecondarySoapClient(config);
    return client;
    

    It's unnecessary to change the value of this local variable right before the function returns. The following is the same:

    return (TClient) new SoapClientFactory().GetSecondarySoapClient(config);
    

    (You might don't need the casting too.)

  6. Comments like this are rather noise:

    // Read the config during construction.
    config = new ConfigReader().Read();
    
    // Constructor inject FooIntegration
    this.FooIntegration = FooIntegration;
    
    // Exception Logger
    this.exceptionLogger = new ExceptionLogger("FaultTolerantCommunication");
    

    What they say is obvious from the code itself. I'd remove them. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

  7. Usage of this. doesn't seem consistent:

    config = new ConfigReader().Read();
    this.FooIntegration = FooIntegration;
    this.exceptionLogger = new ExceptionLogger("FaultTolerantCommunication");
    this.client = default(TClient);
    switcher = new SoapClientSwitcher<TClient>(config);
    

    If not necessary I'd remove them. I'm not familiar with C# IDEs but if they can show fields and local variables with different colors this. is usually unnecessary.

  8. I'd use lowercase first letter for the field name here:

    private readonly FooIntegration FooIntegration;
    

    It would be consistent with the other fields and wouldn't be the same as the type.

  9. The soapClient field is never read:

    public class SoapClientFactory
    {
        private ISoapClient soapCient;
    
        public ISoapClient GetPrimarySoapClient(Configuration configuration)
        {
            soapCient = new SoapClient(configuration);
            return soapCient;
        }
        ...
    }
    

    You could use a local variable or just return:

    public ISoapClient GetPrimarySoapClient(Configuration configuration)
    {
        return new SoapClient(configuration);
    }
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.