Ensure all ADB processes exit before questpatcher quits

(closes #181)

Author: Lauriethefish

QuestPatcher.Core/AndroidDebugBridge.cs

@@ -60,7 +60,7 @@ public AdbDevice(string id, string model)
     /// <summary>
     /// Abstraction over using ADB to interact with the Quest.
     /// </summary>
-    public class AndroidDebugBridge
+    public class AndroidDebugBridge : IDisposable
     {
         /// <summary>
         /// Package names that will not be included in the apps to patch list
@@ -95,6 +95,7 @@ public class AndroidDebugBridge
         private readonly IUserPrompter _prompter;
         private readonly string _adbExecutableName = OperatingSystem.IsWindows() ? "adb.exe" : "adb";
         private readonly Action _quit;
+        private readonly ProcessUtil _processUtil = new();
 
         /// <summary>
         /// The minimum time between checks for the currently open ADB daemon.
@@ -144,7 +145,7 @@ public async Task PrepareAdbPath()
             {
                 // Redownloading ADB - existing installation was not valid
                 Log.Information("Existing downloaded ADB was out of date or corrupted - fetching again");
-                await ProcessUtil.InvokeAndCaptureOutput(downloadedAdb, "kill-server"); // Kill server first, otherwise directory will be in use, so can't be deleted.
+                await _processUtil.InvokeAndCaptureOutput(downloadedAdb, "kill-server"); // Kill server first, otherwise directory will be in use, so can't be deleted.
                 _adbPath = await _filesDownloader.GetFileLocation(ExternalFileType.PlatformTools, true);
             }
             else
@@ -166,7 +167,7 @@ private async Task<bool> SetAdbPathIfValid(string adbExecutablePath)
             try
             {
                 Log.Verbose("Checking if ADB at {AdbPath} is present and up-to-date", adbExecutablePath);
-                var outputInfo = await ProcessUtil.InvokeAndCaptureOutput(adbExecutablePath, "version");
+                var outputInfo = await _processUtil.InvokeAndCaptureOutput(adbExecutablePath, "version");
                 string output = outputInfo.AllOutput;
 
                 Log.Debug("Output from checking ADB version: {VerisonOutput}", output);
@@ -225,7 +226,7 @@ private async Task<List<AdbDevice>> ListDevices()
                 await PrepareAdbPath();
             }
 
-            var output = await ProcessUtil.InvokeAndCaptureOutput(_adbPath!, "devices -l");
+            var output = await _processUtil.InvokeAndCaptureOutput(_adbPath!, "devices -l");
             Log.Debug("Listing devices output {Output}", output.AllOutput);
 
             string[] lines = output.StandardOutput.Trim().Split('\n');
@@ -390,7 +391,7 @@ public async Task<ProcessOutput> RunCommand(string command, params int[] allowed
             Log.Debug("Executing ADB command: {Command}", $"adb {command}");
             while (true)
             {
-                var output = await ProcessUtil.InvokeAndCaptureOutput(_adbPath, $"-s {chosenDeviceId} " + command);
+                var output = await _processUtil.InvokeAndCaptureOutput(_adbPath, $"-s {chosenDeviceId} " + command);
                 if (output.StandardOutput.Length > 0)
                 {
                     Log.Verbose("Standard output: {StandardOutput}", output.StandardOutput);
@@ -827,5 +828,11 @@ public async Task<bool> FileExists(string path)
             var directoryFiles = await ListDirectoryFiles(dirName, true);
             return directoryFiles.Contains(Path.GetFileName(path));
         }
+
+        public void Dispose()
+        {
+            _logcatProcess?.Dispose();
+            _processUtil.Dispose(); // Ensure all ADB processes are killed.
+        }
     }
 }

QuestPatcher.Core/ProcessUtil.cs

@@ -1,4 +1,6 @@
-using System.ComponentModel;
+using System;
+using System.Collections.Generic;
+using System.ComponentModel;
 using System.Diagnostics;
 using System.Text;
 using System.Threading.Tasks;
@@ -39,18 +41,27 @@ public struct ProcessOutput
         public string? FullPath { get; set; }
     }
 
-    public static class ProcessUtil
+    /// <summary>
+    /// A utility class for invoking processes and capturing their output.
+    /// Automatically kills all running processes when the instance is disposed.
+    /// This class is thread safe.
+    /// </summary>
+    public class ProcessUtil : IDisposable
     {
+        private readonly HashSet<Process> _runningProcesses = new();
+        private readonly object _processesLock = new object();
+        private bool _disposed = false;
+        
         /// <summary>
         /// Invokes an executable fileName with args and captures its standard and error output.
         /// Waits until the process exits before the task completes.
         /// </summary>
         /// <param name="fileName">File name of the application to call</param>
         /// <param name="arguments">Arguments to pass</param>
         /// <returns>The standard and error output of the process</returns>
-        public static async Task<ProcessOutput> InvokeAndCaptureOutput(string fileName, string arguments)
+        public async Task<ProcessOutput> InvokeAndCaptureOutput(string fileName, string arguments)
         {
-            Process process = new();
+            using Process process = new();
 
             var startInfo = process.StartInfo;
             startInfo.FileName = fileName;
@@ -78,6 +89,11 @@ public static async Task<ProcessOutput> InvokeAndCaptureOutput(string fileName,
             };
 
             process.Start();
+            lock (_processesLock)
+            {
+                _runningProcesses.Add(process);
+            }
+
             string? fullPath = null;
             try
             {
@@ -92,6 +108,15 @@ public static async Task<ProcessOutput> InvokeAndCaptureOutput(string fileName,
             process.BeginErrorReadLine();
 
             await process.WaitForExitAsync();
+            lock (_processesLock)
+            {
+                _runningProcesses.Remove(process);
+            }
+
+            if (_disposed)
+            {
+                throw new OperationCanceledException("QuestPatcher closing, process prematurely ended");
+            }
 
             return new ProcessOutput
             {
@@ -101,5 +126,34 @@ public static async Task<ProcessOutput> InvokeAndCaptureOutput(string fileName,
                 FullPath = fullPath
             };
         }
+
+        public void Dispose()
+        {
+            if(_disposed)
+            {
+                return;
+            }
+            _disposed = true;
+
+            lock(_processesLock)
+            {
+                if(_runningProcesses.Count > 0)
+                {
+                    Log.Information("Killing {NumActiveProcesses} active processes", _runningProcesses.Count);
+                }
+
+                foreach (var process in _runningProcesses)
+                {
+                    try
+                    {
+                        process.Kill();
+                    }
+                    catch (Exception ex)
+                    {
+                        Log.Warning(ex, "Failed to kill process PID {ProcessId} upon exit", process.Id);
+                    }
+                }
+            }
+        }
     }
 }

QuestPatcher.Core/QuestPatcherService.cs

@@ -97,6 +97,10 @@ public void CleanUp()
         {
             Log.Debug("Closing QuestPatcher . . .");
             _configManager.SaveConfig();
+
+            // Kill active ADB processes to ensure that the temp folder can be deleted.
+            DebugBridge.Dispose();
+
             try
             {
                 Directory.Delete(SpecialFolders.TempFolder, true);
@@ -105,6 +109,7 @@ public void CleanUp()
             {
                 Log.Warning("Failed to delete temporary directory");
             }
+
             Log.Debug("Goodbye!");
             Log.CloseAndFlush();
         }