33

I am looking for the correct way/structure to create a loop in a Thread/Task...

The reason for this is, i need to check the DB every 15sec for report requests.

This is what i tried so far, but i get OutOfMemoryException:

    private void ViewBase_Loaded(object sender, RoutedEventArgs e)
{
    //On my main view loaded start thread to check report requests.
    Task.Factory.StartNew(() => CreateAndStartReportRequestTask());
}

private void CreateAndStartReportRequestTask()
{
    bool noRequest = false;

    do
    {
         //Starting thread to Check Report Requests And Generate Reports
         //Also need the ability to Wait/Sleep when there are noRequest.
         reportRequestTask = Task.Factory.StartNew(() => noRequest = CheckReportRequestsAndGenerateReports());

         if (noRequest)
         {
             //Sleep 15sec
             reportRequestTask.Wait(15000);
             reportRequestTask = null;
         }
         else
         {
             if (reportRequestTask.IsCompleted)
             {
                 reportRequestTask = null;
             }
             else
             {
                 //Don't want the loop to continue until the first request is done
                 //Reason for this is, losts of new threads being create in CheckReportRequestsAndGenerateReports()
                 //Looping until first request is done.
                 do
                 {

                 } while (!reportRequestTask.IsCompleted);

                 reportRequestTask = null;
             }
         }

    } while (true);
}

private bool CheckReportRequestsAndGenerateReports()
{
    var possibleReportRequest = //Some linq query to check for new requests

    if (possibleReportRequest != null)
    {
        //Processing report here - lots of new threads/task in here as well
        return false;
    }
    else
    {
        return true;
    }
}

What am i doing wrong?

Is this correct way or am i total off?

EDIT:

Most important, my UI must still be responsive!

Willem
  • 9,166
  • 17
  • 68
  • 92
  • 2
    See http://stackoverflow.com/questions/7402146/cpu-friendly-infinite-loop/7402177#7402177 – sll Sep 19 '11 at 13:51
  • 2
    Instead of placing the loop inside of this task, you create the task in endless loop. – Alex F Sep 19 '11 at 13:53
  • 1
    You're creating who knows how many Tasks. Your code doesn't make much sense. You probably should edit and describe *exactly what you are trying to accomplish*. –  Sep 19 '11 at 13:54
  • Who tells your process when to end? The program? Or the task processing itself (i.e. once you get FALSE return)? – James Michael Hare Sep 19 '11 at 13:56
  • It's worth noting that best practice in 2021 is to use `Task.Run` not `Task.Factory.StartNew` – Liam May 06 '21 at 12:17

4 Answers4

62

Something like this would work:

var cancellationTokenSource = new CancellationTokenSource();
var task = Repeat.Interval(
        TimeSpan.FromSeconds(15),
        () => CheckDatabaseForNewReports(), cancellationTokenSource.Token);

The Repeat class looks like this:

internal static class Repeat
{
    public static Task Interval(
        TimeSpan pollInterval,
        Action action,
        CancellationToken token)
    {
        // We don't use Observable.Interval:
        // If we block, the values start bunching up behind each other.
        return Task.Factory.StartNew(
            () =>
            {
                for (;;)
                {
                    if (token.WaitCancellationRequested(pollInterval))
                        break;

                    action();
                }
            }, token, TaskCreationOptions.LongRunning, TaskScheduler.Default);
    }
}

