C# : Unable to use async functions

godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine

https://godotengine.org

MIT License

91.14k stars 21.19k forks source link

C# : Unable to use async functions #63725

Closed tvardero closed 1 year ago

tvardero commented 2 years ago

Originally posted on https://github.com/godotengine/godot/issues/63379

Godot version

v3.4.4.stable.mono.official.419e713a2

System information

Win10Pro x64 19044.1826

Issue description

Installed frameworks: .Net Core 3.1, .Net 6.0

Waiting for async function to complete makes Godot to freeze.

~~Making the async function to run synchronously fixes the freeze. However, next _Process won't be called before previous _Process call is finished.~~

Steps to reproduce

Create a blank project with a simple Node. Paste this code to Node.cs:

public class Node : Godot.Node
{
public override void _Ready()
{
    GD.Print($"_Ready started: {DateTime.Now:HH:mm:ss.fffffff}");

    // Task.WaitAll(AsyncFunction());

    GD.Print($"_Ready is done: {DateTime.Now:HH:mm:ss.fffffff}");
}

public override void _Process(float delta)
{
    GD.Print($"_Process started: {DateTime.Now:HH:mm:ss.fffffff}");

    // Task.WaitAll(AsyncFunction());

    GD.Print($"_Process is done: {DateTime.Now:HH:mm:ss.fffffff}");
}

async Task AsyncFunction()
{
    await Task.Delay(1000);
}
}

Uncomment any of the Task.WaitAll(AsyncFunction()).
If you launch the game, you will never see any output in console.
~~If you change await Task.Delay(1000); to Task.Delay(1000).Wait();, then game does not freezes, but you can see that _Process executes once per second.~~ (UPD: it is expected behavior)

Minimal reproduction project

No response

Expected behaviour:

Async call is executed correctly.
_Process is called on every frame, and not on previous _Process execution completion. If i exceed thread pool - new tasks will be queued until thread pool has available threads again. Also, game should throw a warning in console telling about tasks overflow. (UPD: it is expected behavior)

tvardero commented 2 years ago

Feels like the game is limited to one thread. And calling async function does not create a new thread for them to run. But I'm no rocket scientist.

Delpire commented 2 years ago

One thing you can do is mark _Ready and _Process as async. This would allow you to await your function.

    public override async void _Ready()
    {
        await AsyncFunction(nameof(_Ready));
    }

    public override async void _Process(float delta)
    {
        await AsyncFunction(nameof(_Process));
    }

    private async Task AsyncFunction(string from)
    {
        GD.Print($"{from} started: {DateTime.Now:HH:mm:ss.fffffff}");
        await Task.Delay(1000);
        GD.Print($"{from} done: {DateTime.Now:HH:mm:ss.fffffff}");
    }

That gives you an output like this:

 _Ready started: 09:04:12.6299530
_Process started: 09:04:12.9159556
_Process started: 09:04:12.9389541
_Process started: 09:04:12.9409560
_Process started: 09:04:12.9839554
_Process started: 09:04:12.9990295
_Process started: 09:04:13.0160016
_Process started: 09:04:13.0400184
_Process started: 09:04:13.0529537
_Process started: 09:04:13.0679521
_Process started: 09:04:13.0839523
_Process started: 09:04:13.1003782
_Process started: 09:04:13.1154533
_Process started: 09:04:13.1344427
_Process started: 09:04:13.1514419
_Process started: 09:04:13.1664473
_Process started: 09:04:13.1825179
_Process started: 09:04:13.2000907
_Process started: 09:04:13.2179628
_Process started: 09:04:13.2540909
_Process started: 09:04:13.2580794
_Process started: 09:04:13.2662315
_Process started: 09:04:13.2825248
_Process started: 09:04:13.3013643
_Process started: 09:04:13.3169513
_Process started: 09:04:13.3336522
_Process started: 09:04:13.3506511
_Process started: 09:04:13.3666522
_Process started: 09:04:13.3836507
_Process started: 09:04:13.4006505
_Process started: 09:04:13.4166511
_Process started: 09:04:13.4328320
_Process started: 09:04:13.4492157
_Process started: 09:04:13.4657736
_Process started: 09:04:13.4830115
_Process started: 09:04:13.4994036
_Process started: 09:04:13.5167150
_Process started: 09:04:13.5327594
_Process started: 09:04:13.5507117
_Process started: 09:04:13.5658226
_Process started: 09:04:13.5827017
_Process started: 09:04:13.5992486
_Process started: 09:04:13.6154722
_Process started: 09:04:13.6325569
_Process started: 09:04:13.6511577
_Process started: 09:04:13.6671567
_Ready done: 09:04:13.6825460
_Process started: 09:04:13.6855127
_Process started: 09:04:13.7005344
_Process started: 09:04:13.7285514
_Process started: 09:04:13.7425458
_Process started: 09:04:13.7485336
_Process started: 09:04:13.7652179
_Process started: 09:04:13.7832908
_Process started: 09:04:13.8727067
_Process started: 09:04:13.8746726
_Process started: 09:04:13.8826794
_Process started: 09:04:13.8997189
_Process started: 09:04:13.9157310
_Process done: 09:04:13.9326830
_Process done: 09:04:13.9417004
_Process done: 09:04:13.9516860

