chore: Introduce dedicated runtime #571

holzeis · 2022-12-12T15:07:15Z

Annotating the function with #[tokio::main] killed the runtime after the function completes. Thus we had to block the run function indefinitely for the long living threads to keep going.

This change introduces a runtime managed outside of the function scope and thus allows the run function to return bringing the following advantages.

We don't block a whole frb worker thread just to run the lightning node, sync tasks, background processor, etc.
We are using a multi threaded runtime instead of the current thread
- allowing to actually join the background processor without blocking all other tasks.
- making better use of multiple cpu cores.
We are not creating a new runtime on every async bridge call.

Annotating the function with `#[tokio::main]` killed the runtime after the function completes. Thus we had to block the run function indefinitely for the long living threads to keep going. This change introduces a runtime managed outside of the function scope and thus allows the `run` function to return bringing the following advantages. - We don't block a whole frb worker thread just to run the lightning node, sync tasks, background processor, etc. - We are using a multi threaded runtime instead of the current thread - allowing to actually join the background processor without blocking all other tasks. - making better use of multiple cpu cores. - We are not creating a new runtime on every async bridge call.

luckysori

Much better imo.

luckysori · 2022-12-14T23:55:21Z

rust/src/api.rs

+    if RUNTIME.try_get().is_none() {
+        let runtime = Runtime::new()?;
+        RUNTIME.set(runtime);
+    }


❓ Can't we make get_or_set work. I think that's what the library would want you to use here.

I tried, but I wasn't able to get it to work with a Result - do you have a suggestion how this could work?

luckysori · 2022-12-15T00:06:38Z

rust/src/api.rs

