The internally-managed daemon isn't properly shut down when stopping the GUI

[darosior]

See https://github.com/wizardsardine/liana/pull/592#issuecomment-1686485130, https://github.com/wizardsardine/liana/pull/592#issuecomment-1686621375 and finally https://github.com/wizardsardine/liana/pull/592#issuecomment-1687861737.

The trivial way to go about it would be to add a Mutex. This is a lot of churn, and a lot of added overhead, only to be able to stop the daemon. I'd prefer to avoid this.

A minimal and hacky way of fixing this would be to use some unsafe code:

--- a/gui/src/loader.rs
+++ b/gui/src/loader.rs
@@ -172,11 +172,15 @@ impl Loader {
         if let Step::Syncing { daemon, .. } = &mut self.step {
             if !daemon.is_external() {
                 info!("Stopping internal daemon...");
-                if let Some(d) = Arc::get_mut(daemon) {
-                    d.stop().expect("Daemon is internal");
-                    info!("Internal daemon stopped");
-                } else {
+                // NOTE: this is a hack. The application will be stopped immediately after exiting
+                // this function, so no other reference should be accessed anymore.
+                while Arc::strong_count(&daemon) > 1 {
+                    unsafe {
+                        Arc::decrement_strong_count(Arc::as_ptr(&daemon));
+                    }
                 }
+                Arc::get_mut(daemon).expect("Strong count is 1 by now").stop().expect("Daemon is internal");
+                info!("Internal daemon stopped");
             }
             if self.gui_config.internal_bitcoind_exe_config.is_some() {
                 if let Some(daemon_config) = daemon.config() {

Of course, i'd prefer to avoid this too.

I've also considered adding some internal mutability inside the DaemonHandle's poller.

Another option could be to somehow leverage the Drop implementation of the various states of the GUI? We'd stop the internally managed daemon here and eventually stop the internally managed bitcoind.

Finally, i've also considered a poll-based interface on DaemonHandle instead of the current shutdown that waits for the poller thread to shutdown. This is because waiting for the thread immediately triggers this for the GUI: So we could instead have some sort of interface like:

1. Trigger shutdown
2. Is it shut down yet?

[edouardparis]

I am open to use a Mutex to make daemon stop use &self instead of &mut, I already use one https://github.com/wizardsardine/liana/blob/master/gui/src/daemon/embedded.rs#L13.

[darosior]

I'm working on my last suggestion now, but if it works i'll need your help to generalize it to all states.

[darosior]

Using a Mutex would add overhead to 99.99999% of the calls while we'd need it only in 0.000001% of those. It strikes me as a bad idea.

[darosior]

What do you think of this?

diff --git a/src/bitcoin/poller/mod.rs b/src/bitcoin/poller/mod.rs
index 95a2053..563ad2f 100644
--- a/src/bitcoin/poller/mod.rs
+++ b/src/bitcoin/poller/mod.rs
@@ -36,11 +36,22 @@ impl Poller {
         Poller { shutdown, handle }
     }

-    pub fn stop(self) {
+    pub fn trigger_stop(&self) {
         self.shutdown.store(true, atomic::Ordering::Relaxed);
+    }
+
+    pub fn stop(self) {
+        self.trigger_stop();
         self.handle.join().expect("The poller loop must not fail");
     }

+    pub fn is_stopped(&self) -> bool {
+        // Doc says "This might return true for a brief moment after the thread’s main function has
+        // returned, but before the thread itself has stopped running.". But it's not an issue for
+        // us, as long as the main poller function has returned we are good.
+        self.handle.is_finished()
+    }
+
     #[cfg(test)]
     pub fn test_stop(&mut self) {
         self.shutdown.store(true, atomic::Ordering::Relaxed);
diff --git a/src/lib.rs b/src/lib.rs
index be6e1cf..b57b78d 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -458,12 +458,22 @@ impl DaemonHandle {
         Ok(())
     }

-    // NOTE: this moves out the data as it should not be reused after shutdown
     /// Shut down the Liana daemon.
     pub fn shutdown(self) {
         self.bitcoin_poller.stop();
     }

+    /// Tell the daemon to shut down. This will return before the shutdown completes. The structure
+    /// must not be reused after triggering shutdown.
+    pub fn trigger_shutdown(&self) {
+        self.bitcoin_poller.trigger_stop()
+    }
+
+    /// Whether the daemon has finished shutting down.
+    pub fn shutdown_complete(&self) -> bool {
+        self.bitcoin_poller.is_stopped()
+    }
+
     // We need a shutdown utility that does not move for implementing Drop for the DummyLiana
     #[cfg(test)]
     pub fn test_shutdown(&mut self) {

And the GUI would split the stop step in two (tell the daemon to stop using trigger_shutdown and then wait for it to be stopped using shutdown_complete).

[edouardparis]

Seems good to me, please provide me a branch so I can integrate it in the gui. Would be awesome, if it helps also to get ride of RW lock in darmon/embedded.rs

[darosior]

This was fixed in #624.