SafeFireAndForget is not safe

[Szer]

https://github.com/brminnick/AsyncAwaitBestPractices/blame/main/README.md#L140
This line says that:

`SafeFireAndForget` allows a Task to safely run on a different thread while the calling thread does not wait for its completion.

That couldn't be true, because implementation doesn't switch threads at all

AsyncAwaitBestPractices/Src/AsyncAwaitBestPractices/SafeFireAndForgetExtensions.shared.cs

Lines 77 to 90 in d7ae190

	static async void HandleSafeFireAndForget<TException>(ValueTask valueTask, bool continueOnCapturedContext, Action<TException>? onException) where TException : Exception
	{
	try
	{
	await valueTask.ConfigureAwait(continueOnCapturedContext);
	}
	catch (TException ex) when (_onException != null || onException != null)
	{
	HandleException(ex, onException);
	if (_shouldAlwaysRethrowException)
	throw;
	}
	}

Everything prior first await will be executed on caller thread because that's how C# asyncs work (e.g. F# asyncs are different)

Code below will block caller thread for 5 sec before printing, which demonstrate that SafeFireAndForget isn't actually safe.

static async Task BadTask()
{
    Thread.Sleep(5000);
    await Task.CompletedTask;
}

static void Main(string[] args)
{
    BadTask().SafeFireAndForget();
    Console.WriteLine("Hello World!");
}

[giammin]

Thread.Sleep(5000); is blocking the caller

in async await you should use Task.Delay(5000);

this works as expected

        static void Main(string[] args)
        {
            Console.WriteLine("Hello World!");
            BadTask().SafeFireAndForget();
            Console.WriteLine("Hello World!");
            Console.ReadLine();
        }
        static async Task BadTask()
        {
            await Task.Delay(5000);
            Console.WriteLine("Hello World!");
        }

[brminnick]

I agree that this is true, however, I'm not sure how to fix it. It may just be a limitation of .NET.

Expanding on your example, even if we use async/await to call await BadTask(), it will still run synchronously and Thread.Sleep will still block the thread. (I.e. In the provided example, SafeFireAndForget has the same behavior as await):

static Task BadTask()
{
    Thread.Sleep(5000);
    return Task.CompletedTask;
}

static async Task Main(string[] args)
{
    await BadTask();
    Console.WriteLine("Hello World!");
}

[brminnick]

If anyone has ideas on how to best resolve this Issue, please do share them!

If it turns out that we cannot resolve this issue, I'll likely convert this to a Discussion so that we can continue the conversation.

[Szer]

@brminnick adding Task.Yield() at the start will push everything below to thread pool

 static async void HandleSafeFireAndForget<TException>(ValueTask valueTask, bool continueOnCapturedContext, Action<TException>? onException) where TException : Exception 
 { 
     try 
     { 
         await Task.Yield();
         await valueTask.ConfigureAwait(continueOnCapturedContext); 
     } 
     catch (TException ex) when (_onException != null || onException != null) 
     { 
         HandleException(ex, onException); 
  
         if (_shouldAlwaysRethrowException) 
             throw; 
     } 
 } 

[brminnick]

I had the same thought, and tried adding await Task.Yeild() last night, but because of the way .NET handles Tasks, the Task is run synchronously until the first await keyword is reached.

This means that HandleSafeFireAndForget doesn't execute until the Task is executed. In other words, HandleSafeFireAndForget executes when the first await is reached.

For example, in the following code, Thread.Sleep executes synchronously, before await Task.Yield(); is reached in HandleSafeFireAndForget:

Task.Run(() => 
{
    Thread.Sleep(5000);
    await Task.Delay(1000); // This is when HandleSafeFireAndForget executes
}.SafeFireAndForget();

Here's another example, using a Unit Test:

Expected: not equal to 12
But was: 12

[Test]
public async Task SafeFireAndForget_NonAsyncMethodThreadTest()
{
    //Arrange
    Thread initialThread, workingThread, finalThread;
    var threadTCS = new TaskCompletionSource<Thread>();

    //Act
    initialThread = Thread.CurrentThread;

    NonAsyncMethod().SafeFireAndForget();

    finalThread = Thread.CurrentThread;

    workingThread = await threadTCS.Task;

    //Assert
    Assert.IsNotNull(initialThread);
    Assert.IsNotNull(workingThread);
    Assert.IsNotNull(finalThread);

    Assert.AreEqual(initialThread.ManagedThreadId, finalThread.ManagedThreadId);
    Assert.AreNotEqual(initialThread.ManagedThreadId, workingThread.ManagedThreadId);
    Assert.AreNotEqual(finalThread.ManagedThreadId, workingThread.ManagedThreadId);

    async Task NonAsyncMethod()
    {
        threadTCS.SetResult(Thread.CurrentThread);
        await Task.Delay(1000);
    }
}

[Szer]

In current design it's not possible without API break, because with any call like this one:
SomeTask().SafeFireAndForget();
We will always start SomeTask first and it will execute whatever it can inside that task synchronously on the caller thread, and only after it will reach continuation (first await) and return Task object it will be passed to SafeFireAndForget extension method.

The only way to fix eagerness of C# Tasks is to wrap them in thunk like

SafeFireAndForget(() => SomeTask);

This will introduce breaking change in API (Func<ValueTask> valueTask instead of ValueTask valueTask)

 static async void HandleSafeFireAndForget<TException>(Func<ValueTask> valueTask, bool continueOnCapturedContext, Action<TException>? onException) where TException : Exception 
 { 
     try 
     { 
         await Task.Yield();
         await valueTask().ConfigureAwait(continueOnCapturedContext); 
     } 
     catch (TException ex) when (_onException != null || onException != null) 
     { 
         HandleException(ex, onException); 
  
         if (_shouldAlwaysRethrowException) 
             throw; 
     } 
 } 

That way it will definitely execute Task.Yield or Task.Run first, jump to thread pool and then proceed safely.

It's not my call to make, but maybe just add "even more safer" API? :)

[SIDOVSKY]

I thought that the purpose of this API is to provide an exception-safe and explicit way to forget about async (not background/parallel) task execution and nothing more.

Fire-and-forget is an alternative to await and should not be safe against thread-blocking because it's a responsibility of a task itself.

Modifying existing logic with thread switching under the hood will break the compatibility for applications that fire-and-forget tasks that interact with the UI thread.

Adding SafeFireAndForgetOnBackground may be a solution because it clearly indicates the threading aspect.
However, I dont think it's necessary because users which want to additionally switch the thread can just:

Task.Run(BadTask).SafeFireAndForget();

[Szer]

I thought that the purpose of this API is to provide an exception-safe and explicit way to forget about async (not background/parallel) task execution and nothing more.

That's why I've mentioned what's written in README.md:

SafeFireAndForget allows a Task to safely run on a different thread while the calling thread does not wait for its completion.

It's just not true :)

[geometrikal]

This just got me big time today.

Read a blog post a few years ago that recommended SafeFireAndForget and I've often used it.

Did not realise that the Task only returns once the first await is called. So obvious, but yea I missed that thinking it was some secret sauce to firing a task off in a background thread.

So maybe there could be a little sentence for the less experienced so they don't run into this gotcha, even though a proper understanding of async / await would not necessitate it.