0

Consider the below code:

private Task _task;

private void M()
{
    _task = Task.Factory.StartNew(() => {
        // Do work
        _task = null;
    }, TaskCreationOptions.LongRunning);
}

I know that Task.Factory.StartNew creates a new Task and schedules it for execution, then returns that Task. I'm wondering whether the above logic is faulty. Is there a chance that the return value of Task.Factory.StartNew will be assigned to _task later than the execution of the passed in lambda?

Or is there some logic implemented in StartNew that prevents this?

I can certainly imagine a scenario where a context switch happens inside the StartNew, executing the passed in lambda setting _task to null, then the return value overriding it with the non null Task instance. Am I correct in this assumption?

Szabolcs Dézsi
  • 8,743
  • 21
  • 29
  • 4
    Why do you need to do this? – Owen Pauling Aug 24 '17 at 15:26
  • https://stackoverflow.com/questions/5985973/do-i-need-to-dispose-of-a-task – Owen Pauling Aug 24 '17 at 15:33
  • 1
    The only thing you can achive with such code is generate a bug. You are erasing the placeholder that would receive the result of the operation. Might as well get rid of the assignment altogether – Panagiotis Kanavos Aug 24 '17 at 15:34
  • 1
    *Why* are you storing the task in a global field? A task isn't a thread. It's a promise that you'll get a result at some point in the future. When that completes, you can execute more code with `ContinueWith` or after an `await` call. There is no reason to store the initial promise anywhere. If `M()` is called multiple times that field will get overwritten anyway – Panagiotis Kanavos Aug 24 '17 at 15:36
  • The reason I'm asking this is the following. Assume this method can be called multiple times, but any point in time I only ever want one `Task` to be running. So by having a null check before the assignment I'm wondering if this would achieve that. – Szabolcs Dézsi Aug 24 '17 at 15:38
  • @PanagiotisKanavos What about using a Task as a _light_ thread to do some background work? Kick off and forget? – Dennis Kuypers Aug 24 '17 at 15:41
  • @SzabolcsDézsi The best way to solve that problem is for `M` to add a continuation to `_task` rather than overwriting it with a new copy, although the specifics of how you go about doing that will depend on the exact semantics you want to have, you'd also need to use some synchronization mechanisms to make sure that the operations happen atomically. – Servy Aug 24 '17 at 15:43
  • @DennisKuypers a task is not a thread. It may get scheduled to run on a thread or not (eg IO tasks). You can schedule 100 tasks and use just 1 thread to run them. Or you can make 100 async calls with HttpClient and not use even 1 thread. Fire and forget isn't a light thread. It's a task that you schedule but never wait for it to finish – Panagiotis Kanavos Aug 24 '17 at 15:43
  • @DennisKuypers if you want a fire-and-forget task, call `Task.Run()` without assigning the result anywhere. That's what `forget` means – Panagiotis Kanavos Aug 24 '17 at 15:45

2 Answers2

1

This code does have a race condition. StartNew is going to schedule the opeartion to run, then it will return a Task, then the assignment to _task happens. Because of that, the task's work might, or might not, have reached the _task assignment in its body before the assignment of the return value of StartNew, so after running that code _task could be either null or the returned task, we have no way of knowing.

Servy
  • 202,030
  • 26
  • 332
  • 449
-1

If you don't need the task itself and instead want a "am i busy" state maybe the following pattern would be more appropriate

private bool _busy;

private void M()
{
    if(_busy)
    {
        return;
    }
    _busy = true;
    Task.Factory.StartNew(() => {
        try
        {
            // Do work
        }
        finally
        {
            _busy = false;
        }
    }, TaskCreationOptions.LongRunning);
}

Depending on the context and thread safety guarantees your code makes you have to take more measures...

Dennis Kuypers
  • 546
  • 4
  • 16
  • Yes, this occured to me and probably I will do something similar. My question was mostly theoritical, can the above scenario happen or not? Thank you for this answer. People are really cutthroat here :) – Szabolcs Dézsi Aug 24 '17 at 15:41
  • 2
    You have not properly synchronized access to this state that you're sharing between threads, so this is not a safe implementation either, in addition to not actually answering the question asked. – Servy Aug 24 '17 at 15:42
  • I have added the code to check for the bool. If `M()` is only called by one thread then the worst case is that the work finished, but the Task didn't reset the bool in time. Like I said, if the code guarantees thread safety then more measures have to be taken. Assuming that this is only called by one thread...we're fine. – Dennis Kuypers Aug 24 '17 at 15:47
  • @Servy I gave an answer taking the comments into consideration. He wants to make sure that only one task is active at at time. – Dennis Kuypers Aug 24 '17 at 15:48
  • @DennisKuypers And you didn't do it correctly, and you also didn't answer the question being asked. Both of those things are problems. – Servy Aug 24 '17 at 15:49
  • There are two serious problems with this. First, it's vulnerable to race conditions. Without locking, it's quite possible to start two tasks. Second, it means that `M()` is acting like a task scheduler itself. Why do that (in a buggy way) instead of using a task scheduler with limits? Better yet, why not use an ActionBlock ? Or a Lazy ? – Panagiotis Kanavos Aug 24 '17 at 15:49
  • .NET has a lot of low and high level features and classes that allow buffering jobs, throttling tasks, evaluating in a lazy manner. – Panagiotis Kanavos Aug 24 '17 at 15:50
  • @PanagiotisKanavos Put aside the question of "answering the question" and tell me how to improve https://stackoverflow.com/questions/45866138/run-background-work-but-ensure-that-this-can-only-run-once-at-any-given-time – Dennis Kuypers Aug 24 '17 at 15:55