[Android] Ensure all events touching Android happen on the main thread. #334

Laimiux · 2024-01-25T17:03:20Z

What

Given we are starting to support non-main threads within formula core (#332), we need to make sure that our Android module handles this change more robustly. To ensure this, I've been auditing various entries and making them thread-safe.

fun send(effect: Activity.() -> Unit) will now ensure that an activity side-effect is executed on the main thread
Fragment state changes now usesAndroidUpdateScheduler to ensure that they happen on the main thread
Activity update function now also uses AndroidUpdateScheduler to ensure updates happen on the main thread

The new AndroidUpdateScheduler class helps coordinate state updates to the main thread. If there are multiple updates before the main thread can handle them, it will only process the last update throwing all the previous values away. This mechanism should help reduce unnecessary intermediate updates.

carrotkite · 2024-01-25T17:07:28Z

JaCoCo Code Coverage 79.8% ✅

Class	Covered	Meta	Status
`com/instacart/formula/android/FragmentFlowStore`	100%	0%	✅
`com/instacart/formula/android/internal/ActivityManager`	64%	0%	✅
`com/instacart/formula/android/internal/StreamConfiguratorIml`	100%	0%	✅
`com/instacart/formula/android/internal/FeatureObservableAction`	100%	0%	✅
`com/instacart/formula/android/internal/AndroidUpdateScheduler`	100%	0%	✅
`com/instacart/formula/android/internal/FeatureBinding`	100%	0%	✅
`com/instacart/formula/android/internal/FragmentFlowRenderView`	68%	0%	✅
`com/instacart/formula/android/internal/Utils`	50%	0%	✅
`com/instacart/formula/android/internal/ActivityStoreContextImpl`	66%	0%	✅

Generated by 🚫 Danger

Laimiux · 2024-01-25T19:55:25Z

formula-android/src/main/java/com/instacart/formula/android/internal/CompositeBinding.kt

-            if (it.binds(key)) return true
-        }
-        return false
+        return types.contains(key.javaClass)


We should have all types in a set, so no need to iterate over bindings manually.

Ugh, this actually is a problem if the fragment key is a sealed class

formula-android/src/main/java/com/instacart/formula/android/internal/FeatureBinding.kt

Jawnnypoo · 2024-01-25T20:01:48Z

...ula-android/src/main/java/com/instacart/formula/android/internal/ActivityStoreContextImpl.kt

