3
\$\begingroup\$

I have implemented a simple "switch" function - that switches and invokes an alternate function, if the function that is to be invoked fails or timesout.

This code will be later employed as part of circuit breaker.

I request to review the code in general and in particular for correct use of async\await:

public class Switch<R>
    {        
        private CancellationTokenSource combinedCancelTokenSource;
        private CancellationToken cancelToken;
        private CancellationToken timeCancelToken;        
        private Func<CancellationToken, Task<R>> alternateFn;
        private Action<Exception> exceptionIntercept;

        public Switch() {
            this.combinedCancelTokenSource = new CancellationTokenSource();           
        }

        public async Task<R> InvokeAysnc(Func<CancellationToken, Task<R>> f) {

            bool computeAlternate = false;

            R r = default(R);
            Stopwatch watch = new Stopwatch();
            try {
                watch.Start();
                r = await f(combinedCancelTokenSource.Token);
                watch.Stop();
            } catch(OperationCanceledException) {
                watch.Stop();
                if (timeCancelToken.IsCancellationRequested) {
                    // log as Fault
                    logTimedOut(watch.Elapsed);
                    computeAlternate = true;
                } 
                //else - cancellation was requested by the callee, return default value, do not log as fault                
            } catch(Exception exp) {
                watch.Stop();
                exceptionIntercept?.Invoke(exp);
                logFault(watch.Elapsed, exp);
                computeAlternate = true;
            } 

            if(computeAlternate && alternateFn != null) {
                r = await alternateFn(cancelToken);
            }
            return r;
        }

        private void logFault(TimeSpan elapsed, Exception exp) {
           // will implement later
        }

        private void logTimedOut(TimeSpan elapsed) {
            // will implement later
        }

        public Switch<R> Cancellation(CancellationToken cancelToken) {
            this.cancelToken = cancelToken;
            this.combinedCancelTokenSource = combineTokens(this.cancelToken);
            return this;
        }


        public Switch<R> WithinTime(TimeSpan timespan) {
            CancellationTokenSource timeCancelTokenSource = new CancellationTokenSource();
            timeCancelTokenSource.CancelAfter(timespan);
            this.timeCancelToken = timeCancelTokenSource.Token;

            this.combinedCancelTokenSource = combineTokens(this.timeCancelToken);
            return this;
        }

        public Switch<R> InterceptException(Action<Exception> interceptor) {
            this.exceptionIntercept = interceptor;
            return this;
        }

        public Switch<R> Alternate(Func<CancellationToken,Task<R>> alternateFn) {
            this.alternateFn = alternateFn;
            return this;
        }

        private CancellationTokenSource combineTokens(CancellationToken t) {
            CancellationToken[] tokens;
            if (combinedCancelTokenSource != null) {
                tokens = new CancellationToken[] { combinedCancelTokenSource.Token, t };
            } else {
                tokens = new CancellationToken[] { t };
            }
            return CancellationTokenSource.CreateLinkedTokenSource(tokens);
        }
    }

Unit Tests:

[TestClass()]
    public class SwitchTests {

        [TestMethod]
        public void SimpleInvokeTest() {
            Switch<int> c = new Switch<int>();
            int val = 10;

            int x = c.InvokeAysnc((token) => {
                // just to test - wrap the return in a Task run, in real world we would use a async method
                return Task.Run(() => { return val; });
            }).Result;

            Assert.AreEqual<int>(val, x);
        }


        [TestMethod]
        public void OnExceptionAlternateTest() {
            int val = 10;
            int alternate = 5;

            Switch<int> c = new Switch<int>().Alternate((token) => {
                // just to test - wrap the return in a Task run, in real world we would use a async method
                return Task.Run(() => { return alternate; });
            });


            int x = c.InvokeAysnc((token) => {
                // just to test - wrap the return in a Task run, in real world we would use a async method
                return Task.Run(() => {
                    // deleberate divide by zero
                    return val/0; });
            }).Result;

            Assert.AreEqual<int>(alternate, x);
        }


        [TestMethod]
        public void OnExceptionInterceptInvokeTest() {
            int val = 10;
            int alternate = 5;

            Exception exp = null;

            Switch<int> c = new Switch<int>().Alternate((token) => {
                // just to test - wrap the return in a Task run, in real world we would use a async method
                return Task.Run(() => { return alternate; });
            }).InterceptException((e) => {
                exp = e;
            });


            int x = c.InvokeAysnc((token) => {
                // just to test - wrap the return in a Task run, in real world we would use a async method
                return Task.Run(() => {
                    // deleberate divide by zero
                    return val / 0;
                });
            }).Result;

            Assert.AreEqual<int>(alternate, x);
            Assert.IsTrue(exp is DivideByZeroException);
        }



        [TestMethod]
        public void OnTimeoutAlternateTest() {
            int val = 10;
            int alternate = 5;

            Switch<int> c = new Switch<int>().Alternate((token) => {
                // just to test - wrap the return in a Task run, in real world we would use a async method
                return Task.Run(() => { return alternate; });
            }).WithinTime(new TimeSpan(0, 0, 0, 0, 10));


            int x = c.InvokeAysnc((token) => {
                // just to test - wrap the return in a Task run, in real world we would use a async method
                return Task.Run(() => {
                    for(int i=0;i<10;i++) {
                        if(token.IsCancellationRequested) {
                            throw new TaskCanceledException();
                        }
                        Thread.Sleep(5);
                    }
                    return val;
                });
            }).Result;

            Assert.AreEqual<int>(alternate, x);
        }


        [TestMethod]
        public void OnCalleeCancelDefaultTest() {
            int val = 10;
            int alternate = 5;

            CancellationTokenSource cts = new CancellationTokenSource();
            cts.CancelAfter(2);

            Switch<int> c = new Switch<int>().Alternate((token) => {
                // just to test - wrap the return in a Task run, in real world we would use a async method
                return Task.Run(() => { return alternate; });
            }).Cancellation(cts.Token);


            int x = c.InvokeAysnc((token) => {
                // just to test - wrap the return in a Task run, in real world we would use a async method
                return Task.Run(() => {
                    for (int i = 0; i < 10; i++) {
                        Thread.Sleep(2);
                        if (token.IsCancellationRequested) {
                            throw new TaskCanceledException();
                        }                        
                    }
                    return val;
                });
            }).Result;

            Assert.AreEqual<int>(default(int),x);
        }
    }
\$\endgroup\$
1
\$\begingroup\$

I don't want to start a holy war about it but you should know that your bracing style isn't generally the expected style for C#. It is more common for the opening brace to appear on a newline:

public int GetFoo()
{
    // ...
}

According to the official naming guidelines all methods - public or otherwise - should be PascalCase


Why have you chosen R for the Type parameter? It's a small thing, but most people would expect to see T. Try to stick with convention where you can.


With that small housekeeping aside, here's some example code to highlight an issue with your code:

// set up switch
var someSwitch = new Switch<int>()
    .WithinTime(TimeSpan.FromSeconds(1))
    .Alternate(token => Task.FromResult(100));

// do some long work
Thread.Sleep(4000);

// use the switch
var result = await someSwitch.InvokeAsync(token => 
    { 
        Thread.Sleep(Timeout.Infinite);
        return Task.FromResult(1234);
    });

If I've read your code correctly, this will never return because the time based cancellation token will have already requested cancellation and you don't check the IsCancellationRequested property at all.

It would be better to store the TimeSpan in a field and set up the time based cancellation token as the first step of your InvokeAsync method.


Have you thought about how you'd reuse an instance of your switch?


I'd also rename your methods so it looks more like:

var result = await new Switch<string>()
    .WithTimeLimit(TimeSpan.FromSeconds(1))
    .WithFallbackFunction(token => Task.FromResult("Fell off a cliff"))
    .RunFunctionAsync(token => Task.FromResult("Hello world"));

InvokeAysnc should be InvokeAsync.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Congrats to reaching 10k btw although I am late to the party. \$\endgroup\$
    – Heslacher
    Jul 26 '16 at 9:55

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.