Abstract async runtimes #1848

malivix · 2024-03-31T14:04:24Z

Changes

Close #1846 (EDITed by @fzyzcjy to use "Close" to make github auto trigger correlation)

Checklist

An issue to be fixed by this PR is listed above.
New tests are added to ensure new features are working. Please refer to this page to see how to add a test.
./frb_internal precommit --mode slow (or fast) is run (it internal runs code generator, does auto formatting, etc).
If this PR adds/changes features, documentations (in the ./website folder) are updated.
CI is passing. Please refer to this page to see how to solve a failed CI.

Remark for PR creator

./frb_internal --help shows utilities for development.
If fzyzcjy does not reply for a few days, maybe he just did not see it, so please ping him.

welcome · 2024-03-31T14:04:27Z

Hi! Thanks for opening this pull request! 😄

malivix · 2024-03-31T14:10:07Z

@fzyzcjy can you check you are happy with approach then I need to fix some stuff about cargo.lock also how user can spawn or spawn_local ...

fzyzcjy · 2024-03-31T14:45:39Z

@malivix Looked at the code very briefly and the rough approach seems OK to me!

codecov · 2024-03-31T20:45:20Z

Codecov Report

Attention: Patch coverage is 69.56522% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 97.69%. Comparing base (b6300cd) to head (e3a0938).

Files	Patch %	Lines
frb_rust/src/rust_async/io/tokio.rs	68.88%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1848      +/-   ##
==========================================
- Coverage   99.27%   97.69%   -1.58%     
==========================================
  Files         359      359              
  Lines       15127    15119       -8     
==========================================
- Hits        15017    14771     -246     
- Misses        110      348     +238

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fzyzcjy

Good job! Mainly some nits

fzyzcjy · 2024-04-02T00:45:59Z

frb_example/flutter_via_create/ios/Runner.xcodeproj/project.pbxproj

@@ -362,6 +362,7 @@
 				ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon;
 				CLANG_ENABLE_MODULES = YES;
 				CURRENT_PROJECT_VERSION = "$(FLUTTER_BUILD_NUMBER)";
+				DEVELOPMENT_TEAM = FX7V3ATKWH;


nit: Is this your development team? Anyway just run ./frb_internal generate-run-frb-codegen-command-integrate --package frb_example/flutter_via_create (and same command for frb_example/flutter_via_integrate) and it will be fixed

fzyzcjy · 2024-04-02T00:48:04Z

frb_rust/Cargo.toml

 user-utils = ["dep:android_logger", "dep:oslog"]
 dart-opaque = ["dep:dart-sys-fork"]
+tokio-runtime = ["dep:tokio"]
+async-std-runtime = ["dep:async-std"]


nit: seems nuclei is not there

fzyzcjy · 2024-04-02T00:48:37Z

frb_rust/Cargo.toml

+tokio-runtime = ["dep:tokio"]
+async-std-runtime = ["dep:async-std"]


nit: shall we make the names more coherent. e.g. "rust-async, rust-async-tokio, rust-async-async-std, rust-async-nuclei" or something like that

fzyzcjy · 2024-04-02T00:49:20Z

frb_rust/src/codec/cst.rs

@@ -22,6 +22,7 @@ impl BaseCodec for CstCodec {
    // frb-coverage:ignore-end
 }

+#[allow(dead_code)]


nit: curious why is this dead code?

fzyzcjy · 2024-04-02T00:50:32Z

frb_rust/src/ffi_binding/mod.rs

-#[cfg(not(wasm))]
-pub use io::*;

 #[cfg(wasm)]
 mod web;
-#[cfg(wasm)]
-pub use web::*;


nit: is it really ok to remove these exports 👀

fzyzcjy · 2024-04-02T00:51:24Z

frb_rust/src/rust_async/io/async_std.rs

nit: seems missing nuclei.rs

fzyzcjy · 2024-04-02T00:53:32Z

