Async/Await for Signals

[lilizoey]

It should be possible to implement Future for signals, and add support for async functions in exported methods, with them returning a signal.

These two things combined should allow us to use async/await syntax when calling methods both in rust and gdscript.

This is going to require a lot of upfront work before it's feasible to start working on this feature. First an actual implementation of signals are needed, but we'll also likely need to create some godot-objects directly in our bindings that will handle the awaiting and execution of these futures, and that is not something we have planned/thought out how to do yet.

[TitanNano]

I implemented a POC for a SignalFuture and minimal async runtime that allows to await signals. 5891c5b

It still has two issues which I did not bother to resolve yet:

1. it's currently not possible to spawn nested tasks. It should be possible to resolve that, but I'm not sure how important it actually is. EDIT: has been resolved.
2. Only Signal arguments that are Sync + Send can currently be used in the async context. This makes it impossible to access any Gd<T> coming from a signal.

[jrb0001]

@TitanNano I think your implementation doesn't work correctly if the Callable is called on another thread than where the task has been created.

jrb0001@74afb5b is a variant without the thread-local, but it needs some unsafe and panics when called from another thread. It should also fix recursively creating tasks, but I didn't test it.

jrb0001@a27d0bc is a third, more complex variant which has both !Send and Send futures. It can only be called by another thread if both the task and the future are Send. Otherwise it panics or doesn't even compile.

Sync isn't needed here because polling a Future needs a &mut so only one thread can access it at a time anyway.

[TitanNano]

@jrb0001 yes that is true. I ignored that so far. Have you encountered a case were the callable was called on an other thread?

jrb0001@74afb5b is a variant without the thread-local, but it needs some unsafe and panics when called from another thread. It should also fix recursively creating tasks, but I didn't test it.

I don't see the benefit of this if it still panics.

jrb0001@a27d0bc is a third, more complex variant which has both !Send and Send futures. It can only be called by another thread if both the task and the future are Send. Otherwise it panics or doesn't even compile.

Sync isn't needed here because polling a Future needs a &mut so only one thread can access it at a time anyway.

Gd<T> is neither Send nor Sync so as soon as either of them is required the future becomes much less useful since you can't access any Gd<T> inside of it.

[jrb0001]

@jrb0001 yes that is true. I ignored that so far. Have you encountered a case were the callable was called on an other thread?

No, I spotted it while reading through it.

jrb0001@74afb5b is a variant without the thread-local, but it needs some unsafe and panics when called from another thread. It should also fix recursively creating tasks, but I didn't test it.

I don't see the benefit of this if it still panics.

The panic is guaranteed while your version can run another future which was assigned the same ID on the thread of the signal.

jrb0001@a27d0bc is a third, more complex variant which has both !Send and Send futures. It can only be called by another thread if both the task and the future are Send. Otherwise it panics or doesn't even compile.

Sync isn't needed here because polling a Future needs a &mut so only one thread can access it at a time anyway.

Gd<T> is neither Send nor Sync so as soon as either of them is required the future becomes much less useful since you can't access any Gd<T> inside of it.

LocalSignalFuture of the third variant works with Gd<T> both as signal argument and when captured by a closure with the limitation that the future is !Send and panics if the signal happens on another thread. I think that solves the main usecase?

SendSignalFuture of the third variant is less useful, I agree. Maybe it makes sense to merge them and select the implementation based on the experimental-threads feature?

[TitanNano]

The panic is guaranteed while your version can run another future which was assigned the same ID on the thread of the signal.

This can easily be solved by adding the ThreadId into the Waker.

LocalSignalFuture of the third variant works with Gd both as signal argument and when captured by a closure with the limitation that the future is !Send and panics if the signal happens on another thread. I think that solves the main usecase?

But it adds a lot of complexity without solving any problems (at least as far as I'm aware). Correctly, panicking can be solved like I mentioned above.

SendSignalFuture of the third variant is less useful, I agree. Maybe it makes sense to merge them and select the implementation based on the experimental-threads feature?

Even with extermental-threads Gd<T> is still not thread-safe. Without access to engine managed objects, I don't see much value for a Send + Sync future. But we can even offer a thread-safe task spawn function which either moves the future into the Waker or uses a static (i.e. not thread-local) ASYNC_RUNTIME.

---

I have been thinking about the stated concern of signals being emitted on other threads, and I believe the correct solution would be to redirect the wakers future polling into the correct thread.
Unfortunately, this is not possible as far as I can see. Godot does not expose any way to queue callable for a specific thread. Generally, there is also too little control over where things get executed to come up with a reliable approach.

So the alternatives for now that I can see are:

a) limit async tasks to the main thread by panicking when trying to spawn a task on another thread. With this limitation in place, we can connect to signals in deferred mode inside the Signal Future and reliably run the callable on the main-thread.

b) properly panic when the callable runs on a different thread than the future and leave the rest up to the user. This is more flexible but much less predictable, so I think a) is probably preferred.

Once there is a thread-safe version of Gd<T> we can add a way to spawn Send + Sync futures and poll them on whatever thread the signal is emitted. This should be quite trivial.

[jrb0001]

