George
08/05/2022, 7:17 PM@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
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?dewildte
08/05/2022, 7:43 PMLaunchedEffect(key1 = Unit) {
viewModel.load()
}
Zach Klippenstein (he/him) [MOD]
08/05/2022, 8:02 PMSnapshot.withMutableSnapshot
?Bryan Herbst
08/05/2022, 8:03 PMLaunchedEffect
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.Zach Klippenstein (he/him) [MOD]
08/05/2022, 8:05 PMGeorge
08/05/2022, 9:27 PMSnapshot.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.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.
Alex Vanyo
08/05/2022, 9:58 PMI 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.George
08/05/2022, 9:58 PMSimply instantiating an object shouldn’t trigger I/O operationsConstructor 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? ;)The solution of moving it into a LaunchedEffectalso 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.dewildte
08/05/2022, 11:05 PMfun 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
?George
08/06/2022, 12:05 AMThe bottom line is that the solutions proposed to you workSure. 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 inEmpty 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.
Zach Klippenstein (he/him) [MOD]
08/06/2022, 1:30 AMviewModelScope
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.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.
suspend fun awaitGlobalSnapshotReady() {
suspendCancellableCoroutine { continuation ->
lateinit var handle: ObserverHandle
handle = Snapshot.registerApplyObserver { _, _ ->
handle.dispose()
continuation.resume(Unit)
}
continuation.invokeOnCancellation {
handle.dispose()
}
}
}
George
08/06/2022, 8:27 AMPut both the state initialization and the write in explicit snapshotsWow, 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 applicationEven 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)
Zach Klippenstein (he/him) [MOD]
08/06/2022, 3:20 PMviewModel
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.George
08/06/2022, 5:38 PMif 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 indefinitelyI 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 allThanks for the clarification, I haven't realized this.
it would have to handle the case where the snapshot is disposed without applicationJust curious: what may cause composition to fail in a typical Android app? How often does that happen?
Zach Klippenstein (he/him) [MOD]
08/07/2022, 5:53 AMGeorge
08/07/2022, 11:57 AM@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.Zach Klippenstein (he/him) [MOD]
08/07/2022, 2:57 PM