+        } else {
+            Utils.mainThreadHandler.post {
+                startedActivity()?.effect()
+            }


Is there a chance that all effects happen before the activity has started and therefore all effects are lost and never emitted?

Should they be stored somewhere until the activity is started and then one it is, propagated? I am not sure if this ever even would happen.

We would miss any effect that happens before the activity is started. This has been the behavior since this API was introduced, I haven't seen any problems with this as most issues would happen if somebody emits an effect during activity initialization.

formula-android/src/main/java/com/instacart/formula/android/internal/AndroidUpdateScheduler.kt

jasonostrander · 2024-01-25T20:37:52Z

formula-android/src/main/java/com/instacart/formula/android/internal/AndroidUpdateScheduler.kt

+
+                if (localPending != null) {
+                    // We will take over processing, so let's clear the message
+                    Utils.mainThreadHandler.removeCallbacks(this)


I think we also want to set updateScheduled to false here as well? If emitUpdate was called during the above update, the pendingValue would be set and updateScheduled would have been set to true.

Good catch, moved it after localPending != null)

Laimiux · 2024-01-26T16:15:18Z

formula-android/src/main/java/com/instacart/formula/android/FragmentFlowStore.kt

-                            feature.state.onErrorResumeNext {
-                                input.onScreenError(fragmentId.key, it)
-                                Observable.empty()
+                        val action = FeatureObservableAction(


Created a custom action to move updates from the background to the main thread when needed.

Laimiux · 2024-01-26T16:15:46Z

formula-android/src/main/java/com/instacart/formula/android/internal/ActivityManager.kt

@@ -19,24 +19,18 @@ internal class ActivityManager<Activity : FragmentActivity>(
    private val store: ActivityStore<Activity>
 ) {

-    private val fragmentState = store


Simplification

Laimiux · 2024-01-26T16:16:36Z

formula-android/src/main/java/com/instacart/formula/android/internal/ActivityManager.kt


+        uiSubscription = delegate.fragmentFlowState().subscribe {


Since delegate uses BehaviorRelay for fragmentFlowState, we don't need to have the replay(1) with connect mechanism anymore.

Laimiux · 2024-01-26T16:17:09Z

formula-android/src/main/java/com/instacart/formula/android/internal/FeatureObservableAction.kt

+        }
+
+        // We ensure all feature state updates come on the main thread.
+        val androidUpdateScheduler = AndroidUpdateScheduler(send)


Custom action to enable moving update from bg thread to the main one.

Laimiux · 2024-01-26T16:41:30Z

formula-android/src/test/java/com/instacart/formula/android/FragmentFlowStoreTest.kt

@@ -175,6 +178,47 @@ class FragmentFlowStoreTest {
            )
    }

+    @Test fun `background feature events are moved to the main thread`() {


Testing fragment flow store to handle background events

alexanderbezverhni · 2024-01-26T17:39:06Z

formula-android-tests/src/test/java/com/instacart/formula/FragmentFlowRenderViewTest.kt

@@ -199,6 +210,51 @@ class FragmentFlowRenderViewTest {
        assertThat(activeContracts()).containsExactly(TestKey(), TestKeyWithId(1)).inOrder()
    }

+    @Test fun `background feature events are moved to the main thread`() {


alexanderbezverhni · 2024-01-26T17:42:10Z

...oid-tests/src/test/java/com/instacart/formula/android/internal/AndroidUpdateSchedulerTest.kt

+        assertThat(computedValues).containsExactly("first", "next").inOrder()
+    }
+
+    @Test fun `when update arrives on bg thread, handle it on main thread`() {


Not quite clear how this test checks if update was handled on the main thread?

Addressed in #336

alexanderbezverhni · 2024-01-26T17:57:03Z

formula-android/src/main/java/com/instacart/formula/android/internal/AndroidUpdateScheduler.kt

+
+/**
+ * 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


it will added it the main thread queue copy is off

Addressed in #336

alexanderbezverhni · 2024-01-26T20:48:13Z

formula-android/src/main/java/com/instacart/formula/android/internal/Utils.kt

 import android.os.Looper

 internal object Utils {
+    internal val mainThreadHandler = Handler(Looper.getMainLooper())


Can this be lazily initialized?

I looked into both functions, they don't do much so lazy init should not be necessary.

Laimiux force-pushed the laimonas/formula-android-main-thread branch from 8970422 to 8f43e10 Compare January 25, 2024 17:05

Laimiux added 2 commits January 25, 2024 14:46

[Android] Ensure main thread internally.

4ace105

Test AndroidUpdateScheduler.

4b60bc6

Laimiux force-pushed the laimonas/formula-android-main-thread branch from bb20476 to 675363d Compare January 25, 2024 19:46

Laimiux requested review from jasonostrander, alexanderbezverhni and Jawnnypoo January 25, 2024 19:54

Laimiux commented Jan 25, 2024

View reviewed changes

formula-android/src/main/java/com/instacart/formula/android/internal/FeatureBinding.kt Outdated Show resolved Hide resolved

Jawnnypoo reviewed Jan 25, 2024

View reviewed changes

formula-android/src/main/java/com/instacart/formula/android/internal/AndroidUpdateScheduler.kt Outdated Show resolved Hide resolved

Jawnnypoo approved these changes Jan 25, 2024

View reviewed changes

Add a bunch of tests.

d4a4400

Laimiux force-pushed the laimonas/formula-android-main-thread branch from 675363d to d4a4400 Compare January 25, 2024 20:15

jasonostrander approved these changes Jan 25, 2024

View reviewed changes

Laimiux added 2 commits January 25, 2024 16:27

Fix issue with updateScheduled being removed only once.

562a964

Make sure to clear pending update.

25f6b12

Laimiux force-pushed the laimonas/formula-android-main-thread branch from 6c7c17d to 25f6b12 Compare January 25, 2024 22:25

Laimiux commented Jan 26, 2024

View reviewed changes

Laimiux force-pushed the laimonas/formula-android-main-thread branch from 2cc53c3 to 25c4017 Compare January 26, 2024 16:30

Laimiux commented Jan 26, 2024

View reviewed changes

Move main thread scheduler to FragmentFlowStore.

bb0eda5

Laimiux force-pushed the laimonas/formula-android-main-thread branch from 25c4017 to bb0eda5 Compare January 26, 2024 17:13

Laimiux merged commit 9676c10 into master Jan 26, 2024
3 checks passed

Laimiux deleted the laimonas/formula-android-main-thread branch January 26, 2024 17:53

alexanderbezverhni reviewed Jan 26, 2024

View reviewed changes

Laimiux added a commit that referenced this pull request Jan 30, 2024

Address feedback from #334

2a2acfc

Laimiux added a commit that referenced this pull request Jan 30, 2024

Address feedback from #334 (#336)

dc7d523

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android] Ensure all events touching Android happen on the main thread. #334

[Android] Ensure all events touching Android happen on the main thread. #334

Laimiux commented Jan 25, 2024

carrotkite commented Jan 25, 2024 •

edited

Loading

Laimiux Jan 25, 2024

Laimiux Jan 25, 2024

Jawnnypoo Jan 25, 2024

Jawnnypoo Jan 25, 2024

Laimiux Jan 25, 2024

jasonostrander Jan 25, 2024

Laimiux Jan 25, 2024

Laimiux Jan 26, 2024

Laimiux Jan 26, 2024

Laimiux Jan 26, 2024

Laimiux Jan 26, 2024

Laimiux Jan 26, 2024

alexanderbezverhni Jan 26, 2024

alexanderbezverhni Jan 26, 2024

Laimiux Jan 30, 2024

alexanderbezverhni Jan 26, 2024

Laimiux Jan 30, 2024

alexanderbezverhni Jan 26, 2024

Laimiux Jan 30, 2024

[Android] Ensure all events touching Android happen on the main thread. #334

[Android] Ensure all events touching Android happen on the main thread. #334

Conversation

Laimiux commented Jan 25, 2024

What

carrotkite commented Jan 25, 2024 • edited Loading

JaCoCo Code Coverage 79.8% ✅

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carrotkite commented Jan 25, 2024 •

edited

Loading