_Process will continue to get called continuously.

tvardero commented 2 years ago

@Delpire Your _Process started execution before _Ready finished it's job. Ouch.

That's because return type of your async functions is void, which is a really bad practice (+ is a code smell). Why: https://stackoverflow.com/questions/45447955/why-exactly-is-void-async-bad Only event handlers are allowed to be async void.

_Ready function should never be async, because it results in race condition (as we already see this in your debug console).

Because we can't turn neither our _Ready nor _Process to return Task instead of void, and also to evade race condition, we need to run async tasks synchronously to execute our code correctly. This is what we are unable to because of the bug.

Delpire commented 2 years ago

@tvardero Yeah I wasn't suggesting that it was a good practice. I just misunderstood your post. I though you wanted your method to run asynchronously.

MatthijsMud commented 2 years ago

An alternative to marking the callbacks as async is to invoke Task.Run. This is however essentially the same as marking the method as async, so that doesn't really help much… (but it does cause the code to be run asynchronously, unlike .Wait())

public override void _Ready()
{
  Task.Run(async () => {
    GD.print($"_Ready started: {DateTime.Now:HH:mm:ss.fffffff}");
    await Task.Delay(TimeSpan.FromSeconds(1));
    GD.print($"_Ready finished: {DateTime.Now:HH:mm:ss.fffffff}");
  });
}

Besides that, one could consider the methods _Ready, _Process, _PhysicsProcess, etc. as event handlers; thus "allowing" them to be marked as async void. But that is more of a philosophical "fix" that still does nothing to solve the issue of _Process starting before _Ready has finished.

There are however a few options you could consider, with their upsides and downsides:

Pausing the node: Save the node's PauseMode, and set it to PauseModeEnum.Stop before performing the async logic. When finished, restore the PauseMode. This should prevent _Process and _PhysicsProcess from being called until initialization has finished. This property can however be set externally, so it could happen that _Process gets called before the instance is actually ready.
```
public override async void _Ready()
{
var restorePoint = PauseState;
PauseMode = PauseModeEnum.Stop;

GD.print($"_Ready started: {DateTime.Now:HH:mm:ss.fffffff}");
await Task.Delay(TimeSpan.FromSeconds(1));
GD.print($"_Ready finished: {DateTime.Now:HH:mm:ss.fffffff}");

PauseMode = restorePoint;

}
// ...
```
Polling relevant running tasks: Keep a reference to the tasks that need to be completed first, and check whether Status/IsCompleted has the required value. If not, exit early. This essentially treats the node as paused. Slight downside is that the state still gets checked long after establishing it is no longer needed, the JIT might be able to optimize this.
```
private Task? initialization;
public override void _Ready()
{
initialization = Task.Run(async () => { /* ... */ });
}

public override void _Process()
{
// `null` indicates `_Ready` hasn't been called yet.
if (initialization?.IsCompleted != true) return; 

// Whatever needs to happen after initialization.
}
```

When giving an asynchronous implementation for the update related methods (_Process, _PhysicsProcess, _Input, etc.), make sure to handle the state from the previous operation appropriately; not doing so could result in a race condition when the tasks finish in a different order than they were started.

As such, I would recommend keeping a reference to a task running in the background, and polling them in the relevant update methods in addition to other work that need to be done each frame.

JohnWordsworth commented 2 years ago

I'm not sure if this extra information is helpful, but I have been starting to port a console application I have written to Godot as a hobby project and I use async/await a lot in that code. In the longer-run, I intend to thread it a lot better and have a lot of the http calls run in the background - but I have been getting a lot of freezing issues in Godot around use of async/await.

However, I noticed that calling await task.ConfigureAwait(false); on the task before the await line seems to fix the issue for me (at least, in my tests this evening). For instance, this example allows you to turn on/off the freezing behaviour and have each 'Process' take 1 second (not that I want that outcome, but I do want the method to wait and not freeze forever).

public partial class Node2D : Godot.Node2D
{
    [Export]
    public bool FreezeApp = false;

    public override void _Ready() { }

    public override void _Process(double delta)
    {
        GD.Print($"_Process started: {DateTime.Now:HH:mm:ss.fffffff}");
        Task.WaitAll(AsyncFunction());
        GD.Print($"_Process is done: {DateTime.Now:HH:mm:ss.fffffff}");
    }

    async Task AsyncFunction()
    {
        Task task = Task.Delay(1000);

        if (!FreezeApp)
        {
            await task.ConfigureAwait(false);
        }

        await task;
    }
}

In my own code, as I still share the code between the console application and the new application, I am using the following (and I set the flag in the Godot application when it launches);

    public static class GodotHacks
    {
        public static bool GodotWaitHack = false;

        public static async void SafeWaitHack(this Task task)
        {
            if (GodotWaitHack)
            {
                await task.ConfigureAwait(false);
                await task;
            }
            else
            {
                await task;
            }
        }
    }

