Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Async Signals #1043

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

TitanNano
Copy link
Contributor

@TitanNano TitanNano commented Feb 9, 2025

This has been developed last year in #261 and consists of two somewhat independent parts:

  • A Future for Signal: an implementation of the Future trait for Godots signals.
  • Async runtime for Godot: a wrapper around Godots deferred code execution that acts as a async runtime for rust futures.

The SignalFuture does not depend on the async runtime and vice versa, but there is no point in having a future without a way to execute it.

For limitations see: #261 (comment)


TODOs

  • Decide if we want to keep the GuaranteedSignalFuture. Should it be the default?
  • Documentation
  • figure out async testing.

CC @jrb0001 because they provided very valuable feedback while refining the POC.
Closes #261

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1043

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, this is very cool!

From the title I was first worried this might cause many conflicts with #1000, but it seems like it's mostly orthogonal, which is nice 🙂

I have only seen the first 1-2 files, will review more at a later point. Is there maybe an example, or should we just check tests?

Comment on lines +33 to +35
if os.get_thread_caller_id() != os.get_main_thread_id() {
panic!("godot_task can only be used on the main thread!");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We typically use Rust's thread APIs for this, see e.g. here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that, but it needs #1045

godot-core/src/builtin/async_runtime.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/async_runtime.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/async_runtime.rs Outdated Show resolved Hide resolved
@TitanNano TitanNano force-pushed the jovan/async_rt branch 3 times, most recently from 2877010 to 9687f3b Compare February 10, 2025 21:19
@Bromeon Bromeon added the feature Adds functionality to the library label Feb 10, 2025
@jrb0001
Copy link
Contributor

jrb0001 commented Feb 10, 2025

I am currently testing it with my project.

  • Executor from this PR and signals from my old implementation (based on async_channel) seems to work ingame.
  • My old executor (based on async_task running once per frame) and signals from this PR is the next step, hopefully tomorrow.
  • Both Executor and signals from this PR will come after that. I expect some issues with recursive signals but let's see.
  • I am getting a weird segfault on hotreloading with a completely useless backtrace which didn't happen with my executor implementation. I need to debug this more, but I suspect it is related to having a tool node spawn a future which listens on its signals and/or a signal>drop>signal>drop something else>signal chain.

@lilizoey
Copy link
Member

* I am getting a weird segfault on hotreloading with a completely useless backtrace which didn't happen with my executor implementation. I need to debug this more, but I suspect it is related to having a tool node spawn a future which listens on its signals and/or a signal>drop>signal>drop something else>signal chain.

i'd guess it's related to using thread_local here which we need to do some hacky stuff to support with hot-reloading enabled

@TitanNano
Copy link
Contributor Author

i'd guess it's related to using thread_local here which we need to do some hacky stuff to support with hot-reloading enabled

Shouldn't the hot-reload hack only leak memory? 🤔

@jrb0001 does the segfault occur on every hot-reload?

@jrb0001
Copy link
Contributor

jrb0001 commented Feb 11, 2025

i'd guess it's related to using thread_local here which we need to do some hacky stuff to support with hot-reloading enabled

Shouldn't the hot-reload hack only leak memory? 🤔

@jrb0001 does the segfault occur on every hot-reload?

I am not completely sure yet. It doesn't happen if there are no open scenes or if none of them contains a node which spawns a Future.

It also doesn't seem to happen every single time if I close all scenes and then open one with a Future before triggering the hot-reload. In this case it panics with some scenes:

ERROR: godot-rust function call failed: <Callable>::GodotWaker::wake()
    Reason: [panic]  Future no longer exists when waking it! This is a bug!
  at /home/jrb0001/.cargo/git/checkouts/gdext-3ec94bd991a90eb6/2877010/godot-core/src/builtin/async_runtime.rs:271

With another scene it segfaults in this scenario.

Simply reopening the editor (same scene gets opened automatically) and then triggering a hot-reload segfaults for both scenes.

With both executor + Future from this PR, the hot-reload issue doesn't happen at all?!? So the issue could also be in my code, let me debug it properly before you waste more time on it.

I will do some more debugging later this week (probably weekend).


I also finished testing the Future part of the PR and it works fine with both my old executor and your executor in my relatively simple usage.

Unfortunately all my complex usages (recursion, dropping, etc.) need a futures_lite::Stream which I can't implement on top of your GuaranteedSignalFuture without potentially missing (or duplicating?) some signals while reconnecting with a new Future instance.

The R: Debug bound on to_future()/to_guaranteed_future() was a bit annoying and doesn't seem to be used? Or did I miss something?

@TitanNano
Copy link
Contributor Author

The R: Debug bound on to_future()/to_guaranteed_future() was a bit annoying and doesn't seem to be used? Or did I miss something?

Yeah, it's completely unnecessary now. Probably an old artifact. I removed the bound.


Unfortunately all my complex usages (recursion, dropping, etc.) need a futures_lite::Stream which I can't implement on top of your GuaranteedSignalFuture without potentially missing (or duplicating?) some signals while reconnecting with a new Future instance.

Can you elaborate what the issue here is?


I'm also curious what your use-case for the GuaranteedSignalFuture is. Currently, I'm still thinking to get rid of it again. I have never come across a future that resolves when the underlying source disappears, and I wonder if it is really that useful for most users. But maybe you can share how it's important for you.

@TitanNano
Copy link
Contributor Author

TitanNano commented Feb 12, 2025

ERROR: godot-rust function call failed: <Callable>::GodotWaker::wake()
    Reason: [panic]  Future no longer exists when waking it! This is a bug!
  at /home/jrb0001/.cargo/git/checkouts/gdext-3ec94bd991a90eb6/2877010/godot-core/src/builtin/async_runtime.rs:271

@jrb0001 Do you have an idea what could have triggered this? The only thing that I can think of is that a waker got cloned and reused after the future resolved. The panic probably doesn't make any sense, since the waker can technically be called an infinite number of times. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Async/Await for Signals
5 participants