Merge branch 'refactor-problem-report-into-an-actor'

3 files changed, 153 insertions, 128 deletions

diff --git a/android/src/main/kotlin/net/mullvad/mullvadvpn/dataproxy/MullvadProblemReport.kt b/android/src/main/kotlin/net/mullvad/mullvadvpn/dataproxy/MullvadProblemReport.kt
index 12dd4cd43b..ae2d1b2df2 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/dataproxy/MullvadProblemReport.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/dataproxy/MullvadProblemReport.kt
@@ -5,114 +5,128 @@ import kotlinx.coroutines.CompletableDeferred
 import kotlinx.coroutines.Deferred
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.GlobalScope
-import kotlinx.coroutines.Job
 import kotlinx.coroutines.async
-import kotlinx.coroutines.launch
+import kotlinx.coroutines.channels.Channel
+import kotlinx.coroutines.channels.ClosedReceiveChannelException
+import kotlinx.coroutines.channels.actor
+import kotlinx.coroutines.channels.sendBlocking
 
 const val PROBLEM_REPORT_FILE = "problem_report.txt"
 
 class MullvadProblemReport {
+    private sealed class Command {
+        class Collect() : Command()
+        class Load(val logs: CompletableDeferred<String>) : Command()
+        class Send(val result: CompletableDeferred<Boolean>) : Command()
+        class Delete() : Command()
+    }
+
     val logDirectory = CompletableDeferred<File>()
     val resourcesDirectory = CompletableDeferred<File>()
 
+    private val commandChannel = spawnActor()
+
     private val problemReportPath = GlobalScope.async(Dispatchers.Default) {
         File(logDirectory.await(), PROBLEM_REPORT_FILE)
     }
 
-    private var collectJob: Deferred<Boolean>? = null
-    private var sendJob: Deferred<Boolean>? = null
-    private var deleteJob: Job? = null
+    private var isCollected = false
 
     var confirmNoEmail: CompletableDeferred<Boolean>? = null
 
     var userEmail = ""
     var userMessage = ""
 
-    val isActive: Boolean
-        get() {
-            synchronized(this) {
-                val collectJob = this.collectJob
-                val sendJob = this.sendJob
-
-                return (collectJob != null && collectJob.isActive) ||
-                    (sendJob != null && sendJob.isActive)
-            }
-        }
-
     init {
         System.loadLibrary("mullvad_jni")
     }
 
     fun collect() {
-        synchronized(this) {
-            if (!isActive) {
-                collectJob = GlobalScope.async(Dispatchers.Default) {
-                    val logDirectoryPath = logDirectory.await().absolutePath
-                    val reportPath = problemReportPath.await().absolutePath
-
-                    deleteReportFile().join()
-                    collectReport(logDirectoryPath, reportPath)
-                }
-            }
-        }
+        commandChannel.sendBlocking(Command.Collect())
     }
 
     suspend fun load(): String {
-        if (collectJob == null) {
-            collect()
-        }
+        val logs = CompletableDeferred<String>()
 
-        if (collectJob?.await() ?: false) {
-            return problemReportPath.await().readText()
-        } else {
-            return "Failed to collect logs for problem report"
-        }
+        commandChannel.send(Command.Load(logs))
+
+        return logs.await()
     }
 
     fun send(): Deferred<Boolean> {
-        synchronized(this) {
-            var currentJob = sendJob
+        val result = CompletableDeferred<Boolean>()
 
-            if (currentJob == null || currentJob.isCompleted) {
-                currentJob = GlobalScope.async(Dispatchers.Default) {
-                    val result = (collectJob?.await() ?: false) &&
-                        sendProblemReport(
-                            userEmail,
-                            userMessage,
-                            problemReportPath.await().absolutePath,
-                            resourcesDirectory.await().absolutePath
-                        )
+        commandChannel.sendBlocking(Command.Send(result))
 
-                    if (result) {
-                        deleteReportFile()
-                    }
+        return result
+    }
 
-                    result
-                }
+    fun deleteReportFile() {
+        commandChannel.sendBlocking(Command.Delete())
+    }
 
-                sendJob = currentJob
-            }
+    private fun spawnActor() = GlobalScope.actor<Command>(Dispatchers.Default, Channel.UNLIMITED) {
+        try {
+            while (true) {
+                val command = channel.receive()
 
-            return currentJob
+                when (command) {
+                    is Command.Collect -> doCollect()
+                    is Command.Load -> command.logs.complete(doLoad())
+                    is Command.Send -> command.result.complete(doSend())
+                    is Command.Delete -> doDelete()
+                }
+            }
+        } catch (exception: ClosedReceiveChannelException) {
         }
     }
 
-    fun deleteReportFile(): Job {
-        synchronized(this) {
-            val oldDeleteJob = deleteJob
+    private suspend fun doCollect() {
+        val logDirectoryPath = logDirectory.await().absolutePath
+        val reportPath = problemReportPath.await().absolutePath
 
-            val job = GlobalScope.launch(Dispatchers.Default) {
-                oldDeleteJob?.join()
-                problemReportPath.await().delete()
-                collectJob = null
-            }
+        doDelete()
+
+        isCollected = collectReport(logDirectoryPath, reportPath)
+    }
 
-            deleteJob = job
+    private suspend fun doLoad(): String {
+        if (!isCollected) {
+            doCollect()
+        }
 
-            return job
+        if (isCollected) {
+            return problemReportPath.await().readText()
+        } else {
+            return "Failed to collect logs for problem report"
         }
     }
 
+    private suspend fun doSend(): Boolean {
+        if (!isCollected) {
+            doCollect()
+        }
+
+        val result = isCollected &&
+            sendProblemReport(
+                userEmail,
+                userMessage,
+                problemReportPath.await().absolutePath,
+                resourcesDirectory.await().absolutePath
+            )
+
+        if (result) {
+            doDelete()
+        }
+
+        return result
+    }
+
+    private suspend fun doDelete() {
+        problemReportPath.await().delete()
+        isCollected = false
+    }
+
     private external fun collectReport(logDirectory: String, reportPath: String): Boolean
     private external fun sendProblemReport(
         userEmail: String,
diff --git a/mullvad-jni/src/lib.rs b/mullvad-jni/src/lib.rs
index 94d5d77313..35fe78dd01 100644
--- a/mullvad-jni/src/lib.rs
+++ b/mullvad-jni/src/lib.rs
@@ -5,6 +5,7 @@ mod classes;
 mod daemon_interface;
 mod is_null;
 mod jni_event_listener;
+mod problem_report;
 mod talpid_vpn_service;
 
 use crate::{daemon_interface::DaemonInterface, jni_event_listener::JniEventListener};
@@ -12,7 +13,7 @@ use jnix::{
     jni::{
         objects::{GlobalRef, JObject, JString, JValue},
         signature::{JavaType, Primitive},
-        sys::{jboolean, jlong, JNI_FALSE, JNI_TRUE},
+        sys::{jboolean, jlong},
         JNIEnv, JavaVM,
     },
     FromJava, IntoJava, JnixEnv,
@@ -935,69 +936,6 @@ pub extern "system" fn Java_net_mullvad_mullvadvpn_service_MullvadDaemon_updateR
     }
 }
 
-#[no_mangle]
-#[allow(non_snake_case)]
-pub extern "system" fn Java_net_mullvad_mullvadvpn_dataproxy_MullvadProblemReport_collectReport(
-    env: JNIEnv<'_>,
-    _: JObject<'_>,
-    logDirectory: JString<'_>,
-    outputPath: JString<'_>,
-) -> jboolean {
-    let env = JnixEnv::from(env);
-    let log_dir_string = String::from_java(&env, logDirectory);
-    let log_dir = Path::new(&log_dir_string);
-    let output_path_string = String::from_java(&env, outputPath);
-    let output_path = Path::new(&output_path_string);
-
-    match mullvad_problem_report::collect_report(&[], output_path, Vec::new(), log_dir) {
-        Ok(()) => JNI_TRUE,
-        Err(error) => {
-            log::error!(
-                "{}",
-                error.display_chain_with_msg("Failed to collect problem report")
-            );
-            JNI_FALSE
-        }
-    }
-}
-
-#[no_mangle]
-#[allow(non_snake_case)]
-pub extern "system" fn Java_net_mullvad_mullvadvpn_dataproxy_MullvadProblemReport_sendProblemReport(
-    env: JNIEnv<'_>,
-    _: JObject<'_>,
-    userEmail: JString<'_>,
-    userMessage: JString<'_>,
-    outputPath: JString<'_>,
-    resourcesDirectory: JString<'_>,
-) -> jboolean {
-    let env = JnixEnv::from(env);
-    let user_email = String::from_java(&env, userEmail);
-    let user_message = String::from_java(&env, userMessage);
-    let output_path_string = String::from_java(&env, outputPath);
-    let output_path = Path::new(&output_path_string);
-    let resources_directory_string = String::from_java(&env, resourcesDirectory);
-    let resources_directory = Path::new(&resources_directory_string);
-
-    let send_result = mullvad_problem_report::send_problem_report(
-        &user_email,
-        &user_message,
-        output_path,
-        resources_directory,
-    );
-
-    match send_result {
-        Ok(()) => JNI_TRUE,
-        Err(error) => {
-            log::error!(
-                "{}",
-                error.display_chain_with_msg("Failed to send problem report")
-            );
-            JNI_FALSE
-        }
-    }
-}
-
 fn log_request_error(request: &str, error: &daemon_interface::Error) {
     match error {
         daemon_interface::Error::RpcError(RestError::Aborted(_)) => {
diff --git a/mullvad-jni/src/problem_report.rs b/mullvad-jni/src/problem_report.rs
new file mode 100644
index 0000000000..fb83c06bdc
--- /dev/null
+++ b/mullvad-jni/src/problem_report.rs
@@ -0,0 +1,73 @@
+use jnix::{
+    jni::{
+        objects::{JObject, JString},
+        sys::{jboolean, JNI_FALSE, JNI_TRUE},
+        JNIEnv,
+    },
+    FromJava, JnixEnv,
+};
+use std::path::Path;
+use talpid_types::ErrorExt;
+
+#[no_mangle]
+#[allow(non_snake_case)]
+pub extern "system" fn Java_net_mullvad_mullvadvpn_dataproxy_MullvadProblemReport_collectReport(
+    env: JNIEnv<'_>,
+    _: JObject<'_>,
+    logDirectory: JString<'_>,
+    outputPath: JString<'_>,
+) -> jboolean {
+    let env = JnixEnv::from(env);
+    let log_dir_string = String::from_java(&env, logDirectory);
+    let log_dir = Path::new(&log_dir_string);
+    let output_path_string = String::from_java(&env, outputPath);
+    let output_path = Path::new(&output_path_string);
+
+    match mullvad_problem_report::collect_report(&[], output_path, Vec::new(), log_dir) {
+        Ok(()) => JNI_TRUE,
+        Err(error) => {
+            log::error!(
+                "{}",
+                error.display_chain_with_msg("Failed to collect problem report")
+            );
+            JNI_FALSE
+        }
+    }
+}
+
+#[no_mangle]
+#[allow(non_snake_case)]
+pub extern "system" fn Java_net_mullvad_mullvadvpn_dataproxy_MullvadProblemReport_sendProblemReport(
+    env: JNIEnv<'_>,
+    _: JObject<'_>,
+    userEmail: JString<'_>,
+    userMessage: JString<'_>,
+    outputPath: JString<'_>,
+    resourcesDirectory: JString<'_>,
+) -> jboolean {
+    let env = JnixEnv::from(env);
+    let user_email = String::from_java(&env, userEmail);
+    let user_message = String::from_java(&env, userMessage);
+    let output_path_string = String::from_java(&env, outputPath);
+    let output_path = Path::new(&output_path_string);
+    let resources_directory_string = String::from_java(&env, resourcesDirectory);
+    let resources_directory = Path::new(&resources_directory_string);
+
+    let send_result = mullvad_problem_report::send_problem_report(
+        &user_email,
+        &user_message,
+        output_path,
+        resources_directory,
+    );
+
+    match send_result {
+        Ok(()) => JNI_TRUE,
+        Err(error) => {
+            log::error!(
+                "{}",
+                error.display_chain_with_msg("Failed to send problem report")
+            );
+            JNI_FALSE
+        }
+    }
+}