Stop the scope editor stalling on a cold app list

manager/src/main/kotlin/org/matrix/vector/manager/data/repository/AppRepository.kt

@@ -1,8 +1,14 @@
 package org.matrix.vector.manager.data.repository
 import android.content.pm.ApplicationInfo
 import android.content.pm.PackageManager
+import java.util.concurrent.atomic.AtomicInteger
 import kotlinx.coroutines.CancellationException
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Deferred
 import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.async
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock
 import kotlinx.coroutines.withContext
 import org.matrix.vector.manager.data.model.AppInfo
 import org.matrix.vector.manager.data.model.ModuleDetectionCache
@@ -15,73 +21,125 @@ class AppRepository(
     private val daemonClient: DaemonClient,
     private val packageManager: PackageManager,
     private val moduleDetection: ModuleDetectionCache,
+    private val scope: CoroutineScope,
 ) {
     @Volatile private var cachedApps: List<AppInfo>? = null
     @Volatile private var cachedModulePackages: Set<String>? = null
 
+    /**
+     * Bumped by every [invalidate], so a read already in flight when one lands can tell.
+     *
+     * That read has by definition missed whatever the package event carried, and writing its answer
+     * into the cache afterwards would hold the stale list until the *next* event — on a device
+     * where nothing else changes, indefinitely. The counter is sampled when a read starts and again
+     * before it publishes: on a mismatch the answer still goes back to the caller who asked for it,
+     * because it is the best that read can offer, but it is not cached for anyone else.
+     */
+    private val generation = AtomicInteger(0)
+
+    /** Guards [inFlight] alone. Never held across a fetch — that is what lets a fetch be shared. */
+    private val lock = Mutex()
+
+    /**
+     * The enumeration running right now, so concurrent callers join it rather than each start one.
+     *
+     * Enumerating is expensive in a way the call site cannot see: the daemon answers
+     * `getInstalledPackagesFromAllUsers` with `filterNoProcess = true` by asking the package
+     * manager for the full component list of every installed package, several hundred of them on a
+     * normal device. The scope editor asks twice at once — once from `load`, once for the module
+     * packages it filters by — and with a cold cache both used to go through. That is issue #917:
+     * any install or uninstall drops the cache, so the next module's scope screen ran two of those
+     * enumerations against each other and sat on its spinner until they finished.
+     *
+     * Started on the application scope rather than the caller's, so leaving the screen part-way
+     * through does not throw the work away: the next visit finds the cache warm rather than paying
+     * for the same enumeration again.
+     *
+     * Retired by the next reader that finds it finished rather than by the job itself. A fetch that
+     * succeeded has filled the cache, so the check above this one answers before the field is ever
+     * consulted; a fetch that failed deliberately cached nothing, and dropping the finished job is
+     * exactly what lets the next caller retry instead of joining a failure for the life of the
+     * process.
+     */
+    private var inFlight: Deferred<List<AppInfo>>? = null
+
     /**
      * Drops the cache so the next read goes back to the daemon.
      *
      * Called from the package added, replaced and removed broadcasts: without it a module installed
      * while the manager is open would not appear for the life of the process.
      */
     fun invalidate() {
+        generation.incrementAndGet()
         cachedApps = null
         cachedModulePackages = null
     }
 
-    suspend fun getInstalledApps(forceRefresh: Boolean = false): List<AppInfo> =
-        withContext(Dispatchers.IO) {
-            if (!forceRefresh && cachedApps != null) {
-                return@withContext cachedApps!!
+    suspend fun getInstalledApps(): List<AppInfo> {
+        cachedApps?.let {
+            return it
+        }
+        val job =
+            lock.withLock {
+                // Re-read under the lock. A fetch that finished while this call was waiting has
+                // already published, and joining a job only to learn what the field beside it now
+                // holds is a suspension for nothing.
+                cachedApps?.let {
+                    return it
+                }
+                inFlight?.takeIf { it.isActive }
+                    ?: scope.async(Dispatchers.IO) { fetchInstalledApps() }.also { inFlight = it }
             }
+        return job.await()
+    }
 
-            val flags = PackageManager.MATCH_UNINSTALLED_PACKAGES or PackageManager.GET_META_DATA
+    private suspend fun fetchInstalledApps(): List<AppInfo> {
+        val startedAt = generation.get()
+        val flags = PackageManager.MATCH_UNINSTALLED_PACKAGES or PackageManager.GET_META_DATA
 
-            val result =
-                daemonClient.getInstalledPackagesFromAllUsers(flags, filterNoProcess = true)
-            val failure = result.exceptionOrNull()
-            if (failure != null) {
-                if (failure !is CancellationException) {
-                    logW("apps: installed package list unavailable from daemon", failure)
-                }
-                return@withContext emptyList()
+        val result = daemonClient.getInstalledPackagesFromAllUsers(flags, filterNoProcess = true)
+        val failure = result.exceptionOrNull()
+        if (failure != null) {
+            if (failure !is CancellationException) {
+                logW("apps: installed package list unavailable from daemon", failure)
             }
+            return emptyList()
+        }
 
-            val packages = result.getOrNull() ?: emptyList()
-            val PER_USER_RANGE = 100000
+        val packages = result.getOrNull() ?: emptyList()
+        val PER_USER_RANGE = 100000
 
-            val appList =
-                packages.mapNotNull { pkg ->
-                    val appInfo = pkg.applicationInfo ?: return@mapNotNull null
-                    val isSystem = (appInfo.flags and ApplicationInfo.FLAG_SYSTEM) != 0
-                    // FLAG_IS_GAME was replaced by the category in API 26 and is deprecated, but an
-                    // app built before that still ships it and sets no category, so both are read.
-                    @Suppress("DEPRECATION")
-                    val isGame =
-                        appInfo.category == ApplicationInfo.CATEGORY_GAME ||
-                            (appInfo.flags and ApplicationInfo.FLAG_IS_GAME) != 0
+        val appList =
+            packages.mapNotNull { pkg ->
+                val appInfo = pkg.applicationInfo ?: return@mapNotNull null
+                val isSystem = (appInfo.flags and ApplicationInfo.FLAG_SYSTEM) != 0
+                // FLAG_IS_GAME was replaced by the category in API 26 and is deprecated, but an
+                // app built before that still ships it and sets no category, so both are read.
+                @Suppress("DEPRECATION")
+                val isGame =
+                    appInfo.category == ApplicationInfo.CATEGORY_GAME ||
+                        (appInfo.flags and ApplicationInfo.FLAG_IS_GAME) != 0
 
-                    val userId = appInfo.uid / PER_USER_RANGE
+                val userId = appInfo.uid / PER_USER_RANGE
 
-                    AppInfo(
-                        packageName = pkg.packageName,
-                        userId = userId,
-                        appName = appInfo.loadLabel(packageManager).toString(),
-                        isSystemApp = isSystem,
-                        isGame = isGame,
-                        isSelectedInScope = false, // To be merged later in the ViewModel
-                        isRecommended = false,
-                        lastUpdateTime = pkg.lastUpdateTime,
-                        firstInstallTime = pkg.firstInstallTime,
-                        versionCode = pkg.versionCodeCompat,
-                        applicationInfo = appInfo,
-                    )
-                }
+                AppInfo(
+                    packageName = pkg.packageName,
+                    userId = userId,
+                    appName = appInfo.loadLabel(packageManager).toString(),
+                    isSystemApp = isSystem,
+                    isGame = isGame,
+                    isSelectedInScope = false, // To be merged later in the ViewModel
+                    isRecommended = false,
+                    lastUpdateTime = pkg.lastUpdateTime,
+                    firstInstallTime = pkg.firstInstallTime,
+                    versionCode = pkg.versionCodeCompat,
+                    applicationInfo = appInfo,
+                )
+            }
 
-            cachedApps = appList
-            return@withContext appList
-        }
+        if (generation.get() == startedAt) cachedApps = appList
+        return appList
+    }
 
     /**
      * Which installed packages are themselves Xposed modules.
@@ -101,6 +159,7 @@ class AppRepository(
             cachedModulePackages?.let {
                 return@withContext it
             }
+            val startedAt = generation.get()
             val packages =
                 getInstalledApps()
                     .asSequence()
@@ -116,7 +175,7 @@ class AppRepository(
                     }
                     .map { it.packageName }
                     .toSet()
-            cachedModulePackages = packages
+            if (generation.get() == startedAt) cachedModulePackages = packages
             packages
         }
 }

manager/src/main/kotlin/org/matrix/vector/manager/di/ServiceLocator.kt

@@ -117,7 +117,7 @@ object ServiceLocator {
     val modules: ModuleRepository by lazy { ModuleRepository(daemon, appScope) }
 
     val apps: AppRepository by lazy {
-        AppRepository(daemon, context.packageManager, moduleDetection)
+        AppRepository(daemon, context.packageManager, moduleDetection, appScope)
     }
 
     /**