tokio console support for core and repl tool

[Simon-Laux]

attached to repl tool:

attached to dc-desktop:

• add tokio console support to repl tool
• create a readme for the repl tool
• name tasks in repl tool and in core
• add console-subscriber to the deltachat-rpc-server, this allows debugging in desktop and bots that use the rpc server.
• Label Handle::current().spawn_blocking tasks after I know why it uses Handle::current().spawn_blocking instead of just tokio::task::spawn_blocking
• Figure out how to address cargo deny report

The named tasks are useful also in other projects that depend on deltachat core, like bots written in rust.

I'm not sure whether there is a benefit of adding a new cargo feature to conditionally depend on console-subscriber.
I think the tokio_unstable rustflag is enough of a toggle switch.

successor of #4418

[Simon-Laux]

I haven't thought much about naming conventions for the tasks yet, currently is is rather inconsistent in some places.

I prefixed the tasks for the REPL with repl::, but haven't done so for core, maybe dc:: could be a good prefix?

Things to consider:

• unless you have a wide terminal only first chars of name are shown in the table view.
• there is a column with source location, so maybe name doesn't need full location info?
• should tasks have "loop" in their name?

[iequidoo]

Jsonrpc tasks aren't prefixed, so maybe don't prefix anything at all? Having Location in the console seems sufficient

[Hocuri]

I know I'm late to the party with reviewing, so I'd be fine with my review being ignored here, but I do think that it would be better to include the console-subscriber and tracing dependencies only optionally in rpc-server.

[Hocuri]

Did you check whether it might be possible not to need duplicate versions here?

[Simon-Laux]

no

[Hocuri]

This will increase the app size for all users of Delta Chat, I don't think we should do this.

