diff --git a/formula-android-tests/build.gradle.kts b/formula-android-tests/build.gradle.kts index e0e81567..5769be9b 100644 --- a/formula-android-tests/build.gradle.kts +++ b/formula-android-tests/build.gradle.kts @@ -40,11 +40,11 @@ dependencies { implementation(libs.lifecycle.extensions) implementation(libs.androidx.test.core.ktx) - testImplementation(libs.junit) - testImplementation(libs.truth) testImplementation(libs.androidx.test.junit) testImplementation(libs.androidx.test.rules) testImplementation(libs.androidx.test.runner) testImplementation(libs.espresso.core) + testImplementation(libs.junit) testImplementation(libs.robolectric) + testImplementation(libs.truth) } diff --git a/formula-android-tests/src/test/java/com/instacart/formula/FragmentFlowRenderViewTest.kt b/formula-android-tests/src/test/java/com/instacart/formula/FragmentFlowRenderViewTest.kt index 4d886429..7f50a911 100644 --- a/formula-android-tests/src/test/java/com/instacart/formula/FragmentFlowRenderViewTest.kt +++ b/formula-android-tests/src/test/java/com/instacart/formula/FragmentFlowRenderViewTest.kt @@ -1,5 +1,6 @@ package com.instacart.formula +import android.os.Looper import androidx.fragment.app.Fragment import androidx.lifecycle.Lifecycle import androidx.test.core.app.ActivityScenario @@ -9,6 +10,8 @@ import com.google.common.truth.Truth.assertThat import com.instacart.formula.android.FragmentFlowState import com.instacart.formula.android.FragmentKey import com.instacart.formula.android.BackCallback +import com.instacart.formula.android.FragmentEnvironment +import com.instacart.formula.rxjava3.toObservable import com.instacart.formula.test.TestKey import com.instacart.formula.test.TestKeyWithId import com.instacart.formula.test.TestFragmentActivity @@ -19,7 +22,11 @@ import org.junit.Rule import org.junit.Test import org.junit.rules.RuleChain import org.junit.runner.RunWith +import org.robolectric.Shadows import org.robolectric.annotation.LooperMode +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit @RunWith(AndroidJUnit4::class) class FragmentFlowRenderViewTest { @@ -29,6 +36,7 @@ class FragmentFlowRenderViewTest { private var lastState: FragmentFlowState? = null private val stateChangeRelay = PublishRelay.create>() private var onPreCreated: (TestFragmentActivity) -> Unit = {} + private var updateThreads = linkedSetOf() private val formulaRule = TestFormulaRule( initFormula = { app -> FormulaAndroid.init(app) { @@ -40,6 +48,8 @@ class FragmentFlowRenderViewTest { }, onRenderFragmentState = { a, state -> lastState = state + + updateThreads.add(Thread.currentThread()) }, contracts = { bind(TestFeatureFactory { stateChanges(it) }) @@ -52,6 +62,7 @@ class FragmentFlowRenderViewTest { }, cleanUp = { lastState = null + updateThreads = linkedSetOf() }) private val activityRule = ActivityScenarioRule(TestFragmentActivity::class.java) @@ -199,6 +210,51 @@ class FragmentFlowRenderViewTest { assertThat(activeContracts()).containsExactly(TestKey(), TestKeyWithId(1)).inOrder() } + @Test fun `background feature events are moved to the main thread`() { + + val executor = Executors.newSingleThreadExecutor() + val latch = CountDownLatch(1) + + val initial = TestKey() + val keyWithId = TestKeyWithId(1) + + navigateToTaskDetail(1) + // Both contracts should be active. + assertThat(activeContracts()).containsExactly(TestKey(), TestKeyWithId(1)).inOrder() + + // Pass feature updates on a background thread + executor.execute { + stateChangeRelay.accept(initial to "main-state-1") + stateChangeRelay.accept(initial to "main-state-2") + stateChangeRelay.accept(initial to "main-state-3") + + stateChangeRelay.accept(keyWithId to "detail-state-1") + stateChangeRelay.accept(keyWithId to "detail-state-2") + stateChangeRelay.accept(keyWithId to "detail-state-3") + latch.countDown() + } + + // Wait for background execution to finish + if(!latch.await(100, TimeUnit.MILLISECONDS)) { + throw IllegalStateException("timeout") + } + + Shadows.shadowOf(Looper.getMainLooper()).idle() + + val currentState = lastState?.states.orEmpty() + .mapKeys { it.key.key } + .mapValues { it.value.renderModel } + + val expected = mapOf( + TestKey() to "main-state-3", + TestKeyWithId(1) to "detail-state-3" + ) + + assertThat(currentState).isEqualTo(expected) + assertThat(updateThreads).hasSize(1) + assertThat(updateThreads).containsExactly(Thread.currentThread()) + } + private fun navigateBack() { scenario.onActivity { it.onBackPressed() } } diff --git a/formula-android-tests/src/test/java/com/instacart/formula/android/internal/AndroidUpdateSchedulerTest.kt b/formula-android-tests/src/test/java/com/instacart/formula/android/internal/AndroidUpdateSchedulerTest.kt new file mode 100644 index 00000000..dd8658be --- /dev/null +++ b/formula-android-tests/src/test/java/com/instacart/formula/android/internal/AndroidUpdateSchedulerTest.kt @@ -0,0 +1,77 @@ +package com.instacart.formula.android.internal + +import android.os.Looper +import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.Shadows.shadowOf +import java.util.LinkedList +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit + +@RunWith(AndroidJUnit4::class) +class AndroidUpdateSchedulerTest { + + @Test fun `when an update triggers another update, scheduler finishes first one before proceeding to the next`() { + val computedValues = LinkedList() + val scheduler = AndroidUpdateScheduler<() -> String> { valueComputation -> + val value = valueComputation() + computedValues.addLast(value) + } + + scheduler.emitUpdate { + scheduler.emitUpdate { "next" } + "first" + } + + assertThat(computedValues).containsExactly("first", "next").inOrder() + } + + @Test fun `when update arrives on bg thread, handle it on main thread`() { + val computedValues = LinkedList() + val scheduler = AndroidUpdateScheduler<() -> String> { valueComputation -> + val value = valueComputation() + computedValues.addLast(value) + } + + val latch = CountDownLatch(1) + Executors.newSingleThreadExecutor().execute { + scheduler.emitUpdate { "bg update" } + latch.countDown() + } + + if (!latch.await(100, TimeUnit.MILLISECONDS)) { + throw IllegalStateException("timeout") + } + + shadowOf(Looper.getMainLooper()).idle() + assertThat(computedValues).containsExactly("bg update").inOrder() + } + + @Test fun `when multiple updates arrive on bg thread before main thread is ready, we handle only last`() { + val computedValues = LinkedList() + val scheduler = AndroidUpdateScheduler<() -> String> { valueComputation -> + val value = valueComputation() + computedValues.addLast(value) + } + + val latch = CountDownLatch(1) + Executors.newSingleThreadExecutor().execute { + scheduler.emitUpdate { "bg update-1" } + scheduler.emitUpdate { "bg update-2" } + scheduler.emitUpdate { "bg update-3" } + scheduler.emitUpdate { "bg update-4" } + latch.countDown() + } + + if (!latch.await(100, TimeUnit.MILLISECONDS)) { + throw IllegalStateException("timeout") + } + + shadowOf(Looper.getMainLooper()).idle() + assertThat(computedValues).containsExactly("bg update-4").inOrder() + } +} \ No newline at end of file diff --git a/formula-android/build.gradle.kts b/formula-android/build.gradle.kts index 96c89066..bffcb1c6 100644 --- a/formula-android/build.gradle.kts +++ b/formula-android/build.gradle.kts @@ -31,12 +31,15 @@ dependencies { api(libs.rxandroid) api(libs.rxrelay) + testImplementation(libs.androidx.test.rules) testImplementation(libs.androidx.test.runner) + testImplementation(libs.androidx.test.junit) testImplementation(libs.espresso.core) - testImplementation(libs.truth) + testImplementation(libs.kotlin.reflect) testImplementation(libs.mockito.core) testImplementation(libs.mockito.kotlin) - testImplementation(libs.kotlin.reflect) + testImplementation(libs.robolectric) + testImplementation(libs.truth) } diff --git a/formula-android/src/main/java/com/instacart/formula/android/FragmentFlowStore.kt b/formula-android/src/main/java/com/instacart/formula/android/FragmentFlowStore.kt index d0e408bd..65fa2053 100644 --- a/formula-android/src/main/java/com/instacart/formula/android/FragmentFlowStore.kt +++ b/formula-android/src/main/java/com/instacart/formula/android/FragmentFlowStore.kt @@ -5,6 +5,7 @@ import com.instacart.formula.Formula import com.instacart.formula.Snapshot import com.instacart.formula.android.internal.Binding import com.instacart.formula.android.events.FragmentLifecycleEvent +import com.instacart.formula.android.internal.FeatureObservableAction import com.instacart.formula.rxjava3.RxAction import com.instacart.formula.rxjava3.toObservable import com.jakewharton.rxrelay3.PublishRelay @@ -122,14 +123,18 @@ class FragmentFlowStore @PublishedApi internal constructor( val fragmentId = entry.key val feature = (entry.value as? FeatureEvent.Init)?.feature if (feature != null) { - RxAction.fromObservable(feature) { - feature.state.onErrorResumeNext { - input.onScreenError(fragmentId.key, it) - Observable.empty() + val action = FeatureObservableAction( + fragmentEnvironment = input, + fragmentId = fragmentId, + feature = feature, + ) + action.onEvent { + if (state.activeIds.contains(fragmentId)) { + val keyState = FragmentState(fragmentId.key, it) + transition(state.copy(states = state.states.plus(fragmentId to keyState))) + } else { + none() } - }.onEvent { - val keyState = FragmentState(fragmentId.key, it) - transition(state.copy(states = state.states.plus(fragmentId to keyState))) } } } diff --git a/formula-android/src/main/java/com/instacart/formula/android/internal/ActivityManager.kt b/formula-android/src/main/java/com/instacart/formula/android/internal/ActivityManager.kt index 8c42699d..9089aeb2 100644 --- a/formula-android/src/main/java/com/instacart/formula/android/internal/ActivityManager.kt +++ b/formula-android/src/main/java/com/instacart/formula/android/internal/ActivityManager.kt @@ -19,12 +19,6 @@ internal class ActivityManager( private val store: ActivityStore ) { - private val fragmentState = store - .contracts - .state(environment) - .doOnNext(delegate.fragmentFlowStateRelay::accept) - .replay(1) - internal val stateSubscription: Disposable private var uiSubscription: Disposable? = null private var fragmentRenderView: FragmentFlowRenderView? = null @@ -32,11 +26,11 @@ internal class ActivityManager( init { stateSubscription = if (store.streams != null) { val disposables = CompositeDisposable() - disposables.add(fragmentState.connect()) + disposables.add(subscribeToFragmentStateChanges()) disposables.add(store.streams.invoke(StreamConfiguratorIml(delegate))) disposables } else { - fragmentState.connect() + subscribeToFragmentStateChanges() } } @@ -61,9 +55,8 @@ internal class ActivityManager( delegate.attachActivity(activity) delegate.onLifecycleStateChanged(Lifecycle.State.CREATED) val renderView = fragmentRenderView ?: throw callOnPreCreateException(activity) - uiSubscription = fragmentState.subscribe { - Utils.assertMainThread() + uiSubscription = delegate.fragmentFlowState().subscribe { renderView.render(it) store.onRenderFragmentState?.invoke(activity, it) } @@ -113,6 +106,13 @@ internal class ActivityManager( return fragmentRenderView?.viewFactory(fragment) } + private fun subscribeToFragmentStateChanges(): Disposable { + return store + .contracts + .state(environment) + .subscribe(delegate.fragmentFlowStateRelay::accept) + } + private fun callOnPreCreateException(activity: FragmentActivity): IllegalStateException { return IllegalStateException("please call onPreCreate before calling Activity.super.onCreate(): ${activity::class.java.simpleName}") } diff --git a/formula-android/src/main/java/com/instacart/formula/android/internal/ActivityStoreContextImpl.kt b/formula-android/src/main/java/com/instacart/formula/android/internal/ActivityStoreContextImpl.kt index eb445af3..f64694c7 100644 --- a/formula-android/src/main/java/com/instacart/formula/android/internal/ActivityStoreContextImpl.kt +++ b/formula-android/src/main/java/com/instacart/formula/android/internal/ActivityStoreContextImpl.kt @@ -72,8 +72,12 @@ internal class ActivityStoreContextImpl : ActivityS override fun send(effect: Activity.() -> Unit) { // We allow emitting effects only after activity has started - startedActivity()?.effect() ?: run { - // Log missing activity. + if (Utils.isMainThread()) { + startedActivity()?.effect() + } else { + Utils.mainThreadHandler.post { + startedActivity()?.effect() + } } } diff --git a/formula-android/src/main/java/com/instacart/formula/android/internal/AndroidUpdateScheduler.kt b/formula-android/src/main/java/com/instacart/formula/android/internal/AndroidUpdateScheduler.kt new file mode 100644 index 00000000..91f67200 --- /dev/null +++ b/formula-android/src/main/java/com/instacart/formula/android/internal/AndroidUpdateScheduler.kt @@ -0,0 +1,71 @@ +package com.instacart.formula.android.internal + +import java.util.concurrent.atomic.AtomicBoolean +import java.util.concurrent.atomic.AtomicReference + +/** + * Handles state update scheduling to the main thread. If update arrives on a background thread, + * it will added it the main thread queue. It will throw away a pending update if a new update + * arrives. + */ +class AndroidUpdateScheduler( + private val update: (Value) -> Unit, +) { + /** + * If not null, that means that we have an update pending. + */ + private val pendingValue = AtomicReference() + + /** + * Defines if an update is currently scheduled. + */ + private val updateScheduled = AtomicBoolean(false) + + /** + * To avoid re-entry, we track if [updateRunnable] is currently handling an update. + */ + private var isUpdating = false + + private val updateRunnable = object : Runnable { + override fun run() { + updateScheduled.set(false) + + var localPending = pendingValue.getAndSet(null) + while (localPending != null) { + // Handle the update + isUpdating = true + update(localPending) + isUpdating = false + + // Check if another update arrived while we were processing. + localPending = pendingValue.getAndSet(null) + + if (localPending != null) { + // We will perform the update, let's clear the values. + updateScheduled.set(false) + Utils.mainThreadHandler.removeCallbacks(this) + } + } + } + } + + fun emitUpdate(value: Value) { + // Set pending value + pendingValue.set(value) + + if (Utils.isMainThread()) { + if (isUpdating) { + // Let's exit and let the [updateRunnable] to pick up the change + return + } else { + // Since we are on main thread, let's force run it + updateRunnable.run() + } + } else { + // If no update is scheduled, schedule one + if (updateScheduled.compareAndSet(false, true)) { + Utils.mainThreadHandler.post(updateRunnable) + } + } + } +} \ No newline at end of file diff --git a/formula-android/src/main/java/com/instacart/formula/android/internal/FeatureBinding.kt b/formula-android/src/main/java/com/instacart/formula/android/internal/FeatureBinding.kt index 06ab7f2a..4c4db867 100644 --- a/formula-android/src/main/java/com/instacart/formula/android/internal/FeatureBinding.kt +++ b/formula-android/src/main/java/com/instacart/formula/android/internal/FeatureBinding.kt @@ -1,6 +1,5 @@ package com.instacart.formula.android.internal -import android.os.SystemClock import com.instacart.formula.Action import com.instacart.formula.Evaluation import com.instacart.formula.Formula diff --git a/formula-android/src/main/java/com/instacart/formula/android/internal/FeatureObservableAction.kt b/formula-android/src/main/java/com/instacart/formula/android/internal/FeatureObservableAction.kt new file mode 100644 index 00000000..30b59e56 --- /dev/null +++ b/formula-android/src/main/java/com/instacart/formula/android/internal/FeatureObservableAction.kt @@ -0,0 +1,29 @@ +package com.instacart.formula.android.internal + +import com.instacart.formula.Action +import com.instacart.formula.Cancelable +import com.instacart.formula.android.Feature +import com.instacart.formula.android.FragmentEnvironment +import com.instacart.formula.android.FragmentId +import io.reactivex.rxjava3.core.Observable + +class FeatureObservableAction( + private val fragmentEnvironment: FragmentEnvironment, + private val fragmentId: FragmentId, + private val feature: Feature<*>, +) : Action { + + override fun key(): Any = fragmentId + + override fun start(send: (Any) -> Unit): Cancelable { + val observable = feature.state.onErrorResumeNext { + fragmentEnvironment.onScreenError(fragmentId.key, it) + Observable.empty() + } + + // We ensure all feature state updates come on the main thread. + val androidUpdateScheduler = AndroidUpdateScheduler(send) + val disposable = observable.subscribe(androidUpdateScheduler::emitUpdate) + return Cancelable(disposable::dispose) + } +} \ No newline at end of file diff --git a/formula-android/src/main/java/com/instacart/formula/android/internal/FragmentFlowRenderView.kt b/formula-android/src/main/java/com/instacart/formula/android/internal/FragmentFlowRenderView.kt index 724cd1a6..2b1d51b6 100644 --- a/formula-android/src/main/java/com/instacart/formula/android/internal/FragmentFlowRenderView.kt +++ b/formula-android/src/main/java/com/instacart/formula/android/internal/FragmentFlowRenderView.kt @@ -118,6 +118,8 @@ internal class FragmentFlowRenderView( } fun render(state: FragmentFlowState) { + Utils.assertMainThread() + fragmentState = state updateVisibleFragments(state) } diff --git a/formula-android/src/main/java/com/instacart/formula/android/internal/StreamConfiguratorIml.kt b/formula-android/src/main/java/com/instacart/formula/android/internal/StreamConfiguratorIml.kt index 9c28e543..6cffa7f8 100644 --- a/formula-android/src/main/java/com/instacart/formula/android/internal/StreamConfiguratorIml.kt +++ b/formula-android/src/main/java/com/instacart/formula/android/internal/StreamConfiguratorIml.kt @@ -18,14 +18,17 @@ internal class StreamConfiguratorIml( val stateEmissions = Observable.combineLatest( state, context.activityStartedEvents(), - BiFunction { state, event -> - state + BiFunction { stateValue, _ -> + stateValue } ) - return stateEmissions.subscribe { state -> + + val updateScheduler = AndroidUpdateScheduler { stateValue -> context.startedActivity()?.let { - update(it, state) + update(it, stateValue) } } + + return stateEmissions.subscribe(updateScheduler::emitUpdate) } } diff --git a/formula-android/src/main/java/com/instacart/formula/android/internal/Utils.kt b/formula-android/src/main/java/com/instacart/formula/android/internal/Utils.kt index c93f1dc0..3a1dc740 100644 --- a/formula-android/src/main/java/com/instacart/formula/android/internal/Utils.kt +++ b/formula-android/src/main/java/com/instacart/formula/android/internal/Utils.kt @@ -1,11 +1,18 @@ package com.instacart.formula.android.internal +import android.os.Handler import android.os.Looper internal object Utils { + internal val mainThreadHandler = Handler(Looper.getMainLooper()) + fun assertMainThread() { - if (Looper.getMainLooper() != Looper.myLooper()) { + if (!isMainThread()) { throw IllegalStateException("should be called on main thread: ${Thread.currentThread()}") } } + + fun isMainThread(): Boolean { + return Looper.getMainLooper() == Looper.myLooper() + } } \ No newline at end of file diff --git a/formula-android/src/test/java/com/instacart/formula/android/FragmentFlowStoreTest.kt b/formula-android/src/test/java/com/instacart/formula/android/FragmentFlowStoreTest.kt index f9075f70..4c5b74aa 100644 --- a/formula-android/src/test/java/com/instacart/formula/android/FragmentFlowStoreTest.kt +++ b/formula-android/src/test/java/com/instacart/formula/android/FragmentFlowStoreTest.kt @@ -1,5 +1,7 @@ package com.instacart.formula.android +import android.os.Looper +import androidx.test.ext.junit.runners.AndroidJUnit4 import com.google.common.truth.Truth.assertThat import com.instacart.formula.android.fakes.DetailKey import com.instacart.formula.android.fakes.FakeAuthFlowFactory @@ -10,9 +12,16 @@ import com.instacart.formula.android.fakes.NoOpViewFactory import com.instacart.formula.android.fakes.TestAccountFragmentKey import com.instacart.formula.android.fakes.TestLoginFragmentKey import com.instacart.formula.android.fakes.TestSignUpFragmentKey +import com.instacart.formula.rxjava3.toObservable import io.reactivex.rxjava3.observers.TestObserver import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.Shadows +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit +@RunWith(AndroidJUnit4::class) class FragmentFlowStoreTest { @Test fun `duplicate contract registration throws an exception`() { @@ -175,6 +184,57 @@ class FragmentFlowStoreTest { ) } + @Test fun `background feature events are moved to the main thread`() { + val executor = Executors.newSingleThreadExecutor() + val component = FakeComponent() + val store = createStore(component) + + val latch = CountDownLatch(1) + + val updates = mutableListOf>() + val updateThreads = linkedSetOf() + val disposable = store.toObservable(FragmentEnvironment()).subscribe { + val states = it.states.mapKeys { it.key.key }.mapValues { it.value.renderModel } + updates.add(states) + + updateThreads.add(Thread.currentThread()) + } + + // Add couple of features + store.onLifecycleEffect(MainKey(1).asAddedEvent()) + store.onLifecycleEffect(DetailKey(2).asAddedEvent()) + + // Pass feature updates on a background thread + executor.execute { + component.updateRelay.accept(MainKey(1) to "main-state-1") + component.updateRelay.accept(MainKey(1) to "main-state-2") + component.updateRelay.accept(MainKey(1) to "main-state-3") + + component.updateRelay.accept(DetailKey(2) to "detail-state-1") + component.updateRelay.accept(DetailKey(2) to "detail-state-2") + component.updateRelay.accept(DetailKey(2) to "detail-state-3") + latch.countDown() + } + + // Wait for background execution to finish + if(!latch.await(100, TimeUnit.MILLISECONDS)) { + throw IllegalStateException("timeout") + } + + Shadows.shadowOf(Looper.getMainLooper()).idle() + + val expected = mapOf( + MainKey(1) to "main-state-3", + DetailKey(2) to "detail-state-3" + ) + + val last = updates.last() + assertThat(last).isEqualTo(expected) + + assertThat(updateThreads).hasSize(1) + assertThat(updateThreads).containsExactly(Thread.currentThread()) + } + private fun FragmentFlowStore.toStates(): TestObserver> { return state(FragmentEnvironment()) .map { it.states.mapKeys { entry -> entry.key.key } }