Minimal async-await foundations, updated #804

chitoyuu · 2021-10-25T15:40:55Z

This is an updated and rebased version of #709, that is no longer blocked on #757. Notable changes outside the original PR include:

TerminateInfo and InitializeInfo are moved back into the public API. These types are used as arguments to custom init/terminate callbacks. Custom callbacks are necessary for calling the gdnative_async register and terminate functions. Currently, they are just moved into the top level of gdnative_core, which I think makes sense given their usage in top-level callbacks. Please let me know if a better place is available.
Renamed the top-level re-export from asn to tasks, since the original author apparently didn't like it.

Supersedes #709

Bromeon

Thanks a lot!

TerminateInfo and InitializeInfo are moved back into the public API. [...] Currently, they are just moved into the top level of gdnative_core, which I think makes sense given their usage in top-level callbacks. Please let me know if a better place is available.

It doesn't look like they are re-exported to gdnative with the current code, or at least they don't appear in its documentation. I generate it with:

cargo doc --no-deps -p gdnative --open

Maybe we can add them to a new gdnative::nativescript::init module? Or even gdnative::init, if we're planning to dismantle the nativescript (but this could also be done separately).

Note that I commented on a few parts that were just moved to a different file (didn't see the code was left unchanged until later). Might still be worth addressing.

Regarding the async intricacies, maybe someone more proficient than me should have a look at those details? But I'm also OK with merging to collect user feedback, it's anyway feature-gated.

Also, do you plan on writing an example or book entry for async godot-rust? Or should we look at tests -- or some external source?

bors try

gdnative-core/src/lib.rs

gdnative-async/Cargo.toml

Bromeon · 2021-10-26T08:08:17Z

gdnative-async/src/method.rs

+    /// If the `spawner` object is not used, the method call will fail, output an error,
+    /// and return a `Nil` variant.


Which method call will fail? spawn_with() has no return type.

The Godot side of it. Clarified in doc-comments.

gdnative-async/src/rt.rs

gdnative-async/src/rt/bridge.rs

bors · 2021-10-26T08:41:57Z

try

Build succeeded:

full-ci

chitoyuu

It doesn't look like they are re-exported to gdnative with the current code, or at least they don't appear in its documentation.

Seems to show up for me with cargo doc. I'm not sure if it's --no-deps that changed the situation, as gdnative-core is technically a dependency. Otherwise try cargo clean maybe?

Maybe we can add them to a new gdnative::nativescript::init module? Or even gdnative::init, if we're planning to dismantle the nativescript (but this could also be done separately).

I think this is best done separately, as further discussion might be needed.

Also, do you plan on writing an example or book entry for async godot-rust? Or should we look at tests -- or some external source?

As the title of the PR suggests, this is just a minimal foundation. It's not meant to be the final user-facing API. I suppose I can do that if you'd like, but it will have to be replaced later when a higher-level API becomes available.

gdnative-core/src/lib.rs

gdnative-async/src/rt/bridge.rs

gdnative-async/src/rt.rs

chitoyuu · 2021-10-26T11:25:17Z

gdnative-async/src/method.rs

+    /// If the `spawner` object is not used, the method call will fail, output an error,
+    /// and return a `Nil` variant.


The Godot side of it. Clarified in doc-comments.

gdnative-async/Cargo.toml

Bromeon · 2021-10-26T14:41:15Z

Seems to show up for me with cargo doc. I'm not sure if it's --no-deps that changed the situation, as gdnative-core is technically a dependency. Otherwise try cargo clean maybe?

Seems to work fine now, must have missed them somehow.
Let's do the module reorg separately, then.

As the title of the PR suggests, this is just a minimal foundation. It's not meant to be the final user-facing API. I suppose I can do that if you'd like, but it will have to be replaced later when a higher-level API becomes available.

Makes sense! Also, of course it doesn't have to be you in particular providing those resources.
It's great to have a foundation on which we can build 🙂

I think we should probably also update the CI release workflow, as there's a new crate to be published. Feel free to add a separate commit to logically separate changes.

bors try

chitoyuu · 2021-10-26T14:48:06Z

Added the new crate to the workflow.

bors · 2021-10-26T14:50:29Z

try

Build succeeded:

full-ci

Bromeon · 2021-10-26T15:25:43Z

Will be merged tomorrow unless there are objections.

bors try

bors · 2021-10-26T15:33:37Z

try

Build failed:

full-ci

Bromeon · 2021-10-26T15:54:06Z

Latest CI passed, including nightly: https://github.com/godot-rust/godot-rust/actions/runs/1386329141

jacobsky

Thank you for the PR! I apologize for the late review, I've had to spread it out over a couple of days. All in all, it looks great! Thanks for all the help with this.

There is only one point that concerns me at the moment, and that's related to my comment about async being a default feature flag for gdnative. (Don't worry about changing anything yet, I just wanted to bring up the discussion).

