3
\$\begingroup\$

I have a few tools that run automatically. In those tools I usually run two to six jobs that run asynchronically. Sometimes when there is more data then expected or to prevent them from running indefinitely in case of a bug I cancel them.

The following example demonstrates my pattern with 90% accuracy. This is, the only parts that are left out is the actual business logic, IoC and the names of the processors are generic. Everthing else is pretty much the same.

I was wondering how good or bad this pattern is? It's been working fine for many days but maybe there is something that I missed and will give me headaches someday?


I start the application from the Main where I initialize the processors and the Timer (from System.Threading). Then I wait untill they all are done or cancel the one that might be either hanging or working with too much data at once.

void Main()
{
    var processors = new Processor[]
    {
        new Processor1(),
        new Processor2(),
        new Processor3(),
    };

    var token = new CancellationTokenSource();
    var timer = new Timer(
        callback: state => token.Cancel(),
        state: null,
        dueTime: TimeSpan.FromSeconds(5),
        period: TimeSpan.FromMilliseconds(-1));

    try
    {
        Task.WaitAll(processors.Select(p => p.Start(token.Token)).ToArray());
    }
    catch (AggregateException ex)
    {
        Console.WriteLine(ex);
    }
    finally
    {
        // When I use a try/catch already, I usually don't use 'using' to avoid deep nesting
        timer.Dispose();
        token.Dispose();
    }
}

Each processor is derived from the Processor class that provides logging for unexpected errors and awaits the core method to return.

abstract class Processor
{
    public Task Start(CancellationToken cancellationToken)
    {
        try
        {
            return Task.Run(async () => await StartCore(cancellationToken));
        }
        catch (Exception ex)
        {
            // logging
            return Task.FromException(ex);
        }
    }

    protected abstract Task StartCore(CancellationToken cancellationToken);
}

The processors themselves just do some work.

class Processor1 : Processor
{
    protected override Task StartCore(CancellationToken cancellationToken)
    {
        Console.WriteLine(GetType().Name + " started.");
        for (int i = 0; i < 6; i++)
        {
            cancellationToken.ThrowIfCancellationRequested();
            Thread.Sleep(TimeSpan.FromSeconds(1));
        }
        Console.WriteLine(GetType().Name + " finished.");
        return Task.FromResult<object>(null);
    }
}

class Processor2 : Processor
{
    protected override Task StartCore(CancellationToken cancellationToken)
    {
        Console.WriteLine(GetType().Name + " started.");
        for (int i = 0; i < 2; i++)
        {
            cancellationToken.ThrowIfCancellationRequested();
            Thread.Sleep(TimeSpan.FromSeconds(1));
        }
        Console.WriteLine(GetType().Name + " finished.");
        return Task.FromResult<object>(null);
    }
}

class Processor3 : Processor
{
    protected override Task StartCore(CancellationToken cancellationToken)
    {
        Console.WriteLine(GetType().Name + " started.");
        for (int i = 0; i < 4; i++)
        {
            cancellationToken.ThrowIfCancellationRequested();
            Thread.Sleep(TimeSpan.FromSeconds(1));
        }
        Console.WriteLine(GetType().Name + " finished.");
        return Task.FromResult<object>(null);
    }
}
\$\endgroup\$
2
\$\begingroup\$

Can't you replace

protected abstract Task StartCore(CancellationToken cancellationToken);

with

protected abstract void StartCore(CancellationToken cancellationToken); 

and call it as

Task.Run(() => {
                  try 
                  {
                      StartCore(cancellationToken);
                  }
                  catch
                  {
                     //logging
                     throw;
                  }
                });

? Why do you need async/await?


You don't really need a timer, you can use WaitAll overload that takes TimeSpan:

var tasks = processors.Select(p => p.Start(token.Token)).ToArray();
if (!Task.WaitAll(tasks, TimeSpan.FromSeconds(5)))
{
    token.Cancel();
    // wait for cancellation, if necessary
    // Task.WaitAll(tasks); 
}

You don't show your business logic and you don't say what kind of processing you are doing, so it's hard to tell whether inheritance is the best approach. It doesn't look like it based on your examples. Maybe it is better to have a single Processor class that would contain all the boiler-plate code you require (similar to BackgroundWorker) and inject StartCore as delegate/interface instead.

\$\endgroup\$
  • \$\begingroup\$ The business logic is quite complex and involves many web services etc. The processors do similar things but with different tools or from different sources etc. You can take my word, it's from a real application just stripped from the irrelevant parts because StartCore is the entry point, the are several classes and many other methods involved. Some processors are really large. They just should run in parallel and not too long and this is the part that should take care of it. \$\endgroup\$ – t3chb0t Jun 5 '17 at 16:32
  • \$\begingroup\$ @t3chb0t, I am not questioning your integrity. It just hard for me to judge this pattern of yours, without seeing it in its full glory. :) In your example Processor1 looks too much like Processor2 which is not that different from Processor3. Which is why it feels like aggregation will do a better job in this case than inheritance. And base processor itself looks too much like Task.Run(...).ContinueWith(logOnError)... \$\endgroup\$ – Nikita B Jun 5 '17 at 18:49
  • \$\begingroup\$ Those are really good suggestions ;-) I call them processors but they could also be called modules where each of them also requires a different set of dependecies injected during construction (like databases) that's why I have several of them and not only one with a method injection but I guess this is an entirely different question. This small piece should just coordinate their work so I extracted only this part of the logic. \$\endgroup\$ – t3chb0t Jun 5 '17 at 20:12
  • \$\begingroup\$ An embarassing fact about my solution: I really have it as shown in the Main but now it's going to be refactored it into something testable ;-) \$\endgroup\$ – t3chb0t Jun 6 '17 at 4:15

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.