There are seem to be some issues related to emitting a signal from a callable connected to itself. This will call the Callable a second time and its Mutex is already locked. From looking at Godot source, it seems that oneshot connections are disconnected after calling the Callable. So it needs to either ignore calls while the Mutex is locked or use deferred mode.

I also tried implementing futures_lite::Stream and the only way to avoid the issue there is to use deferred mode, as far as I can see.

So if deferred mode signals always run on the main thread, then that's probably the best option.

I also ran into an issue with the async-channel crate which deadlocks when the receiving Future is polled from inside the Waker (also caused by a Mutex). std::task documentation doesn't prohibit our behavior, but I wouldn't be surprised if other crates have similar issues. There can be quite some recursion if there is a chain of futures, each doing something that wakes up the next one. I think it would be better to simply avoid calling user code from inside the waker, if that's even possible.

I ended up using an Autoload with a smol::LocalExecutor, ~10 lines. Maybe there is some other way to execute code every frame, without adding a node or registering a class?

[TitanNano]

There are seem to be some issues related to emitting a signal from a callable connected to itself. This will call the Callable a second time and its Mutex is already locked. From looking at Godot source, it seems that oneshot connections are disconnected after calling the Callable. So it needs to either ignore calls while the Mutex is locked or use deferred mode.

I think this was changed in 4.3. In 4.2 it's disconnected after calling and in 4.3 it's disconnected first.

[jrb0001]

I think I found another two issues that affect all our implementations:

• Dropping the Future doesn't disconnect the Callable.
• Disconnecting the Callable causes the Future to get stuck forever.

Disconnecting requires the Callable so we need to keep it around somewhere in the Future. Which means that its internal refcount will never drop to zero so the Callable is never dropped. I don't know any other way to reliably detect a disconnected signal.

[TitanNano]

Disconnecting requires the Callable so we need to keep it around somewhere in the Future. Which means that its internal refcount will never drop to zero so the Callable is never dropped. I don't know any other way to reliably detect a disconnected signal.

We can use a WeakRef I think.

I also ran into an issue with the async-channel crate which deadlocks when the receiving Future is polled from inside the Waker (also caused by a Mutex). std::task documentation doesn't prohibit our behavior, but I wouldn't be surprised if other crates have similar issues. There can be quite some recursion if there is a chain of futures, each doing something that wakes up the next one. I think it would be better to simply avoid calling user code from inside the waker, if that's even possible.

We might want to connect to the signal in the normal way and schedule a deferred callable to actually poll the future. But I haven't tried it yet.

[jrb0001]

Disconnecting requires the Callable so we need to keep it around somewhere in the Future. Which means that its internal refcount will never drop to zero so the Callable is never dropped. I don't know any other way to reliably detect a disconnected signal.

We can use a WeakRef I think.

Isn't that only for RefCounted? Unfortunately Callable are their own type, not even an Object.

I also ran into an issue with the async-channel crate which deadlocks when the receiving Future is polled from inside the Waker (also caused by a Mutex). std::task documentation doesn't prohibit our behavior, but I wouldn't be surprised if other crates have similar issues. There can be quite some recursion if there is a chain of futures, each doing something that wakes up the next one. I think it would be better to simply avoid calling user code from inside the waker, if that's even possible.

We might want to connect to the signal in the normal way and schedule a deferred callable to actually poll the future. But I haven't tried it yet.

I had that idea too but didn't find a generic way to schedule it. There is Callable::call_deferred but that isn't exposed to gdextension as far as I can see. Maybe something like callable.to_variant().call("call_deferred", args) but not sure.

[TitanNano]

Isn't that only for RefCounted? Unfortunately Callable are their own type, not even an Object.

Yeah right I missed that.

Disconnecting requires the Callable so we need to keep it around somewhere in the Future. Which means that its internal refcount will never drop to zero so the Callable is never dropped. I don't know any other way to reliably detect a disconnected signal.

Why do you believe it will never be dropped? As soon as the future is dropped, the callable should be dropped as well, shouldn't it?

I had that idea too but didn't find a generic way to schedule it. There is Callable::call_deferred but that isn't exposed to gdextension as far as I can see. Maybe something like callable.to_variant().call("call_deferred", args) but not sure.

Looks like Callable::call_deferred is only available as a varcall and those are not implemented for built-in types. I will see if your suggestion work and we should see that Callable::call_deferred is properly implemented as a varcall.

[TitanNano]

I added the handling of the following cases to my implementation:

1. godot_task should panic if the current thread is not the main thread.
2. Waker::wake should panic if the current thread ID doesn't match the original one.
3. The future is now polled inside a deferred callable, so waking and polling are no longer happening in the same call stack. This way, signals emitted on a different thread should also be redirected to poll the future on the main thread now.
4. SignalFuture disconnects its callable if the signal hasn't been emitted when the future is dropped.

https://github.com/godot-rust/gdext/compare/7634fe769d1fcb66209586f0b6c06aac40978253...4c2d9a85a0ea29fc0ade525e8538c216cd792c34?expand=1

EDIT: added handing for nested godot_tasks and clean-up when deinitializing.
7634fe7...341a40c