Stop the scope editor stalling on a cold app list - #927
Merged
Conversation
Asking the daemon for the installed applications is not the cheap lookup its call site reads as. It goes out as getInstalledPackagesFromAllUsers with filterNoProcess set, and the daemon answers that by querying the package manager for the full component list of every package on the device -- activities, services, receivers and providers, several hundred times over, with a four-call fallback each time a binder buffer overflows on the way back. AppRepository caches the answer for exactly that reason, and the splash prefetch pays for it once where nobody is waiting. What the cache did not have was a way to say that a read was already running. It checked a volatile field and, finding it empty, went to the daemon; two callers arriving together both found it empty and both went. The scope editor is two such callers by construction -- its load reads the list, and the module-package set it filters by reads it again -- so on a cold cache it ran two of those enumerations against each other, one racing the other for the same threads, while the screen it was opening waited on the first. Cold is not the rare case either: every install, update and uninstall drops the cache, and a package event arrives twice, once from the platform and once from the daemon's re-broadcast. So the fetch becomes a job the repository holds rather than work each caller starts. A caller that finds one running joins it, and the mutex guarding that field is never held across the fetch itself, only across the decision. The job runs on the application scope, not the caller's: leaving a screen part-way through a read now leaves the answer behind for the next visit instead of throwing away the several hundred queries it had already paid for. A finished job is retired by the next reader rather than by itself, which is what lets a failed read be retried instead of joined for the life of the process -- a successful one has filled the cache and is never consulted again. A generation counter goes in beside it. A read already in flight when a package event lands has by definition missed what that event carried, and the old code cached its answer regardless, holding a list known to be wrong until the next event -- on a device where nothing else changes, forever. The counter is sampled when a read starts and again before it publishes: the caller that asked still gets the answer, because it is the best that read can offer, but nobody else inherits it. forceRefresh goes. It had no callers, and sharing a running job leaves nothing for it to mean.
ScopeUiState starts with loading set, and the state built on the last line of load was the only other thing that ever wrote it. Every exit that skipped that line therefore left the flag standing, and the screen is drawn entirely behind it: a throw from one of the reads, or the reader leaving while they were still running, and the spinner was there for as long as the view model was. There is no second load to correct it -- load runs once, from init. A try around the body and the flag cleared in the finally, only when it is still set, so the success path stays the single writer of the state it builds. What a failure now shows is the empty list, which is wrong but says so, rather than a wait that never ends. The one read in that body that could throw is the manifest inspection, which opens the module's APK. The package info fetched immediately above it is already guarded for the same hazard -- a package removed or replaced between one call and the next -- and this one was not, so an APK that went away mid-load took the whole manager down from a viewModelScope coroutine. It recovers the way its neighbour does, as no recommended scope, which costs the reader a few rows that would have arrived pre-ticked.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reading the installed applications is not the cheap lookup its call site looks like: it goes out as
getInstalledPackagesFromAllUserswithfilterNoProcessset, and the daemon answers by asking the package manager for the full component list of every installed package.AppRepositorycaches it for that reason, but had no way to say a read was already running, so two callers arriving together both went to the daemon. The scope editor is two such callers by construction —loadreads the list, and the module-package set it filters by reads it again. Cold is the common case: every install, update and uninstall drops the cache, a package event arrives twice, and the Modules panel's own scan does not refill it.Behind that,
ScopeUiStatestarts withloadingset and the state built on the last line ofloadwas the only other thing that wrote it, so any exit skipping that line left the spinner up for the life of the view model.Fixes #917