Merge branch 'android-simplify-daemon-spawn'

Author: dlon

Cargo.lock

@@ -1,83 +1,19 @@
 package net.mullvad.mullvadvpn.service
 
-import android.annotation.SuppressLint
-import android.content.Context
-import java.io.File
-import kotlinx.coroutines.Dispatchers
-import kotlinx.coroutines.async
-import kotlinx.coroutines.channels.Channel
-import kotlinx.coroutines.runBlocking
-import kotlinx.coroutines.withContext
 import net.mullvad.mullvadvpn.lib.endpoint.ApiEndpoint
-import net.mullvad.mullvadvpn.lib.endpoint.ApiEndpointConfiguration
-import net.mullvad.mullvadvpn.service.migration.MigrateSplitTunneling
-
-private const val RELAYS_FILE = "relays.json"
-
-@SuppressLint("SdCardPath")
-class MullvadDaemon(
-    vpnService: MullvadVpnService,
-    rpcSocketFile: File,
-    apiEndpointConfiguration: ApiEndpointConfiguration,
-    migrateSplitTunneling: MigrateSplitTunneling
-) {
-    // Used by JNI
-    @Suppress("ProtectedMemberInFinalClass") protected var daemonInterfaceAddress = 0L
-
-    private val shutdownSignal = Channel<Unit>()
 
+object MullvadDaemon {
     init {
         System.loadLibrary("mullvad_jni")
-
-        prepareFiles(vpnService)
-
-        migrateSplitTunneling.migrate()
-
-        initialize(
-            vpnService = vpnService,
-            rpcSocketPath = rpcSocketFile.absolutePath,
-            filesDirectory = vpnService.filesDir.absolutePath,
-            cacheDirectory = vpnService.cacheDir.absolutePath,
-            apiEndpoint = apiEndpointConfiguration.apiEndpoint()
-        )
-    }
-
-    suspend fun shutdown() =
-        withContext(Dispatchers.IO) {
-            val shutdownSignal = async { shutdownSignal.receive() }
-            shutdown(daemonInterfaceAddress)
-            shutdownSignal.await()
-            deinitialize()
-        }
-
-    private fun prepareFiles(context: Context) {
-        val shouldOverwriteRelayList =
-            lastUpdatedTime(context) > File(context.filesDir, RELAYS_FILE).lastModified()
-
-        FileResourceExtractor(context).apply { extract(RELAYS_FILE, shouldOverwriteRelayList) }
     }
 
-    private fun lastUpdatedTime(context: Context): Long =
-        context.packageManager.getPackageInfo(context.packageName, 0).lastUpdateTime
-
-    // Used by JNI
-    @Suppress("unused")
-    private fun notifyDaemonStopped() {
-        runBlocking {
-            shutdownSignal.send(Unit)
-            shutdownSignal.close()
-        }
-    }
-
-    private external fun initialize(
+    external fun initialize(
         vpnService: MullvadVpnService,
         rpcSocketPath: String,
         filesDirectory: String,
         cacheDirectory: String,
         apiEndpoint: ApiEndpoint?
     )
 
-    private external fun deinitialize()
-
-    private external fun shutdown(daemonInterfaceAddress: Long)
+    external fun shutdown()
 }

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadDaemon.kt

@@ -1,6 +1,7 @@
 package net.mullvad.mullvadvpn.service
 
 import android.app.KeyguardManager
+import android.content.Context
 import android.content.Intent
 import android.os.Binder
 import android.os.Build
@@ -18,6 +19,7 @@ import kotlinx.coroutines.flow.first
 import kotlinx.coroutines.flow.update
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.runBlocking
+import net.mullvad.mullvadvpn.lib.common.constant.BuildTypes
 import net.mullvad.mullvadvpn.lib.common.constant.GRPC_SOCKET_FILE_NAMED_ARGUMENT
 import net.mullvad.mullvadvpn.lib.common.constant.KEY_CONNECT_ACTION
 import net.mullvad.mullvadvpn.lib.common.constant.KEY_DISCONNECT_ACTION
@@ -40,12 +42,13 @@ import org.koin.android.ext.android.getKoin
 import org.koin.core.context.loadKoinModules
 import org.koin.core.qualifier.named
 
+private const val RELAYS_FILE = "relays.json"
+
 class MullvadVpnService : TalpidVpnService(), ShouldBeOnForegroundProvider {
     private val _shouldBeOnForeground = MutableStateFlow(false)
     override val shouldBeOnForeground: StateFlow<Boolean> = _shouldBeOnForeground
 
     private lateinit var keyguardManager: KeyguardManager
-    private lateinit var daemonInstance: MullvadDaemon
 
     private lateinit var apiEndpointConfiguration: ApiEndpointConfiguration
     private lateinit var managementService: ManagementService
@@ -90,15 +93,11 @@ class MullvadVpnService : TalpidVpnService(), ShouldBeOnForegroundProvider {
         // with intent from API)
         lifecycleScope.launch(context = Dispatchers.IO) {
             managementService.start()
-            daemonInstance =
-                MullvadDaemon(
-                    vpnService = this@MullvadVpnService,
-                    rpcSocketFile = rpcSocketFile,
-                    apiEndpointConfiguration =
-                        intentProvider.getLatestIntent()?.getApiEndpointConfigurationExtras()
-                            ?: apiEndpointConfiguration,
-                    migrateSplitTunneling = migrateSplitTunneling
-                )
+
+            prepareFiles(this@MullvadVpnService)
+            migrateSplitTunneling.migrate()
+
+            startDaemon()
         }
     }
 
@@ -146,6 +145,24 @@ class MullvadVpnService : TalpidVpnService(), ShouldBeOnForegroundProvider {
         return super.onBind(intent) ?: emptyBinder()
     }
 
+    private fun startDaemon() {
+        val apiEndpointConfiguration =
+            if (Build.TYPE == BuildTypes.DEBUG) {
+                intentProvider.getLatestIntent()?.getApiEndpointConfigurationExtras()
+                    ?: apiEndpointConfiguration
+            } else {
+                apiEndpointConfiguration
+            }
+
+        MullvadDaemon.initialize(
+            vpnService = this@MullvadVpnService,
+            rpcSocketPath = rpcSocketFile.absolutePath,
+            filesDirectory = filesDir.absolutePath,
+            cacheDirectory = cacheDir.absolutePath,
+            apiEndpoint = apiEndpointConfiguration.apiEndpoint()
+        )
+    }
+
     private fun emptyBinder() =
         if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) {
             Binder(this.toString())
@@ -193,7 +210,7 @@ class MullvadVpnService : TalpidVpnService(), ShouldBeOnForegroundProvider {
         managementService.stop()
 
         // Shutting down the daemon gracefully
-        runBlocking { daemonInstance.shutdown() }
+        MullvadDaemon.shutdown()
         super.onDestroy()
     }
 
@@ -202,6 +219,16 @@ class MullvadVpnService : TalpidVpnService(), ShouldBeOnForegroundProvider {
         return this?.action == SERVICE_INTERFACE
     }
 
+    private fun prepareFiles(context: Context) {
+        val shouldOverwriteRelayList =
+            lastUpdatedTime(context) > File(context.filesDir, RELAYS_FILE).lastModified()
+
+        FileResourceExtractor(context).apply { extract(RELAYS_FILE, shouldOverwriteRelayList) }
+    }
+
+    private fun lastUpdatedTime(context: Context): Long =
+        context.packageManager.getPackageInfo(context.packageName, 0).lastUpdateTime
+
     companion object {
         init {
             System.loadLibrary("mullvad_jni")

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt

@@ -30,7 +30,7 @@ pub enum Error {
 
 impl<L> Daemon<L>
 where
-    L: EventListener + Clone + Send + 'static,
+    L: EventListener,
 {
     /// Add a [`AccessMethod`] to the daemon's settings.
     ///

mullvad-daemon/src/access_method.rs

@@ -8,7 +8,7 @@ use talpid_types::net::TunnelType;
 
 impl<L> Daemon<L>
 where
-    L: EventListener + Clone + Send + 'static,
+    L: EventListener,
 {
     /// Create a new custom list.
     ///

mullvad-daemon/src/custom_list.rs

@@ -33,7 +33,7 @@ use api::AccessMethodEvent;
 use device::{AccountEvent, PrivateAccountAndDevice, PrivateDeviceEvent};
 use futures::{
     channel::{mpsc, oneshot},
-    future::{abortable, AbortHandle, Future, LocalBoxFuture},
+    future::{abortable, AbortHandle, Future},
     StreamExt,
 };
 use geoip::GeoIpHandler;
@@ -467,7 +467,7 @@ impl DaemonCommandChannel {
     }
 }
 
-#[derive(Clone)]
+#[derive(Debug, Clone)]
 pub struct DaemonCommandSender(Arc<mpsc::UnboundedSender<InternalDaemonEvent>>);
 
 impl DaemonCommandSender {
@@ -540,7 +540,7 @@ where
 }
 
 /// Trait representing something that can broadcast daemon events.
-pub trait EventListener {
+pub trait EventListener: Clone + Send + Sync + 'static {
     /// Notify that the tunnel state changed.
     fn notify_new_state(&self, new_state: TunnelState);
 
@@ -585,7 +585,7 @@ pub struct Daemon<L: EventListener> {
     relay_selector: RelaySelector,
     relay_list_updater: RelayListUpdaterHandle,
     parameters_generator: tunnel::ParametersGenerator,
-    shutdown_tasks: Vec<Pin<Box<dyn Future<Output = ()>>>>,
+    shutdown_tasks: Vec<Pin<Box<dyn Future<Output = ()> + Send + Sync>>>,
     tunnel_state_machine_handle: TunnelStateMachineHandle,
     #[cfg(target_os = "windows")]
     volume_update_tx: mpsc::UnboundedSender<()>,
@@ -594,7 +594,7 @@ pub struct Daemon<L: EventListener> {
 
 impl<L> Daemon<L>
 where
-    L: EventListener + Clone + Send + 'static,
+    L: EventListener,
 {
     pub async fn start(
         log_dir: Option<PathBuf>,
@@ -897,46 +897,27 @@ where
     /// Destroy daemon safely, by dropping all objects in the correct order, waiting for them to
     /// be destroyed, and executing shutdown tasks
     async fn finalize(self) {
-        let (event_listener, shutdown_tasks, api_runtime, tunnel_state_machine_handle) =
-            self.shutdown();
-        for future in shutdown_tasks {
-            future.await;
-        }
-
-        tunnel_state_machine_handle.try_join().await;
-
-        drop(event_listener);
-        drop(api_runtime);
-    }
-
-    /// Shuts down the daemon without shutting down the underlying event listener and the shutdown
-    /// callbacks
-    fn shutdown<'a>(
-        self,
-    ) -> (
-        L,
-        Vec<LocalBoxFuture<'a, ()>>,
-        mullvad_api::Runtime,
-        TunnelStateMachineHandle,
-    ) {
         let Daemon {
             event_listener,
-            mut shutdown_tasks,
+            shutdown_tasks,
             api_runtime,
             tunnel_state_machine_handle,
             target_state,
             account_manager,
             ..
         } = self;
-        shutdown_tasks.push(Box::pin(target_state.finalize()));
-        shutdown_tasks.push(Box::pin(account_manager.shutdown()));
 
-        (
-            event_listener,
-            shutdown_tasks,
-            api_runtime,
-            tunnel_state_machine_handle,
-        )
+        for future in shutdown_tasks {
+            future.await;
+        }
+
+        target_state.finalize().await;
+        account_manager.shutdown().await;
+
+        tunnel_state_machine_handle.try_join().await;
+
+        drop(event_listener);
+        drop(api_runtime);
     }
 
     async fn handle_event(&mut self, event: InternalDaemonEvent) -> bool {
@@ -1686,7 +1667,7 @@ where
 
     #[cfg(not(target_os = "android"))]
     async fn on_factory_reset(&mut self, tx: ResponseTx<(), Error>) {
-        let mut last_error = Ok(());
+        let mut last_error = None;
 
         if let Err(error) = self.account_manager.logout().await {
             log::error!(
@@ -1700,12 +1681,12 @@ where
                 "{}",
                 error.display_chain_with_msg("Failed to clear account history")
             );
-            last_error = Err(Error::FactoryResetError("Failed to clear account history"));
+            last_error = Some("Failed to clear account history");
         }
 
         if let Err(e) = self.settings.reset().await {
             log::error!("Failed to reset settings: {}", e);
-            last_error = Err(Error::FactoryResetError("Failed to reset settings"));
+            last_error = Some("Failed to reset settings");
         }
 
         // Shut the daemon down.
@@ -1717,11 +1698,12 @@ where
                     "{}",
                     e.display_chain_with_msg("Failed to clear cache and log directories")
                 );
-                last_error = Err(Error::FactoryResetError(
-                    "Failed to clear cache and log directories",
-                ));
+                last_error = Some("Failed to clear cache and log directories");
             }
-            Self::oneshot_send(tx, last_error, "factory_reset response");
+            let result = last_error
+                .map(|error| Err(Error::FactoryResetError(error)))
+                .unwrap_or(Ok(()));
+            Self::oneshot_send(tx, result, "factory_reset response");
         }));
     }
 

mullvad-daemon/src/lib.rs

@@ -94,7 +94,7 @@ pub struct SettingsPersister {
     settings: Settings,
     path: PathBuf,
     #[allow(clippy::type_complexity)]
-    on_change_listeners: Vec<Box<dyn Fn(&Settings)>>,
+    on_change_listeners: Vec<Box<dyn Fn(&Settings) + Send + Sync>>,
 }
 
 pub type MadeChanges = bool;
@@ -353,7 +353,10 @@ impl SettingsPersister {
         }
     }
 
-    pub fn register_change_listener(&mut self, change_listener: impl Fn(&Settings) + 'static) {
+    pub fn register_change_listener(
+        &mut self,
+        change_listener: impl Fn(&Settings) + Send + Sync + 'static,
+    ) {
         self.on_change_listeners.push(Box::new(change_listener));
     }
 

mullvad-daemon/src/settings/mod.rs

@@ -18,6 +18,8 @@ api-override = ["mullvad-api/api-override"]
 crate-type = ["cdylib"]
 
 [target.'cfg(target_os = "android")'.dependencies]
+tokio = { workspace = true, features = ["rt"] }
+
 thiserror = { workspace = true }
 futures = "0.3"
 ipnetwork = "0.16"