I think we should make them optional with something along these lines (didn't test):

diff --git a/deltachat-rpc-server/Cargo.toml b/deltachat-rpc-server/Cargo.toml
index ae93ebcca..84917d198 100644
--- a/deltachat-rpc-server/Cargo.toml
+++ b/deltachat-rpc-server/Cargo.toml
@@ -22,9 +22,10 @@ tokio = { workspace = true, features = ["io-std"] }
 tokio-util = { workspace = true }
 tracing-subscriber = { workspace = true, features = ["env-filter"] }
 yerpc = { workspace = true, features = ["anyhow_expose", "openrpc"] }
-console-subscriber = "0.4.0"
-tracing = "0.1.40"
+console-subscriber = { version = "0.4.0", optional = true }
+tracing = { version = "0.1.40", optional = true }
 
 [features]
+tokio_console = ["console-subscriber", "tracing"]
 default = ["vendored"]
 vendored = ["deltachat-jsonrpc/vendored"]
diff --git a/deltachat-rpc-server/README.md b/deltachat-rpc-server/README.md
index 2185db895..3d8a4263a 100644
--- a/deltachat-rpc-server/README.md
+++ b/deltachat-rpc-server/README.md
@@ -43,7 +43,7 @@ ## Usage with `tokio-console`
 You can install it via `cargo install tokio-console`.
 
 ```sh
-RUSTFLAGS="--cfg tokio_unstable" cargo run
+RUSTFLAGS="--cfg tokio_unstable" TOKIO_CONSOLE=1 cargo run --features tokio_console
 ```
 
 ### Usage in deltachat-desktop:
diff --git a/deltachat-rpc-server/npm-package/scripts/build_platform_package.py b/deltachat-rpc-server/npm-package/scripts/build_platform_package.py
index 4ebc29b8d..a1e0e2440 100755
--- a/deltachat-rpc-server/npm-package/scripts/build_platform_package.py
+++ b/deltachat-rpc-server/npm-package/scripts/build_platform_package.py
@@ -1,7 +1,7 @@
 #!/usr/bin/env python3
 import subprocess
 from sys import argv
-from os import path, makedirs, chdir
+from os import path, makedirs, chdir, environ
 from shutil import copy
 from src.make_package import write_package_json
 
@@ -14,10 +14,10 @@
 
 target = argv[1].strip()
 
-subprocess.run(
-    ["cargo", "build", "--release", "-p", "deltachat-rpc-server", "--target", target],
-    check=True,
-)
+cargo_command = ["cargo", "build", "--release", "-p", "deltachat-rpc-server", "--target", target]
+if environ.get("TOKIO_CONSOLE"):
+    cargo_command += ["--features", "tokio_console"]
+subprocess.run(cargo_command, check=True)
 
 newpath = "platform_package"
 if not path.exists(newpath):
diff --git a/deltachat-rpc-server/src/main.rs b/deltachat-rpc-server/src/main.rs
index 27ad90181..469b2670d 100644
--- a/deltachat-rpc-server/src/main.rs
+++ b/deltachat-rpc-server/src/main.rs
@@ -67,22 +67,20 @@ async fn main_impl() -> Result<()> {
     // Logs from `log` crate and traces from `tracing` crate
     // are configurable with `RUST_LOG` environment variable
     // and go to stderr to avoid interferring with JSON-RPC using stdout.
-    #[allow(unexpected_cfgs)]
+    #[cfg(not(feature = "tokio_console"))]
+    tracing_subscriber::fmt()
+        .with_env_filter(EnvFilter::from_default_env())
+        .with_writer(std::io::stderr)
+        .init();
+
+    #[cfg(feature = "tokio_console")]
     tracing::subscriber::set_global_default({
         let subscribers = tracing_subscriber::Registry::default().with(
             tracing_subscriber::fmt::layer()
                 .with_writer(std::io::stderr)
                 .with_filter(EnvFilter::from_default_env()),
         );
-
-        #[cfg(tokio_unstable)]
-        {
-            subscribers.with(console_subscriber::spawn())
-        }
-        #[cfg(not(tokio_unstable))]
-        {
-            subscribers
-        }
+        subscribers.with(console_subscriber::spawn())
     })?;
 
     let path = std::env::var("DC_ACCOUNTS_PATH").unwrap_or_else(|_| "accounts".to_string());

[Simon-Laux]

This will increase the app size for all users of Delta Chat, I don't think we should do this.

so rust has no tree-shaking / include only what is used?

[Simon-Laux]

So I tested it:

-rwxr-xr-x  1 bb  staff  16137432 Oct 23 13:56 target/release/deltachat-rpc-server
-rwxr-xr-x  1 bb  staff  16386520 Oct 23 13:49 target/release/deltachat-rpc-server-before

so statement looks incorrect to me.

This will increase the app size for all users of Delta Chat

only bots and desktop use deltachat-rpc-server

[Hocuri]

After looking into this some more: Probably the unused crates are optimized out, but it's complicated and probably they unnecessarily increase the compile times for every build. I.e. it would still be nice to only pull them in only when needed.

LTO is enabled in our workspace-root-Cargo.toml, so that there is another optimizing step after linking which should remove the unused code. To be sure, one would have to measure, which I could do, but: Compiling the crates, linking them, and then optimizing them out again is some work that will increase compile times. So, I would say that independently of whether the binary size increases, it's worth it to make the new dependencies optional.

[Simon-Laux]

probably they unnecessarily increase the compile times for every build

not all dependencies are compiled on each build again, you don't start from scratch every time normally.

But as long as the feature is only for deltachat-rpc-server, then I won't be against it.
But I also don't see the big benefit, so feel free to make a commit with this change yourself. 🤷

[Hocuri]

So I tested it: [...]

Wait, so, does this mean that this PR here significantly decreases the binary size?

If so, I guess it's fine, even though I don't understand it, but optimizations are weird - and, contrary to what I thought in the beginning, this change only affects Desktop, not Android & iOS, so, it's not as important as I thought.

[Simon-Laux]

Wait, so, does this mean that this PR here significantly decreases the binary size?

I did it two times to be sure, yes that were my results before rebasing, haven't tested again after that.

[Hocuri]

Question, feel free to ignore: Why not make this a simple function instead of a macro?

[Simon-Laux]

without it the location shown in tokio-console is wrong:
#4418 (comment)

[Hocuri]

You can annotate the function with #[track_caller] in order to get the correct location. We're already doing this for log_err(…).

Note that this only works for non-async functions until rust-lang/rust#110011 is implemented, but this would be fine in this case since __spawn_named_task() is non-async.

[Simon-Laux]

good to know, but I'd rather say we try this in a follow up pr