https://kotlinlang.org logo
#compose
Title
# compose
g

George

08/05/2022, 7:17 PM
Compose + ViewModel + MutableState combination is error-prone due to snapshots mechanics. Shouldn't something be done about it? (details inside)
Suppose we have something like this:
Copy code
@Composable
fun MyComposable(viewModel: MyViewModel = viewModel()) {
    Text(text = "${viewModel.loading}")
}

class MyViewModel : ViewModel() {
    var loading by mutableStateOf(false)

    init {
        viewModelScope.launch {
            loading = true
            SomeRepo.loadSomeData()
            loading = false
        }
    }
}
I suppose this is a common pattern for loading data once we get to a certain screen. It worked fine with Fragments and StateFlow. It works in Compose too, but only accidentally. If later: 1) we add more logic around loading data and move it all to
<http://Dispatchers.IO|Dispatchers.IO>
to prevent jank, or 2) future versions of Compose implement parallel composition on non-main threads (there were such plans, AFAIR), this will crash with an
"IllegalStateException: Reading a state that was created after the snapshot was taken or in a snapshot that has not yet been applied"
. This is already a well-known issue which many developers have hit. It is caused by transactional nature of Compose snapshots. Their mechanics is covered by Zach Klippenstein in his wonderful articles and the implications explained by Compose experts here. Unfortunately, I don't see any good solutions. We can't create ViewModel earlier, outside of composition, because we might need to pass arguments ("nav args",
SavedStateHandle
) from Composable to ViewModel. We can't create it later, e.g. in
LaunchedEffect
, because Composable needs the values from ViewModel to render itself. We can extract this logic to
Copy code
fun initialize() {
  if (initialized) return
  ...
  initialized = true
}
and call it from
LaunchedEffect
. But I would say it is a large step back architecture-wise (like navigation-compose is a step back from navigation-fragment, sorry). ViewModel should not depend on UI behavior that much, it should be able to launch whatever logic it needs from the
init
block (ideally being agnostic to whether UI is in Compose or not). And Composable should already show loading state on first composition, not waiting for another frame. From Compose point of view the creation of ViewModel is just a "side effect" of composition, and we should somehow roll back those side effects if the composition fails to apply. But from app developer's standpoint, creation of ViewModel (and all the Repositories, UseCases, etc, it could bring to life) cannot and should not be "undone". ViewModel outlives not only the current composition and Composable, but also the entire UI, in case of configuration changes. I think this is an architecture issue, which ideally should be solved somewhere between Compose and Jetpack Lifecycle. I don't know enough low-level mechanics to make suggestions, maybe there is a way to make this ViewModel creation happen in a global snapshot (while keeping it synchronous), or to apply current snapshot immediately, not waiting for composition to complete, or to suppress these errors. If solving the problem is technically impossible, then we might need clear guidance and Lint checks to prevent unsafe usage of
MutableState
, or maybe to revert back to `StateFlow`s (though I must admit that
MutableState
is nicer to use and especially to update). What do you think? And BTW, can anybody explain why Compose puts an emphasis on fail-and-rollback scenario? In theory there might be conflicting writes in different snapshots, sure, but what can fail in practice in a typical app, where Compose is just rendering the UI?
d

dewildte

08/05/2022, 7:43 PM
What do you think @George?
On top of this you could have the load happen more explicitly.
Copy code
LaunchedEffect(key1 = Unit) {
  viewModel.load()
}
z

Zach Klippenstein (he/him) [MOD]

08/05/2022, 8:02 PM
What if you wrap your state writes
Snapshot.withMutableSnapshot
?
b

Bryan Herbst

08/05/2022, 8:03 PM
The solution of moving it into a
LaunchedEffect
is my preferred route - IMO having data loading be a side-effect of an
init
block is rarely what you want. Simply instantiating an object shouldn’t trigger I/O operations.
z

Zach Klippenstein (he/him) [MOD]

08/05/2022, 8:05 PM
Yea i agree that constructors shouldn't perform side effects. But in general, if you need to perform snapshot operations on a background thread, you probably are going to end up wanting to do them in a snapshot anyway for atomicity/consistency/isolation reasons.
g

George

