CODEWITHSUNDEEP
c#synchronizationtask
Ben Rubin
Ben Rubin

Reputation: 7341

Correct Way to do Task Synchronization?

Is what I'm doing below the correct/best way to accomplish this?

I have a window with a timer. Each time the timer ticks, I call the RunTask method shown below. Within RunTask, I call DoTheThing. DoTheThing may take a while to run, and may fail (it's a database update). I want to make sure that at any point in time, I only have one DoTheThing outstanding. I also want to make sure that I don't have a bunch of RunTask instances all queued and waiting for a lock to be released by the RunTask instance that is running DoTheThing.

public void RunTask()
{
    bool canRunTask = true;

    // Check if another instance of this method is currently executing.  If so, do not execute the rest of this method
    lock (this.runTaskLock)
    {
        if (this.isTaskRunning)
        {
            canRunTask = false;
        }
        else
        {
            this.isTaskRunning = true;
        }
    }

    // Call DoTheThing if another instance is not currently outstanding
    if (canRunTask)
    {
        try
        {
            Task task = new Task(() => DoTheThing());
            task.Start();
        }
        catch (Exception ex)
        {
            // Handle the exception
        }
        finally
        {
            lock (this.runTaskLock)
            {
                this.isTaskRunning = false;
            }
        }
    }
}

Because of the architecture of the program, I would rather put all of my thread synchronization within this method instead of enabling and disabling the timer.

Upvotes: 1

Views: 2489

Answers (2)

spender
spender

Reputation: 120498

By thinking about the problem slightly differently, it becomes a lot easier. Instead of firing a timer every x seconds, why not wait x seconds between invocations?

Now you can just run an async loop to do the scheduled work and save yourself a bunch of painful synchronization work.

async Task RunActionPeriodicallyAsync(Action action, 
                           TimeSpan ts, 
                           CancellationToken token = default(CancellationToken))
{
    while(!token.IsCancellationRequested)
    {
        action();
        await Task.Delay(ts, token);
        //or alternatively (see comment below)
        //var delayTask = Task.Delay(ts, token);
        //action();
        //await delayTask;
    }
}

Now, just call RunActionPeriodicallyAsync once, and calls to its action will never overlap.

RunActionPeriodicallyAsync(() => DoSomething(), TimeSpan.FromSeconds(10))

You could overload this to take an async "action"... actually a Func<Task>...

async Task RunActionPeriodicallyAsync(Func<CancellationToken, Task> actionAsync, 
                           TimeSpan ts, 
                           CancellationToken token = default(CancellationToken))
{
    while(!token.IsCancellationRequested)
    {
        await actionAsync(token);
        await Task.Delay(ts, token);
        //or alternatively (see comment below)
        //await Task.WhenAll(actionAsync(token), Task.Delay(ts, token))
    }
}

and use it:

RunActionPeriodicallyAsync(async cancTok => await DoSomethingAsync(cancTok), 
                           TimeSpan.FromSeconds(10))

Upvotes: 3

wertzui
wertzui

Reputation: 5730

If you are worried about too much locking, you can do the following. You might miss a run if one task completes while the other is just at the check (marked), but you got rid of some locking and you will only need to lock when you set isTaskRunnung = true. In Addition you need to mark your method as async so you can await the task.

public async Task RunTask()
{
    bool canRunTask = true;

    // Check if another instance of this method is currently executing.  If so, do not execute the rest of this method
    if (this.isTaskRunning)
    {                                       // <-- ___MARK___
        canRunTask = false;
    }
    else
    {
        lock (this.runTaskLock)
        {
            if (this.isTaskRunning)
            {
                canRunTask = false;
            }
            else
            {
                    this.isTaskRunning = true;
            }
        }
    }

    // Call DoTheThing if another instance is not currently outstanding
    if (canRunTask)
    {
        try
        {
            await Task.Run(() => DoTheThing());
        }
        catch (Exception ex)
        {
            // Handle the exception
        }
        finally
        {
            this.isTaskRunning = false;
        }
    }
}

Upvotes: 1

Related Questions