jacobsky · 2021-10-26T23:56:17Z

gdnative/Cargo.toml

@@ -12,18 +12,20 @@ readme = "../README.md"
 edition = "2018"

 [features]
-default = ["bindings"]
+default = ["bindings", "async"]


@Bromeon Do we want to make this a default feature for all users? As this is currently a minimal foundation, I was wondering if we should make it opt-in since it will likely add a bunch of dependencies and is only necessary if someone wants to use Rust's async functionality

I agree, I missed this. Would also vote for opt-in (at least once we release v0.10).

There were already reports of very long godot-rust compile times, no need to burden people more if they don't need async functionality.

No strong feelings. Done.

Thank you very much for the changes!

jacobsky

Based on the conversation, I am adding a quick request to remove async from the default features. It may be nice to add a quick section to the readme to mention the feature-flag, but that can also be addressed separately.

The types are used as arguments to custom init/terminate callbacks, and aren't entirely internal.

This sets the foundations for async-await support in godot-rust, based on the original idea in godot-rust#284. However, although the tests work, this is not a full implementation: - Async methods can only be registered manually using `build_method`. Macro syntax and implementation are out of the scope of this PR. - The runtime types aren't registered automatically yet. Users need to manually call `register_runtime` and `terminate_runtime` functions in their library lifecycle hooks. Improving this is out of the scope of this PR for now. - The crate is currently re-exported as `gdnative::asn`, instead of the much longer `async_yield`. The name is open to discussion -- I don't like it very much. - Only local spawners are supported, due to issues with thread safety. Users may off-load tasks that don't contain `yield`-likes to thread pool spawners using something like `futures::future::Remote`, however. - Panics in async methods don't currently behave very well. Their `FunctionState`-likes simply block forever and any outstanding bridge objects for futures can be leaked.

- `set_executor` and `register_runtime` are changed to allow multiple calls, at the expense of slightly worse error messages when they aren't called as necessary. - Renamed the top-level re-export to `tasks` instead of `asn`. - Added documentation for previously undocumented public items. - Removed `async` from default features

jacobsky

Looks good to me! Thank you for all the hard work!

jacobsky · 2021-10-27T13:57:55Z

@Bromeon I'm happy with all the current changes, do you have any additional feedback or should we go ahead and merge it?

Bromeon · 2021-10-27T20:44:36Z

As discussed in Discord, let's merge it.
Thanks to https://github.com/godot-rust/book/pull/44, we also have some examples for it! 🚀

bors r+

bors · 2021-10-27T20:56:54Z

Build succeeded:

full-ci

Bromeon mentioned this pull request Oct 25, 2021

Minimal async-await foundations #709

Closed

Bromeon linked an issue Oct 25, 2021 that may be closed by this pull request

[discussion] Async-await support #284

Closed

Bromeon added c: core Component: core (mod core_types, object, log, init, ...) feature Adds functionality to the library labels Oct 25, 2021

Bromeon reviewed Oct 26, 2021

View reviewed changes

bors bot added a commit that referenced this pull request Oct 26, 2021