08/05/2022, 9:27 PM
@Zach Klippenstein (he/him) [MOD] Unfortunately
Snapshot.withMutableSnapshot
doesn't seem to help here. If I understood your articles correctly, this new snapshot is a child of the current one, which is still not applied.
I see nothing inherently wrong with having data loading be a side-effect of an
init
block. I think it's better than it being a side effect of some ephemeral Composable. Both from practical standpoint (in the latter case loading will be triggered again when navigating to a new screen and back) and architectural one (I prefer ViewModel and lower layers to be as independent of UI behavior and self-contained as possible).
Try keeping Compose out of the ViewModel by using (State)Flows instead.
Reverting to `StateFlow`s is definitely possible, it's just more boilerplate and more work for runtime to convert one reactive system to another.
a

Alex Vanyo

08/05/2022, 9:58 PM
I think it’s better than it being a side effect of some ephemeral Composable.
In some sense, that’s what the
init
block in a
ViewModel
is: a side effect from a Composable. When you request the
ViewModel
for the very first time, it’s because a composable is requesting it. So the precise time when the
ViewModel
is created and when
init
is called is as a side effect of a Composable coming into existence. But it’s also an uncontrolled side-effect, by which I mean it isn’t using a
LaunchedEffect
or
DisposableEffect
that is “safe” from the Compose side of things.
g

George

08/05/2022, 9:58 PM
Simply instantiating an object shouldn’t trigger I/O operations
Constructor itself shouldn't perform (synchronous) I/O, sure. But the app needs to load data as soon as it is started, or a specific screen is reached. In this case starting the operation upon creation of a Repository or a ViewModel looks to me like a most clear & direct representation of this requirement in code. For example, when we're calling
Room.databaseBuilder(...).build()
, it probably involves a lot of I/O. We don't tell it explicitly later "check if DB file already exists", "run migrations if needed", etc. I think it's called "encapsulation", what's wrong with it? ;)
@Alex Vanyo Yes, that's exactly the architectural problem I'm talking about, a discrepancy between how Compose sees the ViewModel and how app developers have been using it since its introduction many years ago as a solid rock beneath the ever-changing UI layer. BTW, the docs specifically argue that the responsibility for "loading data from a database or network" and other asynchronous calls should be moved from UI to VM. I started this thread in hope that this mechanism of VM instantiation can be changed somehow to solve the issue.
Given its repeated appearance here, on SO and issue tracker, I suppose it's not just my personal issue. Currently it may be still a rare combination of factors, but once Compose becomes multithreaded, it can hit a lot of apps, I'm afraid.
The solution of moving it into a LaunchedEffect
also has a downside that the first composition will have an inconsistent state to display: not loading and no data. Or we would need to initialize
loading
to
true
, which looks rather counter-intuitive if the VM doesn't control the loading.
d

dewildte

08/05/2022, 11:05 PM
Correct me if I am wrong but what I think you are missing is that the UI layer is the ultimate controller of the applications universe. ViewModel is just a tool for the UI, and when I say UI I really mean the Android layer. Think Application, Activity, Fragment, View. The domain layer does not get instantiated first, it is the Application and then the rest.
fun MyComposable(viewModel: MyViewModel = viewModel())
Who is controlling who here? The UI is creating the tool it needs. Ultimately the UI is the root controller. Trying to force such a hard separation between the two like you are implying is causing the issue here. The bottom line is that the solutions proposed to you work. Standing here and debating that until you are blue in the face is not serving you any good. Having an object automatically start working does make it hard to control. Can it be done? Sure, I have made some stuff that does things in it’s constructor too. But I have also seen other devs (one was my manager at a point) make an object start doing things and making calls upon instantiation and watched it shoot him in the foot. Imagine if vehicles just started driving forward when you start them. To keep things SOLID constructing an object is part of the S in SOLID. There are some pragmatic exceptions but those are few and far between.
the first composition will have an inconsistent state to display: not loading and no data.
This is called an EMPTY state and is a perfectly valid state to be in. And you could even have the default for
loading
be
true
?
g

George

