From 87f6728e155c38aee81d9cc64f71a4423e4b8f43 Mon Sep 17 00:00:00 2001 From: Lachlan McKee Date: Wed, 15 Mar 2023 16:28:47 +0000 Subject: [PATCH] Raise fatal error if node called outside main thread --- .../src/main/java/com/badoo/ribs/core/Node.kt | 43 ++++++++++++++++++- .../childaware/ChildAwareImplTest.kt | 9 ++++ .../test/java/com/badoo/ribs/core/NodeTest.kt | 34 +++++++++++++-- .../com/badoo/ribs/core/helper/TestNode.kt | 3 ++ .../helper/connectable/ConnectableTestNode.kt | 3 ++ 5 files changed, 88 insertions(+), 4 deletions(-) diff --git a/libraries/rib-base/src/main/java/com/badoo/ribs/core/Node.kt b/libraries/rib-base/src/main/java/com/badoo/ribs/core/Node.kt index c4c7b1609..2e34d6daf 100644 --- a/libraries/rib-base/src/main/java/com/badoo/ribs/core/Node.kt +++ b/libraries/rib-base/src/main/java/com/badoo/ribs/core/Node.kt @@ -1,6 +1,7 @@ package com.badoo.ribs.core import android.os.Bundle +import android.os.Looper import androidx.annotation.CallSuper import androidx.annotation.MainThread import androidx.annotation.VisibleForTesting @@ -48,6 +49,7 @@ open class Node @VisibleForTesting internal constructor( val buildParams: BuildParams<*>, private val viewFactory: ViewFactory?, private val retainedInstanceStore: RetainedInstanceStore, + private val isOnMainThreadFunc: () -> Boolean, plugins: List = emptyList() ) : Rib, LifecycleOwner { @@ -59,7 +61,13 @@ open class Node @VisibleForTesting internal constructor( buildParams: BuildParams<*>, viewFactory: ViewFactory?, plugins: List = emptyList() - ) : this(buildParams, viewFactory, RetainedInstanceStore, plugins) + ) : this( + buildParams = buildParams, + viewFactory = viewFactory, + retainedInstanceStore = RetainedInstanceStore, + isOnMainThreadFunc = { Looper.getMainLooper() == Looper.myLooper() }, + plugins = plugins + ) final override val node: Node get() = this @@ -131,20 +139,24 @@ open class Node @VisibleForTesting internal constructor( get() = isAttachedToView && !isPendingViewDetach && !isPendingDetach init { + verifyMainThread { "init" } this.plugins.filterIsInstance().forEach { it.init(this) } } internal fun onBuildFinished() { + verifyMainThread { "onBuildFinished" } plugins.filterIsInstance().forEach { it.onBuild() } parent?.onChildBuilt(this) } private fun onChildBuilt(child: Node<*>) { + verifyMainThread { "onChildBuilt" } plugins.filterIsInstance().forEach { it.onChildBuilt(child) } } @CallSuper open fun onCreate() { + verifyMainThread { "onCreate" } plugins .filterIsInstance() .forEach { it.onCreate(lifecycleManager.ribLifecycle.lifecycle) } @@ -152,6 +164,7 @@ open class Node @VisibleForTesting internal constructor( } fun onCreateView(parentView: RibView): V? { + verifyMainThread { "onCreateView" } if (isRoot) rootHost = parentView if (view == null && viewFactory != null) { val viewLifecycle = lifecycleManager.initializeViewLifecycle() @@ -174,6 +187,7 @@ open class Node @VisibleForTesting internal constructor( } fun onAttachToView() { + verifyMainThread { "onAttachToView" } onAttachToViewChecks() isAttachedToView = true lifecycleManager.onAttachToView() @@ -194,6 +208,7 @@ open class Node @VisibleForTesting internal constructor( } fun onDetachFromView(): V? { + verifyMainThread { "onDetachFromView" } if (isAttachedToView) { val view = view saveViewState() @@ -209,6 +224,7 @@ open class Node @VisibleForTesting internal constructor( } open fun onDestroy(isRecreating: Boolean) { + verifyMainThread { "onDestroy" } if (view != null) { RIBs.errorHandler.handleNonFatalError( "View was not detached before node detach!", @@ -236,6 +252,7 @@ open class Node @VisibleForTesting internal constructor( */ @MainThread fun attachChildNode(child: Node<*>) { + verifyMainThread { "attachChildNode" } verifyNotRoot(child) _children.add(child) lifecycleManager.onAttachChild(child) @@ -246,6 +263,7 @@ open class Node @VisibleForTesting internal constructor( } internal fun onAttachFinished() { + verifyMainThread { "onAttachFinished" } plugins.filterIsInstance().forEach { it.onAttach() } } @@ -263,6 +281,7 @@ open class Node @VisibleForTesting internal constructor( } fun attachChildView(child: Node<*>) { + verifyMainThread { "attachChildView" } attachChildView(child, child, true) } @@ -281,6 +300,7 @@ open class Node @VisibleForTesting internal constructor( } fun detachChildView(child: Node<*>) { + verifyMainThread { "detachChildView" } detachChildView(child, child, true) } @@ -308,6 +328,7 @@ open class Node @VisibleForTesting internal constructor( */ @MainThread fun detachChildNode(child: Node<*>, isRecreating: Boolean) { + verifyMainThread { "detachChildNode" } plugins.filterIsInstance().forEach { it.onChildDetached(child) } _children.remove(child) child.onDestroy(isRecreating) @@ -325,6 +346,7 @@ open class Node @VisibleForTesting internal constructor( * To be called from the hosting environment (Activity, Fragment, etc.) */ fun onStart() { + verifyMainThread { "onStart" } lifecycleManager.onStartExternal() plugins.filterIsInstance().forEach { it.onStart() } } @@ -333,6 +355,7 @@ open class Node @VisibleForTesting internal constructor( * To be called from the hosting environment (Activity, Fragment, etc.) */ fun onStop() { + verifyMainThread { "onStop" } lifecycleManager.onStopExternal() plugins.filterIsInstance().forEach { it.onStop() } } @@ -341,6 +364,7 @@ open class Node @VisibleForTesting internal constructor( * To be called from the hosting environment (Activity, Fragment, etc.) */ fun onResume() { + verifyMainThread { "onResume" } lifecycleManager.onResumeExternal() plugins.filterIsInstance().forEach { it.onResume() } } @@ -349,12 +373,14 @@ open class Node @VisibleForTesting internal constructor( * To be called from the hosting environment (Activity, Fragment, etc.) */ fun onPause() { + verifyMainThread { "onPause" } lifecycleManager.onPauseExternal() plugins.filterIsInstance().forEach { it.onPause() } } @CallSuper open fun handleBackPress(): Boolean { + verifyMainThread { "handleBackPress" } val subtreeHandlers = plugins.filterIsInstance() val handlers = plugins.filterIsInstance() @@ -365,6 +391,7 @@ open class Node @VisibleForTesting internal constructor( } fun upNavigation() { + verifyMainThread { "upNavigation" } when { isRoot -> integrationPoint.handleUpNavigation() @@ -389,6 +416,7 @@ open class Node @VisibleForTesting internal constructor( .any { it.handleBackPress() } open fun onSaveInstanceState(outState: Bundle) { + verifyMainThread { "onSaveInstanceState" } outState.putSerializable(Identifier.KEY_UUID, identifier.uuid) plugins.filterIsInstance().forEach { it.onSaveInstanceState(outState) } saveViewState() @@ -396,11 +424,13 @@ open class Node @VisibleForTesting internal constructor( } fun saveViewState() { + verifyMainThread { "saveViewState" } val view = view ?: return savedViewState = Bundle().also { view.saveInstanceState(it) } } fun onLowMemory() { + verifyMainThread { "onLowMemory" } plugins.filterIsInstance().forEach { it.onLowMemory() } } @@ -444,4 +474,15 @@ open class Node @VisibleForTesting internal constructor( return found } + + private inline fun verifyMainThread(functionName: () -> String) { + if (!isOnMainThreadFunc()) { + RIBs.errorHandler.handleNonFatalError( + "Node interacted with outside main thread. " + + "Node: ${javaClass.name}, " + + "method: ${functionName()}, " + + "thread: ${Thread.currentThread().name}" + ) + } + } } diff --git a/libraries/rib-base/src/test/java/com/badoo/ribs/clienthelper/childaware/ChildAwareImplTest.kt b/libraries/rib-base/src/test/java/com/badoo/ribs/clienthelper/childaware/ChildAwareImplTest.kt index 5e86aca51..760f380c9 100644 --- a/libraries/rib-base/src/test/java/com/badoo/ribs/clienthelper/childaware/ChildAwareImplTest.kt +++ b/libraries/rib-base/src/test/java/com/badoo/ribs/clienthelper/childaware/ChildAwareImplTest.kt @@ -12,6 +12,7 @@ import com.badoo.ribs.core.modality.BuildContext import com.badoo.ribs.core.modality.BuildParams import com.badoo.ribs.core.view.RibView import com.badoo.ribs.routing.Routing +import com.badoo.ribs.store.RetainedInstanceStore import kotlinx.parcelize.Parcelize import org.junit.Assert.assertEquals import org.junit.Assert.assertNull @@ -25,6 +26,8 @@ class ChildAwareImplTest { Node( buildParams = BuildParams(payload = null, buildContext = buildContext), viewFactory = null, + retainedInstanceStore = RetainedInstanceStore, + isOnMainThreadFunc = { true }, plugins = listOf(registry), ) }.apply { @@ -269,6 +272,8 @@ class ChildAwareImplTest { ) : Node( buildParams = BuildParams(payload = null, buildContext = buildContext), viewFactory = null, + retainedInstanceStore = RetainedInstanceStore, + isOnMainThreadFunc = { true }, ), Child1 { override fun toString(): String = "${this::class.simpleName}@${hashCode()}" } @@ -279,6 +284,8 @@ class ChildAwareImplTest { ) : Node( buildParams = BuildParams(payload = null, buildContext = buildContext), viewFactory = null, + retainedInstanceStore = RetainedInstanceStore, + isOnMainThreadFunc = { true }, ), Child2 { override fun toString(): String = "${this::class.simpleName}@${hashCode()}" } @@ -289,6 +296,8 @@ class ChildAwareImplTest { ) : Node( buildParams = BuildParams(payload = null, buildContext = buildContext), viewFactory = null, + retainedInstanceStore = RetainedInstanceStore, + isOnMainThreadFunc = { true }, ), Child3 { override fun toString(): String = "${this::class.simpleName}@${hashCode()}" } diff --git a/libraries/rib-base/src/test/java/com/badoo/ribs/core/NodeTest.kt b/libraries/rib-base/src/test/java/com/badoo/ribs/core/NodeTest.kt index 00c2f807b..608421e14 100644 --- a/libraries/rib-base/src/test/java/com/badoo/ribs/core/NodeTest.kt +++ b/libraries/rib-base/src/test/java/com/badoo/ribs/core/NodeTest.kt @@ -89,11 +89,13 @@ class NodeTest { private fun createNode( buildParams: BuildParams = testBuildParams(), viewFactory: TestViewFactory? = this@NodeTest.viewFactory, + isOnMainThread: Boolean = true, plugins: List = emptyList() ): Node = Node( buildParams = buildParams, viewFactory = viewFactory, plugins = plugins, + isOnMainThreadFunc = { isOnMainThread }, retainedInstanceStore = retainedInstanceStore ) @@ -334,7 +336,10 @@ class NodeTest { directChild.attachChildNode(grandChild) node.attachChildNode(directChild) - assertEquals(Lifecycle.State.CREATED, grandChild.lifecycleManager.externalLifecycle.currentState) + assertEquals( + Lifecycle.State.CREATED, + grandChild.lifecycleManager.externalLifecycle.currentState + ) } @Test @@ -365,7 +370,10 @@ class NodeTest { directChild.attachChildNode(grandChild) node.attachChildNode(directChild) - assertEquals(Lifecycle.State.STARTED, grandChild.lifecycleManager.externalLifecycle.currentState) + assertEquals( + Lifecycle.State.STARTED, + grandChild.lifecycleManager.externalLifecycle.currentState + ) } @Test @@ -399,7 +407,10 @@ class NodeTest { directChild.attachChildNode(grandChild) node.attachChildNode(directChild) - assertEquals(Lifecycle.State.RESUMED, grandChild.lifecycleManager.externalLifecycle.currentState) + assertEquals( + Lifecycle.State.RESUMED, + grandChild.lifecycleManager.externalLifecycle.currentState + ) } @Ignore("This should be reworked to match new mechanism") @@ -604,4 +615,21 @@ class NodeTest { verify(retainedInstanceStore, never()).removeAll(node.identifier) } + + @Test + fun `WHEN node created outside main thread THEN error is raised`() { + val errorHandler = mock() + RIBs.clearErrorHandler() + RIBs.errorHandler = errorHandler + + node = createNode(viewFactory = viewFactory, isOnMainThread = false) + + verify(errorHandler).handleNonFatalError( + errorMessage = "Node interacted with outside main thread. " + + "Node: com.badoo.ribs.core.Node, " + + "method: init, " + + "thread: SDK 32 Main Thread", + throwable = null + ) + } } diff --git a/libraries/rib-base/src/test/java/com/badoo/ribs/core/helper/TestNode.kt b/libraries/rib-base/src/test/java/com/badoo/ribs/core/helper/TestNode.kt index 1c380dc4c..3ae0804a2 100644 --- a/libraries/rib-base/src/test/java/com/badoo/ribs/core/helper/TestNode.kt +++ b/libraries/rib-base/src/test/java/com/badoo/ribs/core/helper/TestNode.kt @@ -5,6 +5,7 @@ import com.badoo.ribs.core.modality.BuildParams import com.badoo.ribs.core.plugin.Plugin import com.badoo.ribs.core.view.ViewFactory import com.badoo.ribs.routing.router.Router +import com.badoo.ribs.store.RetainedInstanceStore import org.mockito.kotlin.mock open class TestNode( @@ -15,6 +16,8 @@ open class TestNode( ) : Node( buildParams = buildParams, viewFactory = viewFactory, + retainedInstanceStore = RetainedInstanceStore, + isOnMainThreadFunc = { true }, plugins = plugins + listOf(router) ), TestRib { diff --git a/libraries/rib-base/src/test/java/com/badoo/ribs/core/helper/connectable/ConnectableTestNode.kt b/libraries/rib-base/src/test/java/com/badoo/ribs/core/helper/connectable/ConnectableTestNode.kt index fbc089267..e8a34d260 100644 --- a/libraries/rib-base/src/test/java/com/badoo/ribs/core/helper/connectable/ConnectableTestNode.kt +++ b/libraries/rib-base/src/test/java/com/badoo/ribs/core/helper/connectable/ConnectableTestNode.kt @@ -11,6 +11,7 @@ import com.badoo.ribs.core.modality.AncestryInfo import com.badoo.ribs.core.modality.BuildParams import com.badoo.ribs.core.plugin.Plugin import com.badoo.ribs.routing.Routing +import com.badoo.ribs.store.RetainedInstanceStore class ConnectableTestNode( buildParams: BuildParams<*>? = null, @@ -20,6 +21,8 @@ class ConnectableTestNode( ) : Node( buildParams = buildParams ?: getDefaultBuildParams(parent), viewFactory = null, + retainedInstanceStore = RetainedInstanceStore, + isOnMainThreadFunc = { true }, plugins = plugins ), ConnectableTestRib, Connectable by connector {