Try #804:

1cc9eb8

chitoyuu force-pushed the pr/709 branch from 41b62e3 to 6a542b6 Compare October 26, 2021 11:29

chitoyuu commented Oct 26, 2021

View reviewed changes

chitoyuu mentioned this pull request Oct 26, 2021

Run optional initialization/cleanup code automatically if the relevant feature is enabled #805

Open

bors bot added a commit that referenced this pull request Oct 26, 2021

Try #804:

6f4c7d2

chitoyuu force-pushed the pr/709 branch from 6a542b6 to 972854d Compare October 26, 2021 14:46

bors bot added a commit that referenced this pull request Oct 26, 2021

Try #804:

0cb8a22

jacobsky reviewed Oct 26, 2021

View reviewed changes

jacobsky suggested changes Oct 27, 2021

View reviewed changes

chitoyuu and others added 3 commits October 27, 2021 10:28

Bring TerminateInfo and InitializeInfo back to public

afc16aa

The types are used as arguments to custom init/terminate callbacks, and aren't entirely internal.

chitoyuu force-pushed the pr/709 branch from 972854d to 82f928e Compare October 27, 2021 10:28

jacobsky approved these changes Oct 27, 2021

View reviewed changes

bors bot merged commit 03c334e into godot-rust:master Oct 27, 2021

chitoyuu deleted the pr/709 branch October 27, 2021 21:44

chitoyuu mentioned this pull request Nov 10, 2022

[discussion] Async-await support #284

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimal async-await foundations, updated #804

Minimal async-await foundations, updated #804

chitoyuu commented Oct 25, 2021

Bromeon left a comment

Bromeon Oct 26, 2021

chitoyuu Oct 26, 2021

bors bot commented Oct 26, 2021

chitoyuu left a comment

chitoyuu Oct 26, 2021

Bromeon commented Oct 26, 2021

chitoyuu commented Oct 26, 2021

bors bot commented Oct 26, 2021

Bromeon commented Oct 26, 2021 •

edited

bors bot commented Oct 26, 2021

Bromeon commented Oct 26, 2021

jacobsky left a comment

jacobsky Oct 26, 2021

Bromeon Oct 27, 2021

chitoyuu Oct 27, 2021

jacobsky Oct 27, 2021

jacobsky left a comment

jacobsky left a comment

jacobsky commented Oct 27, 2021

Bromeon commented Oct 27, 2021

bors bot commented Oct 27, 2021

		/// If the `spawner` object is not used, the method call will fail, output an error,
		/// and return a `Nil` variant.

Minimal async-await foundations, updated #804

Minimal async-await foundations, updated #804

Conversation

chitoyuu commented Oct 25, 2021

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon Oct 26, 2021

Choose a reason for hiding this comment

chitoyuu Oct 26, 2021

Choose a reason for hiding this comment

bors bot commented Oct 26, 2021

try

chitoyuu left a comment

Choose a reason for hiding this comment

chitoyuu Oct 26, 2021

Choose a reason for hiding this comment

Bromeon commented Oct 26, 2021

chitoyuu commented Oct 26, 2021

bors bot commented Oct 26, 2021

try

Bromeon commented Oct 26, 2021 • edited

bors bot commented Oct 26, 2021

try

Bromeon commented Oct 26, 2021

jacobsky left a comment

Choose a reason for hiding this comment

jacobsky Oct 26, 2021

Choose a reason for hiding this comment

Bromeon Oct 27, 2021

Choose a reason for hiding this comment

chitoyuu Oct 27, 2021

Choose a reason for hiding this comment

jacobsky Oct 27, 2021

Choose a reason for hiding this comment

jacobsky left a comment

Choose a reason for hiding this comment

jacobsky left a comment

Choose a reason for hiding this comment

jacobsky commented Oct 27, 2021

Bromeon commented Oct 27, 2021

bors bot commented Oct 27, 2021

Bromeon commented Oct 26, 2021 •

edited