static class CancellationTokenExtensions
{
    public static bool WaitCancellationRequested(
        this CancellationToken token,
        TimeSpan timeout)
    {
        return token.WaitHandle.WaitOne(timeout);
    }
}
Roger Lipscombe
  • 89,048
  • 55
  • 235
  • 380
  • 1
    @Roger: I like your Repeat class, I wonder if you should add an option to create the task as TaskCreationOptions.LongRunningTask to avoid hogging a thread in the threadpool – James Michael Hare Sep 20 '11 at 14:07
  • 1
    @Roger: Mind if I add your class to our bag of tricks here where I work? We have a very robust Shared Components Committee where we try to make common, reusable utility classes, and this one is wonderfully generic (in the best sense of the term). – James Michael Hare Sep 20 '11 at 14:23
  • 2
    @RogerLipscombe Sheer curiosity: how come the for(;;) insated of a while(true)? – mateuscb Apr 19 '12 at 21:47
  • 1
    @mateuscb Holdover from my C/C++ days, when while(true) would often give a warning -- "condition is always constant" or something like that. I think. – Roger Lipscombe Apr 20 '12 at 10:05
  • 1
    ...also, I find it easier to read. In while(true), you have to actually look at the word 'true'. In for (;;), the whole thing reads as "forever". – Roger Lipscombe Apr 20 '12 at 10:05
  • @RogerLipscombe I've been using your class above but I don't understand how `if (token.WaitCancellationRequested(pollInterval)) break;` works. From what I can fathom is that once the timeout expires it will return true and then break out of the loop and never execute again, can you provide a layman's guide? – Stokedout Nov 21 '15 at 19:33
  • Also @RogerLipscombe in terms of usage, if I wanted to block the host thread from continuing whilst `Repeat.Interval()` was executing should I use the returned task and call `task.Wait();` or `task.Wait(cancellationTokenSource.Token);`? – Stokedout Nov 21 '15 at 19:40
  • Try-catch the action() call, in case of miscellaneous errors when you might want to retry the action. – arviman Feb 08 '16 at 08:45
  • even just naming `WaitOne` as `WaitCancellationRequested` is great – mkb May 12 '18 at 21:36
23

Sounds like you want something like this. Please correct me if I am misinterpretting your intentions...

First, in your kick-off, set as a long running task so it doesn't consume a thread from the thread pool but creates a new one...

private void ViewBase_Loaded(object sender, RoutedEventArgs e)
{
    // store this references as a private member, call Cancel() on it if UI wants to stop
    _cancelationTokenSource = new CancellationTokenSource();
    new Task(() => CreateAndStartReportRequestTask(), _cancelationTokenSource.Token, TaskCreationOptions.LongRunning).Start();
}

Then, in your report watching thread, loop until IsCancelRequested has been set. If there is no work, just wait on the cancel token for 15 seconds (this way if cancelled will wake sooner).

private bool CheckReportRequestsAndGenerateReports()
{
    while (!_cancellationTokenSource.Token.IsCancelRequested) 
    {
        var possibleReportRequest = //Some linq query
        var reportRequestTask = Task.Factory.StartNew(() => noRequest = CheckReportRequestsAndGenerateReports(), _cancellationTokenSource.Token);

        if (noRequest)
        {
            // it looks like if no request, you want to sleep 15 seconds, right?
            // so we'll wait to see if cancelled in next 15 seconds.
            _cancellationTokenSource.Token.WaitHandle.WaitOne(15000);

        }
        else
        {
            // otherwise, you just want to wait till the task is completed, right?
            reportRequestTask.Wait(_cancellationTokenSource.Token);
        }
    }
}

I'd also be wary of having your task kick off more tasks. I have a feeling you are spinning up so many you're consuming too many resources. I think the main reason your program was failing was that you had:

     if (noRequest)
     {
         reportRequestTask.Wait(15000);
         reportRequestTask = null;
     }

