I have ViewModel, those are driven by Scope of `Di...

# coroutines

[ursus]

I have ViewModel, those are driven by Scope of Dispatcher.Main
ViewModel has state, and therefore State (MutableStateFlow<State>) should only be set on main thread -- to avoid synchronization

Is there a way I can assert Scope is on main thread without an android reference? (Looper.myLooper() != Looper.getMainLooper())
(To keep my viewmodels KMP ready)
Or, should I just not care and read-write (reduce) to StateFlow within a synchronization block?

[Adam Powell]

MutableStateFlow writes are thread-safe, you can do them from anywhere

[Adam Powell]

no synchronized blocks required

[ursus]

Yea I know but I reduce them with current value, like this

fun <T> MutableStateFlow<T>.set(reduce: T.() -> T) {
    synchronized(this) {
        value = value.reduce()
    }
}

fun ViewModel.someButtonClick() {
	setState {    
	    copy(counter = counter + 1)
	}
}

[ursus]

and would prefer to not have the synchronized block if in viewmodels since main thread already effectively does that

or, should I not care?

[Adam Powell]

you could do it that way, or you could use a compareAndSet loop, which MutableStateFlow also supports. You could also submit change request messages to a single actor responsible for applying changes. Lots of ways to approach it, all of them have various pros/cons/performance implications

[ursus]

as to avoid the synchr. block?

because I have looked at compareAndSet before and it does use synchronization within, so not sure what the point of it tbh

private fun updateState(expectedState: Any?, newState: Any): Boolean {
        var curSequence = 0
        var curSlots: Array<StateFlowSlot?>? = this.slots // benign race, we will not use it
        synchronized(this) {
            val oldState = _state.value
            if (expectedState != null && oldState != expectedState) return false // CAS support
            ...

or is that its just a "little bit" of synchronization so its okay?

[Adam Powell]

Depends on how your algorithm is set up. There's nothing particularly wrong with using synchronized in cases like this, but you have to use it consistently anywhere that might write to the object.

[Adam Powell]

CAS often sets fewer expectations, which can be useful

[ursus]

Hm, well, react folks told me as to never synchronize on main thread in UI level contstructs, and also unnecessary since main thread has a looper
But I wanted the make the Dispatcher.Main a ctor param, as to test easily, but then I need @VisibleForTesting to disable the assertMainThread, which is ughh

So, youre saying this is fine on android?

fun ViewModel.someButtonClick() {
	synchronized(this) {
		val currentValue = _state.value
        _state.value = currentValue.copy(counter = counter + 1)
    }
}

Or only if synchronized swapped for CAS?

[Adam Powell]

synchronized is fine and android does a ton of it internally where needed, but it's also not magic. Don't do expensive work in synchronized blocks if you can help it. If you synchronize in one place, you have to make sure you synchronize in all of the other places needed to ensure consistency.

[Adam Powell]

taking a monitor lock on art is very fast and probably faster than most lock-free synchronization setups until you reach a level of parallel contention that rarely happens in an android app, as the number of threads per process tends to be much lower for android apps than it can be common in server scenarios. holding a monitor lock during expensive work and thereby blocking other threads from proceeding if they also need to take that lock can have lots of pitfalls.

[Adam Powell]

Your reducer setup above is a good example: you're providing an API for other code to do potentially expensive work while holding a lock that may or may not be contended elsewhere.

[Adam Powell]

If you use a CAS loop you can let that arbitrary user code run outside the lock, and only apply the changes it computed if the inputs didn't change out from under it while the reducer was running. Assuming the reducer has no side effects, if the CAS fails you can simply run the reducer again with the changed inputs. Importantly: only the calling thread attempting to compute and apply changes pays performance costs here, nothing else can be blocked waiting for a reducer to finish.

[Adam Powell]

generally when writing any kind of framework code if you can avoid giving user code a way to run while holding one of your locks, you should. You can't control what people will do with that capability if you offer it.

[ursus]

Right, thanks!
But that brings me back to my first point, I should not synchronize setState in viewModels since 1) not needed because android main thread looper 2) as you said, its potential a foot gun

[ursus]

therefore I'd want to assert main thread in setState, and need to be able to turn this check off for tests via some other setter (ctor param)

[ursus]

and then I guess I'll just abstract the assert body into some interface, to keep it KMP ready

Thanks!

[Adam Powell]

getting into pure opinion at this point, but I tend to regard imposing main thread requirements on other code as a smell. As you've noted, it's a pain to isolate and test. Sometimes you're working with a framework or library that requires it, (e.g. Android views attached to a window requiring UI thread access, the androidx.lifecycle libraries) and you work around it locally there, but imo when writing an abstraction for use elsewhere, either make it entirely thread-safe, or document that it's not thread-safe and requires callers to use some form of external synchronization. Accessing something only from a single thread is one form of external synchronization, but it's up to the caller at that point.

[Adam Powell]

If a caller wants to use synchronized blocks or suspending mutexes or access only via a single actor coroutine, it doesn't matter as any of these are valid external synchronization strategies. If you're going to mandate one, go all the way and just be thread-safe instead.

[Adam Powell]

even the folks working on the androidx.lifecycle library tie themselves in knots regularly trying to keep it testable thanks to the strict main thread mandate.

[ursus]

Interesting. I actually have a sort of a mini framework my self, where I have Scoped types, which means such type has lifecycle (init, close) and holds state

In this sense viewmodel is just a common name for such Scoped subclass which just happens to be close to the ui

And the whole app is just a composition of Scoped instances (Lego bricks)

However one thing, why I mandated the main thread state access in viewmodels, was that, people seem to prefer imperatively accessing state like this

fun buttonClick() {
	val state = _state.value

	if (state.whatever) return
	if (state.somethingElse) {
		...
	}
}

Which is nice and readable and very natural for most people. Same reason why I migrated of rx to coroutines -- to give me the language constructs as the api

[Adam Powell]

yeah that's also why Compose's snapshot state works the way it does

[ursus]

If I were to preventively synchronize, it would be either big synchronized blocks, which is not great, or buy into some framework again, actors as you said, MVI or whatever, kind of defeating my point of migrating (edited) 😕

[ursus]

but then again, those synchronized blocks, would be 100% useless, since android and compose as well send clicks etc on main thread anyways, right?

which means yea leave it to the caller, or a very opinionated type