Merge #1018: Gui handle poller error

a7f2bccb9a443c78a9788eedce06e2d753f7d11e gui: handle daemon stop error (edouardparis)
924df8e1d5440159306504c0ddbbee6f962cd7c5 bump liana:master (edouardparis)

Pull request description:

  based on #986

ACKs for top commit:
  darosior:
    ACK a7f2bccb9a443c78a9788eedce06e2d753f7d11e

Tree-SHA512: 24c48948f10eed7f04dcab0779b75d89c5fd40441c04dffb6f4e5b3e682ca2ca36de51cdc69e7679ed262c4e92691d789c29822972f5c99290c54d13c7dc3472

gui/Cargo.lock

@@ -2620,7 +2620,7 @@ dependencies = [
 [[package]]
 name = "liana"
 version = "4.0.0"
-source = "git+https://github.com/wizardsardine/liana?branch=master#dd29578c500ea7644154a5923bea9fa1db9b68e5"
+source = "git+https://github.com/wizardsardine/liana?branch=master#2aa8874456bd8e43085c1496dedde68d3ce9d49a"
 dependencies = [
  "backtrace",
  "bdk_coin_select",

gui/src/app/mod.rs

@@ -16,7 +16,7 @@ use std::sync::Arc;
 use std::time::Duration;
 
 use iced::{clipboard, time, Command, Subscription};
-use tracing::{info, warn};
+use tracing::{error, info, warn};
 
 pub use liana::{config::Config as DaemonConfig, miniscript::bitcoin};
 use liana_ui::widget::Element;
@@ -217,8 +217,11 @@ impl App {
     pub fn stop(&mut self) {
         info!("Close requested");
         if !self.daemon.is_external() {
-            self.daemon.stop();
+            if let Err(e) = self.daemon.stop() {
+                error!("{}", e);
+            } else {
                 info!("Internal daemon stopped");
+            }
             if let Some(bitcoind) = &self.internal_bitcoind {
                 bitcoind.stop();
             }
@@ -232,6 +235,9 @@ impl App {
                 let datadir_path = self.cache.datadir_path.clone();
                 Command::perform(
                     async move {
+                        // we check every 10 second if the daemon poller is alive
+                        daemon.is_alive()?;
+
                         let info = daemon.get_info()?;
                         // todo: filter coins to only have current coins.
                         let coins = daemon.list_coins()?;
@@ -278,7 +284,7 @@ impl App {
         daemon_config_path: &PathBuf,
         cfg: DaemonConfig,
     ) -> Result<(), Error> {
-        self.daemon.stop();
+        self.daemon.stop()?;
         let daemon = EmbeddedDaemon::start(cfg)?;
         self.daemon = Arc::new(daemon);
 

gui/src/daemon/client/mod.rs

@@ -61,7 +61,11 @@ impl<C: Client + Debug> Daemon for Lianad<C> {
         None
     }
 
-    fn stop(&self) {
+    fn is_alive(&self) -> Result<(), DaemonError> {
+        Ok(())
+    }
+
+    fn stop(&self) -> Result<(), DaemonError> {
         unreachable!("GUI should not ask external client to stop")
     }
 

gui/src/daemon/embedded.rs

@@ -1,4 +1,5 @@
 use std::collections::{HashMap, HashSet};
+use std::sync::Mutex;
 
 use super::{model::*, Daemon, DaemonError};
 use liana::{
@@ -10,24 +11,35 @@ use liana::{
 
 pub struct EmbeddedDaemon {
     config: Config,
-    handle: DaemonHandle,
+    handle: Mutex<Option<DaemonHandle>>,
 }
 
 impl EmbeddedDaemon {
     pub fn start(config: Config) -> Result<EmbeddedDaemon, DaemonError> {
         let handle = DaemonHandle::start_default(config.clone()).map_err(DaemonError::Start)?;
-        Ok(Self { handle, config })
+        Ok(Self {
+            handle: Mutex::new(Some(handle)),
+            config,
+        })
     }
 
-    fn control(&self) -> Result<&DaemonControl, DaemonError> {
-        if self.handle.shutdown_complete() {
-            Err(DaemonError::DaemonStopped)
-        } else {
-            Ok(&self.handle.control)
+    pub fn command<T, F>(&self, method: F) -> Result<T, DaemonError>
+    where
+        F: FnOnce(&DaemonControl) -> Result<T, DaemonError>,
+    {
+        match self.handle.lock()?.as_ref() {
+            Some(DaemonHandle::Controller { control, .. }) => method(control),
+            None => Err(DaemonError::DaemonStopped),
         }
     }
 }
 
+impl<T> From<std::sync::PoisonError<T>> for DaemonError {
+    fn from(value: std::sync::PoisonError<T>) -> Self {
+        DaemonError::Unexpected(format!("Daemon panic: {}", value))
+    }
+}
+
 impl std::fmt::Debug for EmbeddedDaemon {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         f.debug_struct("DaemonHandle").finish()
@@ -43,30 +55,48 @@ impl Daemon for EmbeddedDaemon {
         Some(&self.config)
     }
 
-    fn stop(&self) {
-        self.handle.trigger_shutdown();
-        while !self.handle.shutdown_complete() {
-            tracing::debug!("Waiting daemon to shutdown");
-            std::thread::sleep(std::time::Duration::from_millis(500));
+    fn is_alive(&self) -> Result<(), DaemonError> {
+        let mut handle = self.handle.lock()?;
+        if let Some(h) = handle.as_ref() {
+            if h.is_alive() {
+                return Ok(());
             }
         }
+        // if the daemon poller is not alive, we try to terminate it to fetch the error.
+        if let Some(h) = handle.take() {
+            h.stop()
+                .map_err(|e| DaemonError::Unexpected(e.to_string()))?;
+        }
+        Ok(())
+    }
+
+    fn stop(&self) -> Result<(), DaemonError> {
+        let mut handle = self.handle.lock()?;
+        if let Some(h) = handle.take() {
+            h.stop()
+                .map_err(|e| DaemonError::Unexpected(e.to_string()))?;
+        }
+        Ok(())
+    }
 
     fn get_info(&self) -> Result<GetInfoResult, DaemonError> {
-        Ok(self.control()?.get_info())
+        self.command(|daemon| Ok(daemon.get_info()))
     }
 
     fn get_new_address(&self) -> Result<GetAddressResult, DaemonError> {
-        Ok(self.control()?.get_new_address())
+        self.command(|daemon| Ok(daemon.get_new_address()))
     }
 
     fn list_coins(&self) -> Result<ListCoinsResult, DaemonError> {
-        Ok(self.control()?.list_coins(&[], &[]))
+        self.command(|daemon| Ok(daemon.list_coins(&[], &[])))
     }
 
     fn list_spend_txs(&self) -> Result<ListSpendResult, DaemonError> {
-        self.control()?
+        self.command(|daemon| {
+            daemon
                 .list_spend(None)
                 .map_err(|e| DaemonError::Unexpected(e.to_string()))
+        })
     }
 
     fn list_confirmed_txs(
@@ -75,13 +105,11 @@ impl Daemon for EmbeddedDaemon {
         end: u32,
         limit: u64,
     ) -> Result<ListTransactionsResult, DaemonError> {
-        Ok(self
-            .control()?
-            .list_confirmed_transactions(start, end, limit))
+        self.command(|daemon| Ok(daemon.list_confirmed_transactions(start, end, limit)))
     }
 
     fn list_txs(&self, txids: &[Txid]) -> Result<ListTransactionsResult, DaemonError> {
-        Ok(self.control()?.list_transactions(txids))
+        self.command(|daemon| Ok(daemon.list_transactions(txids)))
     }
 
     fn create_spend_tx(
@@ -91,9 +119,11 @@ impl Daemon for EmbeddedDaemon {
         feerate_vb: u64,
         change_address: Option<Address<address::NetworkUnchecked>>,
     ) -> Result<CreateSpendResult, DaemonError> {
-        self.control()?
+        self.command(|daemon| {
+            daemon
                 .create_spend(destinations, coins_outpoints, feerate_vb, change_address)
                 .map_err(|e| DaemonError::Unexpected(e.to_string()))
+        })
     }
 
     fn rbf_psbt(
@@ -102,32 +132,42 @@ impl Daemon for EmbeddedDaemon {
         is_cancel: bool,
         feerate_vb: Option<u64>,
     ) -> Result<CreateSpendResult, DaemonError> {
-        self.control()?
+        self.command(|daemon| {
+            daemon
                 .rbf_psbt(txid, is_cancel, feerate_vb)
                 .map_err(|e| DaemonError::Unexpected(e.to_string()))
+        })
     }
 
     fn update_spend_tx(&self, psbt: &Psbt) -> Result<(), DaemonError> {
-        self.control()?
+        self.command(|daemon| {
+            daemon
                 .update_spend(psbt.clone())
                 .map_err(|e| DaemonError::Unexpected(e.to_string()))
+        })
     }
 
     fn delete_spend_tx(&self, txid: &Txid) -> Result<(), DaemonError> {
-        self.control()?.delete_spend(txid);
+        self.command(|daemon| {
+            daemon.delete_spend(txid);
             Ok(())
+        })
     }
 
     fn broadcast_spend_tx(&self, txid: &Txid) -> Result<(), DaemonError> {
-        self.control()?
+        self.command(|daemon| {
+            daemon
                 .broadcast_spend(txid)
                 .map_err(|e| DaemonError::Unexpected(e.to_string()))
+        })
     }
 
     fn start_rescan(&self, t: u32) -> Result<(), DaemonError> {
-        self.control()?
+        self.command(|daemon| {
+            daemon
                 .start_rescan(t)
                 .map_err(|e| DaemonError::Unexpected(e.to_string()))
+        })
     }
 
     fn create_recovery(
@@ -136,21 +176,25 @@ impl Daemon for EmbeddedDaemon {
         feerate_vb: u64,
         sequence: Option<u16>,
     ) -> Result<Psbt, DaemonError> {
-        self.control()?
+        self.command(|daemon| {
+            daemon
                 .create_recovery(address, feerate_vb, sequence)
-            .map_err(|e| DaemonError::Unexpected(e.to_string()))
                 .map(|res| res.psbt)
+                .map_err(|e| DaemonError::Unexpected(e.to_string()))
+        })
     }
 
     fn get_labels(
         &self,
         items: &HashSet<LabelItem>,
     ) -> Result<HashMap<String, String>, DaemonError> {
-        Ok(self.handle.control.get_labels(items).labels)
+        self.command(|daemon| Ok(daemon.get_labels(items).labels))
     }
 
     fn update_labels(&self, items: &HashMap<LabelItem, Option<String>>) -> Result<(), DaemonError> {
-        self.handle.control.update_labels(items);
+        self.command(|daemon| {
+            daemon.update_labels(items);
             Ok(())
+        })
     }
 }

gui/src/daemon/mod.rs

@@ -52,7 +52,8 @@ impl std::fmt::Display for DaemonError {
 pub trait Daemon: Debug {
     fn is_external(&self) -> bool;
     fn config(&self) -> Option<&Config>;
-    fn stop(&self);
+    fn is_alive(&self) -> Result<(), DaemonError>;
+    fn stop(&self) -> Result<(), DaemonError>;
     fn get_info(&self) -> Result<model::GetInfoResult, DaemonError>;
     fn get_new_address(&self) -> Result<model::GetAddressResult, DaemonError>;
     fn list_coins(&self) -> Result<model::ListCoinsResult, DaemonError>;

gui/src/installer/mod.rs

@@ -260,10 +260,9 @@ impl Installer {
 pub fn daemon_check(cfg: liana::config::Config) -> Result<(), Error> {
     // Start Daemon to check correctness of installation
     match liana::DaemonHandle::start_default(cfg) {
-        Ok(daemon) => {
-            daemon.shutdown();
-            Ok(())
-        }
+        Ok(daemon) => daemon
+            .stop()
+            .map_err(|e| Error::Unexpected(format!("Failed to stop Liana daemon: {}", e))),
         Err(e) => Err(Error::Unexpected(format!(
             "Failed to start Liana daemon: {}",
             e

gui/src/loader.rs

@@ -228,10 +228,13 @@ impl Loader {
         if let Step::Syncing { daemon, .. } = &mut self.step {
             if !daemon.is_external() {
                 info!("Stopping internal daemon...");
-                daemon.stop();
+                if let Err(e) = daemon.stop() {
+                    warn!("Internal daemon failed to stop: {}", e);
+                } else {
                     info!("Internal daemon stopped");
                 }
             }
+        }
 
         // NOTE: we take() the internal_bitcoind here to make sure the debug.log reader
         // subscription is dropped.