This will return immediately and not wait 15s, because the thread is already complete at this point. Switching it to the cancel token (or a Thread.Sleep(), but then you can't abort it as easily) will give you the processing wait you need.

Hope this helps, let me know if i'm off on my assumptions.

Frank
  • 2,738
  • 19
  • 30
James Michael Hare
  • 37,767
  • 9
  • 73
  • 83
  • Thanks James. This sounds about right. Just read my update quick. I added some comments. – Willem Sep 19 '11 at 14:16
  • 1
    @Willem: I read your updates, I think I have my loop right in respect to them. This will basically kick off the report checker as a long-running task (thread) and inside, if there is no work created it will wait for 15s (or cancellation, whichever comes first) and then try again. If there IS work, it waits for that work to be done then repeats (until cancelled). – James Michael Hare Sep 19 '11 at 14:20
  • Thanks james. It seems to all work fine. One last problem. In `CheckReportRequestsAndGenerateReports()`, i have a `linq` query that needs to access the main `thread`. I call `this.Dispatcher.Invoke((Action)(() => { //some linq query }));` Works fine with the first request, but just stalls the app on the second request. Any idea what i am doing wrong? – Willem Sep 20 '11 at 05:53
  • @Willem: Hmmm, would need to see the code, can you post a swatch of it in an update to your question? – James Michael Hare Sep 20 '11 at 14:05
  • @Willem: Ah great! What was the issue with the dispatch? – James Michael Hare Sep 21 '11 at 13:46
  • I used `Dispatcher.Invoke`. Changed it to `Dispatcher.BeginInvoke`. Solved my problem. =) – Willem Sep 21 '11 at 15:24
  • Hands down fantastic explanation. – pim Mar 18 '16 at 02:16
  • This works great and is very adaptable - I was able to patch bits and pieces into my code and have everything run smoothly. Never would have got it to work so nicely without this. – AndrewBenjamin Mar 25 '16 at 19:50
7

I've made a work-around starting from @Roger's answer. (A friend of mine has given good advices regarding this too)... I copy it here I guess it could be useful:

/// <summary>
/// Recurrent Cancellable Task
/// </summary>
public static class RecurrentCancellableTask
{
    /// <summary>
    /// Starts a new task in a recurrent manner repeating it according to the polling interval.
    /// Whoever use this method should protect himself by surrounding critical code in the task 
    /// in a Try-Catch block.
    /// </summary>
    /// <param name="action">The action.</param>
    /// <param name="pollInterval">The poll interval.</param>
    /// <param name="token">The token.</param>
    /// <param name="taskCreationOptions">The task creation options</param>
    public static void StartNew(Action action, 
        TimeSpan pollInterval, 
        CancellationToken token, 
        TaskCreationOptions taskCreationOptions = TaskCreationOptions.None)
    {
        Task.Factory.StartNew(
            () =>
            {
                do
                {
                    try
                    {
                        action();
                        if (token.WaitHandle.WaitOne(pollInterval)) break;
                    }
                    catch
                    {
                        return;
                    }
                }
                while (true);
            },
            token,
            taskCreationOptions,
            TaskScheduler.Default);
    }
}
Juan
  • 2,156
  • 18
  • 26
4

feeling adventurous?

internal class Program
{
    private static void Main(string[] args)
    {
        var ct = new CancellationTokenSource();

        new Task(() => Console.WriteLine("Running...")).Repeat(ct.Token, TimeSpan.FromSeconds(1));

        Console.WriteLine("Starting. Hit Enter to Stop.. ");
        Console.ReadLine();

        ct.Cancel();

        Console.WriteLine("Stopped. Hit Enter to exit.. ");
        Console.ReadLine();
    }
}


public static class TaskExtensions
{
    public static void Repeat(this Task taskToRepeat, CancellationToken cancellationToken, TimeSpan intervalTimeSpan)
    {
        var action = taskToRepeat
            .GetType()
            .GetField("m_action", BindingFlags.NonPublic | BindingFlags.Instance)
            .GetValue(taskToRepeat) as Action;

        Task.Factory.StartNew(() =>
        {
            while (true)
            {
                if (cancellationToken.WaitHandle.WaitOne(intervalTimeSpan))
                    break;
                if (cancellationToken.IsCancellationRequested)
                    break;
                Task.Factory.StartNew(action, cancellationToken);
            }
        }, cancellationToken);
    }
}
ehosca
  • 945
  • 7
  • 16
  • I don't think this is a good idea. The big problem is that the task to repeat will not behave as if it was in a loop. With that code, multiple instances will be spawned in parallel, with no control on how many instances there is running at the same time. For the loop to work correctly each iteration must be started only once the previous one is completed. Also, the while(true) is consuming a thread from the ThreadPool that is essentially standing there waiting to start a new task. – erbi Jan 23 '14 at 00:47
  • there's no requirement for pipelining. OP just wants to run a task every X seconds :) – ehosca Jan 24 '14 at 03:16