0

I've been tinkering through some stuff, and this code sample made me wonder - should I implement the StopCalculation method with a Task, or the way it's been done is perfectly okay, and actually recommended?

Thanks!

public class TesterSeventeen : MemoryStream
{
    Task task;
    bool stopper;
    public TesterSeventeen() : base()
    {
        task = new Task(() =>
        {
            while (!stopper)
            {
                Console.WriteLine("Generate work.");
                Thread.Sleep(2000);
            }
        });
    }

    public void StartCalculation()
    {
        task.Start();
    }
    public bool IsTaskDisposed()
    {
        return task.IsCompleted;
    }
    public void StopCalculation()
    {
        new Thread(() =>
        {
            Console.WriteLine("***Doomsday thread started.***");
            Console.WriteLine(task.Status);
            stopper = true;
            task.Wait();
            task.Dispose();
            Console.WriteLine(task.Status);
            Console.WriteLine("***Doomsday thread brought apocalypse.***");
        }).Start();
    }
}
class Program
{

    static async Task Main(string[] args)
    {
        using (var mem = new TesterSeventeen())
        {
            mem.StartCalculation();
            Thread.Sleep(100);
            mem.StopCalculation();
        }
        // Even though we've exited the using block, the thread will continue its work
        while (true)
        {
            // Manually exit program, after observations
            Console.WriteLine("hm");
            Thread.Sleep(100);
        }
}
SpiritBob
  • 2,355
  • 3
  • 24
  • 62
  • 1
    Simple answer: prefer `Task` – aybe Oct 25 '19 at 12:51
  • 2
    Possible duplicate of [Task vs Thread differences](https://stackoverflow.com/questions/13429129/task-vs-thread-differences) and specifically see Jon Skeet's [answer](https://stackoverflow.com/a/13429164/1797425) – Trevor Oct 25 '19 at 12:59
  • 1
    From what I can see in your code you need neither thread nor task for the `StopCalculation` as it does nothing special which would deserve a separate thread. – Dmytro Mukalov Oct 25 '19 at 14:12
  • @DmytroMukalov It serves as a fire and forget method. Otherwise you're blocking the main execution thread for the rest of the Thread.Sleep time. 1-2 seconds here and there - and it gets quite a lot. – SpiritBob Oct 25 '19 at 14:34
  • Never, ever, ever use the task constructor. You should read the documentation for how tasks should be used and then read up on cancellationtokens. The given code gives me goosebumps. Particularly with concurrent code it's best to keep it as high level and simple as possible which excludes threads from the get go (yes there are exceptions, but by the time you can argue about those you shouldn't worry about general guidelines anymore). – Voo Oct 25 '19 at 15:43
  • 1
    Such design is totally flawed. If you create a task you should use the task facilities to await for its completion not a fire&forget thread or an additional task. – Dmytro Mukalov Oct 25 '19 at 17:14
  • 1) Declare the `stopper` field as [`volatile`](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/volatile), otherwise your loop may never end. 2) Avoid misleading yourself and anyone else that will maintain your code. The name `IsTaskDisposed` is not appropriate for a property that returns if the task is completed. *Complete* and *Dispose* are different concepts. 3) Learn about [`Task.Run` with async delegates](https://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-using.html), so that you don't block unnecessarily your thread-pool threads. – Theodor Zoulias Oct 25 '19 at 17:48
  • @DmytroMukalov could you show me a quick example of using the task facilities? – SpiritBob Oct 28 '19 at 07:31
  • @Voo where exactly is this documentation you refer about? I've read the following msdn page: https://learn.microsoft.com/en-us/dotnet/standard/parallel-programming/task-based-asynchronous-programming which showcase what I've been doing, as well as some other alternatives, like Task.Run – SpiritBob Oct 28 '19 at 07:36
  • @TheodorZoulias in my case above, if I use async delegates to introduce Task.Delay, instead of Thread.Sleep, if I use `await Task.Delay(2000)`, aren't I essentially blocking the thread again, or it's being reused throughout the time it's waiting for that task's result? If it's not being reused whilst waiting, I feel like I'm adding more overhead than helping the thread-pool. – SpiritBob Oct 28 '19 at 07:51
  • I also feel like if I changed the task's method to be async, just to use Task.Delay, I'm introducing an async wrapper to a synchronous code? https://devblogs.microsoft.com/pfxteam/should-i-expose-asynchronous-wrappers-for-synchronous-methods/ or am I mistaken? – SpiritBob Oct 28 '19 at 08:20
  • When the line `await Task.Delay(2000)` is reached, a continuation is registered in the task scheduler, and the current thread is returned to the ThreadPool immediately. No thread is wasted. Regarding your concerns about introducing an async wrapper to a sync code, this is only the case if you are writing library code, and you are faking asynchrony by `Task.Run`ing sync code. This is discouraged because it misleads the users of the library in believing that are calling block-free code, while they aren't. It is OK to use Task.Run in application code to ofload work, because no one is misleaded. – Theodor Zoulias Oct 28 '19 at 08:59
  • @TheodorZoulias thank you so much for your answer! After gathering your input and reading a few other questions, I went ahead and changed my code to use Timers, which I believe are designed to handle cases such as mine. If you feel like I'm doing something wrong, let me know! http://volatileread.com/utilitylibrary/snippetcompiler?id=125364 – SpiritBob Oct 28 '19 at 09:18
  • Timers behave a little differently. They are invoked in a strict timeline, and they ignore the duration of each job. This opens the possibility that some day in the future your job will need more than 1000 msec to complete, and then you'll have overlapping concurrent jobs! The "Delay-in-a-loop" pattern is safer in that regard. – Theodor Zoulias Oct 28 '19 at 09:42

1 Answers1

0

use the token and simply cancel the task.

msdn explanation and example: https://learn.microsoft.com/en-us/dotnet/standard/parallel-programming/task-cancellation

and rethink the terminology. you are not stopping the task, you are irretrievably cancelling it. stopping suggests that the task may be started again, and stopped again, etc...

Belisarius
  • 31
  • 1
  • 5