-    wallet::sync()?;
-    WalletInfo::build_wallet_info().await
+    if RUNTIME.try_get().is_none() {
+        let runtime = Runtime::new()?;


🔧 I think we should expect here. When we didn't have to deal with building the Runtime explicitly (when we were using #[tokio::main], we must have been doing it implicitly anyway.

This can only be called once, so it is unfortunate that we need to deal with a possibly fallible function because of that. Furthermore, if we can't build the Runtime we cannot easily recover from this AFAIK. The best strategy in such a situation would probably be to crash the application anyway.

I think this can be a problem if it makes us return Results in places where it wasn't already necessary, but I think all the functions which use this were already returning Results in the first place. So it doesn't matter so much in practice.

luckysori · 2022-12-15T00:27:36Z

rust/src/api.rs

+        stream.add(Event::Init(format!("Initialising {network} wallet")));
+        wallet::init_wallet(Path::new(app_dir.as_str()))?;
+
+        stream.add(Event::Init("Initialising database".to_string()));
+        db::init_db(
+            &Path::new(app_dir.as_str())
+                .join(network.to_string())
+                .join("taker.sqlite"),
+        )
+        .await?;
+
+        stream.add(Event::Init("Starting full ldk node".to_string()));
+        let background_processor = wallet::run_ldk()?;
+
+        stream.add(Event::Init("Fetching an offer".to_string()));
+        stream.add(Event::Offer(offer::get_offer().await.ok()));
+
+        stream.add(Event::Init("Fetching your balance".to_string()));
+        stream.add(Event::WalletInfo(
+            WalletInfo::build_wallet_info().await.ok(),
+        ));
+        stream.add(Event::Init("Checking channel state".to_string()));
+        stream.add(Event::ChannelState(get_channel_state()));
+
+        stream.add(Event::Init("Ready".to_string()));
+        stream.add(Event::Ready);


🤡 I am okay with this pragmatic approach where we just explicitly call all of these once so that we can return Ready afterwards and let the UI know that we are done with the startup. But eventually it might be nice to not need to explicitly send a Ready event.

Perhaps the frontend could expect a few events to be emitted before considering the app ready, without needing to be told.

luckysori · 2022-12-15T00:28:56Z

rust/src/api.rs

+            // background processor joins on a sync thread, meaning that join here will block a
+            // full thread, which is dis-encouraged to do in async code.
+            if let Err(err) = background_processor.join() {
+                tracing::error!(?err, "Background processor stopped unexpected");


🔧 It's an adverb.

Suggested change

tracing::error!(?err, "Background processor stopped unexpected");

tracing::error!(?err, "Background processor stopped unexpectedly");

luckysori · 2022-12-15T04:02:42Z

rust/src/api.rs

+        // spawn a connection task keeping the connection with the maker alive.
+        runtime.spawn(async move {
+            let peer_info = config::maker_peer_info();
+            loop {
+                let peer_manager = wallet::get_peer_manager();
+                connection::connect(peer_manager, peer_info).await;
+                // add a delay before retrying to connect
+                tokio::time::sleep(std::time::Duration::from_secs(5)).await;
+            }
+        });


🙃 We could extract the bodies of the spawned tasks into functions. It might aid readability.

Suggested change

// spawn a connection task keeping the connection with the maker alive.

runtime.spawn(async move {

let peer_info = config::maker_peer_info();

loop {

let peer_manager = wallet::get_peer_manager();

connection::connect(peer_manager, peer_info).await;

// add a delay before retrying to connect

tokio::time::sleep(std::time::Duration::from_secs(5)).await;

}

});

runtime.spawn(stay_connected_to_maker());

// ...

fn async stay_connected_to_maker() {

let peer_info = config::maker_peer_info();

loop {

let peer_manager = wallet::get_peer_manager();

connection::connect(peer_manager, peer_info).await;

tokio::time::sleep(std::time::Duration::from_secs(5)).await;

}

luckysori · 2022-12-15T04:04:04Z

rust/src/api.rs

+            let peer_info = config::maker_peer_info();
+            loop {
+                let peer_manager = wallet::get_peer_manager();


🔧 Is this possible? That way we only call it once.

Suggested change

let peer_info = config::maker_peer_info();

loop {

let peer_manager = wallet::get_peer_manager();

let peer_info = config::maker_peer_info();

let peer_manager = wallet::get_peer_manager();

loop {

I had it like this before, but I think clippy complained about the peer_manager being moved or something.

you can probably add .clone() to it to bypass clippy

klochowicz

very interesting!
having an actual multi-threaded runtime that doesn't take any of the threads on frb threadpool is exciting - I'd love to give it a go and see how it works.

holzeis requested review from bonomat, da-kami, klochowicz and luckysori December 12, 2022 15:07

holzeis self-assigned this Dec 12, 2022

holzeis force-pushed the chore/use-dedicated-runtime branch from 97defa9 to d895a5b Compare December 12, 2022 15:08

holzeis changed the title ~~chore: Introduce dedicate runtime~~ chore: Introduce dedicated runtime Dec 12, 2022

holzeis force-pushed the chore/use-dedicated-runtime branch from d895a5b to 4e54c8d Compare December 12, 2022 15:44

luckysori approved these changes Dec 15, 2022

View reviewed changes

luckysori mentioned this pull request Dec 15, 2022

feat: Restart failed background processor #572

Open

klochowicz approved these changes Dec 19, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Introduce dedicated runtime #571

chore: Introduce dedicated runtime #571

holzeis commented Dec 12, 2022

luckysori left a comment

luckysori Dec 14, 2022

holzeis Dec 15, 2022

luckysori Dec 15, 2022

luckysori Dec 15, 2022

luckysori Dec 15, 2022

luckysori Dec 15, 2022

luckysori Dec 15, 2022

luckysori Dec 15, 2022

holzeis Dec 15, 2022

klochowicz Dec 19, 2022

klochowicz left a comment

	tracing::error!(?err, "Background processor stopped unexpected");
	tracing::error!(?err, "Background processor stopped unexpectedly");

chore: Introduce dedicated runtime #571

Are you sure you want to change the base?

chore: Introduce dedicated runtime #571

Conversation

holzeis commented Dec 12, 2022

luckysori left a 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

Choose a reason for hiding this comment

klochowicz left a comment

Choose a reason for hiding this comment