frb_rust/src/rust_async/web.rs

 }

 #[derive(Debug, Clone, Copy, Default)]
 pub struct SimpleAsyncRuntime;

 impl BaseAsyncRuntime for SimpleAsyncRuntime {
+    type JoinHandle<O> = wasm_bindgen_futures::JsFuture;
+
+    #[cfg(feature = "rust-async")]


nit: seems no need for this cfg, since the whole web.rs is under that flag

fzyzcjy · 2024-04-02T00:54:14Z

frb_rust/src/rust_async/io.rs

-/// Similar to tokio's `spawn_blocking`, except that you need to provide a second argumnet.
-/// If you are using flutter_rust_bridge, the second argument can be easily provided:
-/// Just use `FLUTTER_RUST_BRIDGE_HANDLER.thread_pool()`.
-///
-/// More formally, the second argument is defined as:
-///
-/// * When on web: The thread pool you want to use.
-/// * When on non-web: Unused, can be anything (since we use Tokio's built-in pool).


nit: these docs seems to be missing

fzyzcjy · 2024-04-02T00:59:16Z

frb_rust/src/rust_async/mod.rs

It would be great to add tests to ensure everything works.

The most thorough test will need to re-run frb_example/pure_dart multiple times, each using one async runtime. However, that may be super slow.

(I will think about it a bit later)

fzyzcjy · 2024-04-02T01:02:04Z

frb_rust/src/rust_async/mod.rs

nit: maybe we can update doc a bit to tell future users about the new feature :)

https://cjycode.com/flutter_rust_bridge/guides/cross-platform/async, changing things like "tokio" to "tokio / async-std / nuclei"

Inside https://cjycode.com/flutter_rust_bridge/guides/miscellaneous folder, maybe add a file saying "customizing async runtime"

and add a sentence in https://cjycode.com/flutter_rust_bridge/guides/concurrency/async-rust to point to that new doc

malivix · 2024-04-03T22:55:49Z

Good job! Mainly some nits

I am on travel will finalize it soon

fzyzcjy · 2024-04-03T23:48:18Z

I am on travel will finalize it soon

No worries, take your time and have a good vacation!

debanjanbasu · 2024-05-26T08:29:36Z

@malivix - Is this going to get some love? 🥰, have been waiting for it for some time... Is there anything I can be of help with?

malivix · 2024-05-26T08:59:35Z

@malivix - Is this going to get some love? 🥰, have been waiting for it for some time... Is there anything I can be of help with?

Sorry I was in travel and a bit busy will try to finalize it this week

feat: abstract async runtimes

080b6e8

malivix force-pushed the feat/abstract-async-runtime branch from b7352e9 to 080b6e8 Compare March 31, 2024 14:08

malivix added 2 commits April 1, 2024 14:43

fix: update cargo lock

1414487

fix: reexport runtime functions

4d940c0

malivix marked this pull request as ready for review April 1, 2024 15:13

malivix added 2 commits April 1, 2024 17:14

Merge branch 'master' into feat/abstract-async-runtime

7927a0b

fix: use correct version for ci

e3a0938

fzyzcjy reviewed Apr 2, 2024

View reviewed changes

fzyzcjy mentioned this pull request Apr 6, 2024

Cannot implement custom async runtime #1858

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abstract async runtimes #1848

Abstract async runtimes #1848

malivix commented Mar 31, 2024 •

edited

welcome bot commented Mar 31, 2024

malivix commented Mar 31, 2024

fzyzcjy commented Mar 31, 2024

codecov bot commented Mar 31, 2024 •

edited

fzyzcjy left a comment •

edited

fzyzcjy Apr 2, 2024

fzyzcjy Apr 2, 2024

fzyzcjy Apr 2, 2024

fzyzcjy Apr 2, 2024

fzyzcjy Apr 2, 2024

fzyzcjy Apr 2, 2024

fzyzcjy Apr 2, 2024

fzyzcjy Apr 2, 2024

fzyzcjy Apr 2, 2024

fzyzcjy Apr 2, 2024

malivix commented Apr 3, 2024

fzyzcjy commented Apr 3, 2024 •

edited

debanjanbasu commented May 26, 2024

malivix commented May 26, 2024

		tokio-runtime = ["dep:tokio"]
		async-std-runtime = ["dep:async-std"]

Abstract async runtimes #1848

Are you sure you want to change the base?

Abstract async runtimes #1848

Conversation

malivix commented Mar 31, 2024 • edited

Changes

Checklist

Remark for PR creator

welcome bot commented Mar 31, 2024

malivix commented Mar 31, 2024

fzyzcjy commented Mar 31, 2024

codecov bot commented Mar 31, 2024 • edited

Codecov Report

fzyzcjy left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malivix commented Apr 3, 2024

fzyzcjy commented Apr 3, 2024 • edited

debanjanbasu commented May 26, 2024

malivix commented May 26, 2024

malivix commented Mar 31, 2024 •

edited

codecov bot commented Mar 31, 2024 •

edited

fzyzcjy left a comment •

edited

fzyzcjy commented Apr 3, 2024 •

edited