08/06/2022, 12:05 AM
The bottom line is that the solutions proposed to you work
Sure. I've mentioned all of these options in my first post and explained their downsides (from my point of view). And I'm not going to convince anybody that this is a best possible architecture. What I'm trying to say is: this is a valid use case too, which worked reliably with Fragments but may have an unexpected gotcha in pure Compose. If this could be solved technically, once and for all, it would be the best possible outcome for everyone.
This is called an EMPTY state and is a perfectly valid state to be in
Empty data may be fine ("your basket is empty") or represent an error ("order not found"), but in either case I wouldn't show that for a split second before we had a chance to actually load the data. I'd call it an "inconsistent" UI, sorry if I wasn't clear enough.
z

Zach Klippenstein (he/him) [MOD]

08/06/2022, 1:30 AM
You mentioned initially that the code you posted actually works, and it only breaks if you change dispatchers. I believe that’s because
viewModelScope
uses
Dispatchers.Main.immediate
, which means the coroutine starts undispatched and so your first state access happens in the same snapshot as the initialization. I also tested using
Dispatchers.Main
instead, and that also works, because that dispatcher will dispatch the coroutine to the main thread, which means it won’t run until after the composition phase is finished and its snapshot is applied. I think this requirement to use one of the main dispatchers, is perfectly reasonable. There is a lot of Android code that will suddenly break if you just move it to a non-UI thread. Requiring UI code to run on the UI thread is pretty common. And this should work even when compose becomes multi-threaded, I’m pretty sure, because I believe the plan is to fan-out/fan-in composition from the UI thread, but it would still need to be synchronized with the UI thread to keep the phases ordered correctly and ensure everything’s done for the right frame, which means anything dispatched to the UI thread’s event loop will still run after the composition pass is done. In that case, both
Main
and
Main.immediate
would end up dispatching to the UI thread, and defer your coroutine start until after composition.
That said, just for interest’s sake, some other terrible, hacky, very bad ideas that I just played around with are: 1. Put both the state initialization and the write in explicit snapshots. E.g. initialize the ViewModel in a snapshot you can explicitly apply, or just the property (
var loading by Snapshot.withMutableSnapshot { mutableStateOf(false) }
). I’m not sure how feasible changing the VM initialization is, and doing this per-property is not very efficient. It also still requires the write in the coroutine to be in a snapshot, which is more verbose. 2. Launch the coroutine with
CoroutineStart.UNDISPATCHED
, so that first write happens in the snapshot of the current composition. Depending on what your coroutine is doing, this might not actually be desirable, and is brittle because it won’t work if your coroutine hops threads then does another state operation before the composition’s snapshot is applied. 3. Make your coroutine explicitly wait for the next global snapshot application, with a function something like below. This isn’t great because it creates a lot more coupling between your VM and the snapshot system’s behavior, and you have to remember to call this function from every coroutine you launch in a VM initializer.
Copy code
suspend fun awaitGlobalSnapshotReady() {
    suspendCancellableCoroutine { continuation ->
        lateinit var handle: ObserverHandle
        handle = Snapshot.registerApplyObserver { _, _ ->
            handle.dispose()
            continuation.resume(Unit)
        }
        continuation.invokeOnCancellation {
            handle.dispose()
        }
    }
}
g

George

