Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have an IWorkflow interface defined as follows:

public interface IWorkflow
{
    Task ConfigureAsync();
    Task StartAsync();
    Task StopAsync();
}

And I have an Engine class:

public sealed class Engine : IEngine
{
    private readonly List<IWorkflow> workflows = new List<IWorkflow>();

    public Engine(IEnumerable<IWorkflow> workflows)
    {
        this.workflows.AddRange(workflows);
    }

    public void Start()
    {
        var configureTasks = this.workflows.Select(w => w.ConfigureAsync()).ToArray();
        Task.WaitAll(configureTasks);

        var startTasks = this.workflows.Select(w => w.StartAsync()).ToArray();
        Task.WaitAll(startTasks);
    }

    public void Stop()
    {
        var stopTasks = this.workflows.Select(w => w.StopAsync()).ToArray();
        Task.WaitAll(stopTasks);
    }
}

Is this the correct way for the Engine to invoke in parallel the configure method on all workflows and then once all are completed, invoke in parallel the start method on all workflows?

share|improve this question
    
Is there any reason why Engine isn't also asynchronous? –  svick Dec 4 '12 at 20:06
    
I don't see a need for it to be as there is only one Engine and it starts on the app thread. I'm updating an old .NET 2.0 app so I'm just getting to grips with Tasks and async/await & wanted to confirm I was using it in the correct way. –  Trevor Pilley Dec 5 '12 at 8:53
    
If you mean it's a GUI app, then I think not locking up the GUI while the engine starts or stops is a good reason for that. –  svick Dec 5 '12 at 8:55
    
It's a console app running in TopShelf –  Trevor Pilley Dec 5 '12 at 9:06

1 Answer 1

up vote 3 down vote accepted

Your code is absolutely correct in case when you want to start workflows only when all of them are configured.

But if you want to start each workflow once it's configured (independently from other workflows) then it might be a good idea to use continuations... In .NET 4.5 it would look like this:

public sealed class Engine : IEngine
{
    private readonly List<IWorkflow> _workflows;

    public Engine(IEnumerable<IWorkflow> workflows)
    {
        _workflows = new List<IWorkflow>(workflows);
    }

    private async Task RunWorkflow(IWorkflow workflow)
    {
        await workflow.ConfigureAsync();
        await workflow.StartAsync();
    }

    public void Start()
    {
        var startTasks = this._workflows.Select(RunWorkflow).ToArray();
        Task.WaitAll(startTasks);
    }

    public void Stop()
    {
        var stopTasks = _workflows.Select(w => w.StopAsync()).ToArray();
        Task.WaitAll(stopTasks);
    }
}

Also I would suggest to use CancellationToken to stop asynchronous processing.

UPDATE Based on comments it is really needed to wait for all workflows to be configured before starting them. So cancellable implementation can look like this:

public interface IWorkflow
{
    Task ConfigureAsync(CancellationToken token);
    Task StartAsync(CancellationToken token);
}

public sealed class Engine : IEngine
{
    private readonly List<IWorkflow> _workflows;
    private readonly CancellationTokenSource _cancellationTokenSource;
    private Task _mainTask;

    public Engine(IEnumerable<IWorkflow> workflows)
    {
        _workflows = new List<IWorkflow>(workflows);
        _cancellationTokenSource = new CancellationTokenSource();
    }

    private async Task RunWorkflows()
    {
        await Task.WhenAll(_workflows.Select(w => w.ConfigureAsync(_cancellationTokenSource.Token)));
        if (_cancellationTokenSource.IsCancellationRequested)
            return;
        await Task.WhenAll(_workflows.Select(w => w.StartAsync(_cancellationTokenSource.Token)));
    }

    public void Start()
    {
        _mainTask = RunWorkflows();
        _mainTask.Wait();
    }

    public void Stop()
    {
        _cancellationTokenSource.Cancel();
        var mainTask = _mainTask;
        if (mainTask != null)
            mainTask.Wait();
    }
}
share|improve this answer
    
The idea as it stands is to start quickly but only if all workflows configure successfully. –  Trevor Pilley Dec 4 '12 at 16:00
    
Then your solution is correct. Take into account that Engine.Start method will finish only all workflows are configured and started. –  almaz Dec 4 '12 at 16:22
    
I think your suggestion to use CancellationToken is not right. Cancellation is something exceptional (and awaiting a canceled Task throws an exception). Stopping some long-running process is not exceptional. –  svick Dec 4 '12 at 20:09
    
@svick CancellationToken is a recommended way to stop asynchronous processing, it does not throw exceptions and is not something exceptional. You probably confuse it with Thread.Abort, CancellationToken.ThrowIfCancellationRequested or Task.Wait(CancellationToken) which do throw exceptions. See the link I've included in my post for example of usage. –  almaz Dec 4 '12 at 20:47
1  
But ThrowIfCancellationRequested() is the recommended way of using CancellationToken, AFAIK. Also, Task has a special status Canceled and when you Wait() or await such Task, it will throw an exception. –  svick Dec 4 '12 at 22:55

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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