Not sure if this helps anyone, but thought I would share as I've been digging into it and trying to understand the issue!

tvardero commented 2 years ago

Still crashes (deadlocks to be precise) with Godot 4.0 beta 3

using System;
using System.Threading.Tasks;
using Godot;

public partial class Test : Node
{
  public override void _Ready()
  {
    System.Console.WriteLine($"Ready started at {DateTime.Now:HH:mm:ss.fffff}.");

    // Uncomment any line of next two line:
    // Task.WaitAll(AsyncFunction());
    // AsyncFunction().Wait();

    System.Console.WriteLine($"Ready finished at {DateTime.Now:HH:mm:ss.fffff}.");
  }

  public override void _Process(double delta)
  {
    System.Console.WriteLine($"Process started at {DateTime.Now:HH:mm:ss.fffff}.");

    // Uncomment any line of next two line:
    // Task.WaitAll(AsyncFunction());
    // AsyncFunction().Wait();

    System.Console.WriteLine($"Process finished at {DateTime.Now:HH:mm:ss.fffff}.");
  }

  private async Task AsyncFunction()
  {
    await Task.Delay(1000);
  }
}

Current viable workaround is polling for task completion as proposed by @MatthijsMud.

MatthijsMud commented 2 years ago

Stephen Cleary's blog post about not blocking on async code seems to be relevant to the topic at hand, so I'd recommend reading it. Especially the point about using ConfigureAwait(false), which is quoted below.

Using ConfigureAwait(false) to avoid deadlocks is a dangerous practice. You would have to use ConfigureAwait(false) for every await in the transitive closure of all methods called by the blocking code, including all third- and second-party code. Using ConfigureAwait(false) to avoid deadlock is at best just a hack).

The time between start and finish seems to be in line with what one would expect in case of _Ready, taking slighlty longer than one second; this could be explained with scheduling overhead. Why the same does not apply for _Process is a bit puzzling. Could it be that the code did not get uncommented in that run?

tvardero commented 2 years ago

Why the same does not apply for _Process is a bit puzzling. Could it be that the code did not get uncommented in that run?

You were right, sorry @MatthijsMud

njdf18 commented 1 year ago

Game will still freeze in Godot 4 Beta 8.

tvardero commented 1 year ago

All of this because we have synchronization context that is one threaded. (probably)

Use of ConfigureAwait(false) on Task.Delay(...) ~fixes the issue.~ Edit: is a workaround. Why and how: https://devblogs.microsoft.com/dotnet/configureawait-faq/ To understand this, you need to know what is SynchronizationContext and how compiler sees async methods.

In other words, Godot behaves the same as UI thread in WinForms applications.

It makes sense for physics update (_Process), they in theory should not be run on different threads nor in parallel. Imagine two balls running at each other at high speeds and colide with a bounce. If we were running in parallel here, each ball had calculated collision with other and in result bounce force would be duplicated. It would be a bug for a developer of the game.

It also makes sense for _Ready function for obvious reasons: parent scene waits for all children scenes of itself, and also wait for scene that is lower in scene tree.

Calinou commented 1 year ago

This is likely worth mentioning in the documentation, e.g. on the C# basics page. Can you open a pull request there?

tvardero commented 5 months ago

Years have passed and with a little gained knowledge I have come to a conclusion that UI thread (the one that calls _Ready func) should be blocked with MyTask().GetAwaiter().GetResult();. This will not schedule the task on current synchronization context, the task will be executed on Thread Pool's thread - no deadlocking.

Calling MyTask().Wait() can be done only if awaits inside MyTask are configured(false) and with that the user should be careful.

Long polling the task for it's completion, or checking this with a timer - don't. It is not efficient and makes your code more messy. The result will be the same - blocking the code with Thread.Sleep(100) or calling GetAwaiter().GetResult()

There are no other good way to await async task in sync method. Making _Ready async void is dangerous and wrong - engine (caller of _Ready) will newer know that and will continue initializing other nodes - and that will break consistency as _Ready in fact have not yet finished.

About this thread and some missconceptions that were written here, in this issue ticket:

ConfigureAwait is not bad and should not be awoided. But should be used wisely. You need to understand how tasks work under the hood.
async void are still bad, but they have their usage - event handling. This could go for button clicks, area collision, etc. However, as the call is not awaited, godot will never unwrap exceptions there. Do not treat _Ready, _Input and _Process as events in this sense - they should always block and engine should not continue if they did not finish fully. Otherwise you might see node not yet ready when _Process have started executing.
Using async methods in _Process is really really bad. This results not only in bad performance (thread pool exhaustion), but in a massive memory leak (tasks allocation on heap). Process is called every tick, and without propper conditioning, calling async method there every tick might crash your game with OutOfMemory. Same goes for _Input, as input events are sent every tick too (echo events for example).

And I'm still not a guru in C#, I'm still learning and make mistakes. If you see that I'm wrong then say it.