08/06/2022, 8:27 AM
Zach, thank you so much for diving deep into it! If you're sure both "main" dispatchers will still work fine with multi-threaded composition, that's actually a great news! At least the code that's working now won't suddenly break in the future. And the rest can be alleviated by clear docs and Lint checks. "Main" dispatchers for mutating shared state are typically a better choice anyway, to avoid concurrency bugs.
Put both the state initialization and the write in explicit snapshots
Wow, that works, but honestly I can't understand why. When a nested snapshot is taken and immediately applied, what difference does it make?
Make your coroutine explicitly wait for the next global snapshot application
Even if it's not practical for this use case, it's exciting to know that such a thing is possible. But there is no way to "bubble up" VM initialization to global snapshot or to apply current snapshot prematurely, right? (although I'm not sure if the latter, called immediately after VM creation, would solve the issue: it might be too late)
z

Zach Klippenstein (he/him) [MOD]

08/06/2022, 3:20 PM
You could break out of the current snapshot and initialize the VM in the global snapshot explicitly, but then it wouldn't be available in the composition snapshot at all. And in general, I don't think somehow early half-applying the composition snapshot would make sense since it would have to handle the case where the snapshot is disposed without application.
Also, looking at the source of the
viewModel
composable, it might be buggy - it calls through to the non-composable VM provider directly in composition, every time. So if the composition is abandoned, any VMs it's caused to instantiate won't be disposed, which means that any side effects performed in the VM constructor (such as coroutines launched in the vmScope) won't be disposed and will just be leaked by being allowed to continue to run indefinitely. I am not very familiar with the details of all the VM machinery but just looking at the source on my phone it sure looks like a potential leak. (Filed a bug) One more reason not to perform side effects in constructors.
g

George

08/06/2022, 5:38 PM
if the composition is abandoned, any VMs it's caused to instantiate won't be disposed, which means that any side effects performed in the VM constructor (such as coroutines launched in the vmScope) won't be disposed and will just be leaked by being allowed to continue to run indefinitely
I would say it's a feature, not a bug. Next composition will get from
ViewModelStoreOwner
the same instance of VM, already up and running. The same happens on configuration change: UI (Activity / Fragment / Composable) is destroyed, but ViewModel continues to live and perform asynchronous operations, waiting to be picked up by the new UI instance. Technically it's a "leakage" too, but it's one of the main reasons to use a ViewModel at all.
You could break out of the current snapshot and initialize the VM in the global snapshot explicitly, but then it wouldn't be available in the composition snapshot at all
Thanks for the clarification, I haven't realized this.
it would have to handle the case where the snapshot is disposed without application
Just curious: what may cause composition to fail in a typical Android app? How often does that happen?
z

Zach Klippenstein (he/him) [MOD]

08/07/2022, 5:53 AM
It's a bug because nothing guarantees that anything will ever ask for that ViewModel again.
The only case I know of right now is that I believe lazy lists will pre-compose their off-screen-but-soon-to-be-visible items during scroll animations, and those compositions will be disposed of if the scroll is interrupted and reverses direction. I think view pagers do this as well (possibly by using the general lazy list machinery), so any app that initialized a VM in a view pager page would be affected. The compose UI library may introduce more cases in the future though, which would automatically become part of any typical android app.
g

George

08/07/2022, 11:57 AM
AFAIK, out-of-the-box ViewModels can be scoped only to Activity, Fragment, and Navigation Graph or Destination (though there are third-party libraries to scope them to individual Composables). The elements of a lazy list should all get the same instance of a ViewModel, it's typically not what we want. So, in a sense this "scope holder" guarantees that it won't just disappear into thin air, and if it's destroyed completely (not due to configuration change), it should clean up all its VMs as well, closing their coroutine scopes. There might be an edge case when VM is only used for a part of the screen, and that part has begun its composition, but never completed it. Don't know how realistic is that. This leaves us essentially with three options: 1) Make current behavior more explicit by creating ViewModel in the "scope holder" itself, before composition, and pass it down to Composables. It's currently possible with Fragments, but may require reworking navigation-compose and the libraries that depend on it (Hilt, Koin, Compose Destinations...). 2) Move ViewModel initialization to a separate method:
Copy code
@Composable
fun MyComposable(viewModel: MyViewModel = viewModel()) {
    LaunchedEffect(Unit) {
        viewModel.initialize()
    }
    Text(text = "${viewModel.loading}")
}

class MyViewModel : ViewModel() {
    // `loading` should be initialized to `true` to prevent showing wrong state on first composition, before LaunchedEffect kicks in
    var loading by mutableStateOf(true)
    private var initialized = AtomicBoolean(false)

    fun initialize() {
        if (!initialized.compareAndSet(false, true)) {
            return
        }
        viewModelScope.launch(<http://Dispatchers.IO|Dispatchers.IO>) {
            loading = true
            SomeRepo.loadSomeData()
            loading = false
        }
    }
}
This has some downsides (more boilerplate, counter-intuitive initializing
loading
to
true
, extra delay before
loadSomeData
actually starts), but in general should work reliably. 3) Keep everything as-is (see my first post), be careful to use "main" dispatchers for state updates and hope for the best. 😉 Consensus in this thread seems to be in favor of option #2, I'll keep that in mind.
z

Zach Klippenstein (he/him) [MOD]

08/07/2022, 2:57 PM
I filed the leak bug
1741 Views