Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise non-fatal error if node called outside main thread #386

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 42 additions & 1 deletion libraries/rib-base/src/main/java/com/badoo/ribs/core/Node.kt
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -48,6 +49,7 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
val buildParams: BuildParams<*>,
private val viewFactory: ViewFactory<V>?,
private val retainedInstanceStore: RetainedInstanceStore,
private val isOnMainThreadFunc: () -> Boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make it overridable for every junit tested Node in our codebase, which does not seem achievable.

In general makes sense (we do not support any multi-threading anyway, comparing to Appyx which is guarded by coroutines context and/or atomic mutable states).

Let's take a default implementation from RIBs singleton and add RIBs.isOnMainThread and try to override it somehow in tests, potentially in NodeTestHelper and InteractorTestHelper? But the problem there that we do not reset them. (Maybe it is ok just to give up and override always without reset).

plugins: List<Plugin> = emptyList()
) : Rib, LifecycleOwner {

Expand All @@ -59,7 +61,13 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
buildParams: BuildParams<*>,
viewFactory: ViewFactory<V>?,
plugins: List<Plugin> = 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<V>
get() = this
Expand Down Expand Up @@ -131,27 +139,32 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
get() = isAttachedToView && !isPendingViewDetach && !isPendingDetach

init {
verifyMainThread { "init" }
this.plugins.filterIsInstance<NodeAware>().forEach { it.init(this) }
}

internal fun onBuildFinished() {
verifyMainThread { "onBuildFinished" }
plugins.filterIsInstance<NodeLifecycleAware>().forEach { it.onBuild() }
parent?.onChildBuilt(this)
}

private fun onChildBuilt(child: Node<*>) {
verifyMainThread { "onChildBuilt" }
plugins.filterIsInstance<SubtreeChangeAware>().forEach { it.onChildBuilt(child) }
}

@CallSuper
open fun onCreate() {
verifyMainThread { "onCreate" }
plugins
.filterIsInstance<NodeLifecycleAware>()
.forEach { it.onCreate(lifecycleManager.ribLifecycle.lifecycle) }
lifecycleManager.onCreate()
}

fun onCreateView(parentView: RibView): V? {
verifyMainThread { "onCreateView" }
if (isRoot) rootHost = parentView
if (view == null && viewFactory != null) {
val viewLifecycle = lifecycleManager.initializeViewLifecycle()
Expand All @@ -174,6 +187,7 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
}

fun onAttachToView() {
verifyMainThread { "onAttachToView" }
onAttachToViewChecks()
isAttachedToView = true
lifecycleManager.onAttachToView()
Expand All @@ -194,6 +208,7 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
}

fun onDetachFromView(): V? {
verifyMainThread { "onDetachFromView" }
if (isAttachedToView) {
val view = view
saveViewState()
Expand All @@ -209,6 +224,7 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
}

open fun onDestroy(isRecreating: Boolean) {
verifyMainThread { "onDestroy" }
if (view != null) {
RIBs.errorHandler.handleNonFatalError(
"View was not detached before node detach!",
Expand Down Expand Up @@ -236,6 +252,7 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
*/
@MainThread
fun attachChildNode(child: Node<*>) {
verifyMainThread { "attachChildNode" }
verifyNotRoot(child)
_children.add(child)
lifecycleManager.onAttachChild(child)
Expand All @@ -246,6 +263,7 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
}

internal fun onAttachFinished() {
verifyMainThread { "onAttachFinished" }
plugins.filterIsInstance<NodeLifecycleAware>().forEach { it.onAttach() }
}

Expand All @@ -263,6 +281,7 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
}

fun attachChildView(child: Node<*>) {
verifyMainThread { "attachChildView" }
attachChildView(child, child, true)
}

Expand All @@ -281,6 +300,7 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
}

fun detachChildView(child: Node<*>) {
verifyMainThread { "detachChildView" }
detachChildView(child, child, true)
}

Expand Down Expand Up @@ -308,6 +328,7 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
*/
@MainThread
fun detachChildNode(child: Node<*>, isRecreating: Boolean) {
verifyMainThread { "detachChildNode" }
plugins.filterIsInstance<SubtreeChangeAware>().forEach { it.onChildDetached(child) }
_children.remove(child)
child.onDestroy(isRecreating)
Expand All @@ -325,6 +346,7 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
* To be called from the hosting environment (Activity, Fragment, etc.)
*/
fun onStart() {
verifyMainThread { "onStart" }
lifecycleManager.onStartExternal()
plugins.filterIsInstance<AndroidLifecycleAware>().forEach { it.onStart() }
}
Expand All @@ -333,6 +355,7 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
* To be called from the hosting environment (Activity, Fragment, etc.)
*/
fun onStop() {
verifyMainThread { "onStop" }
lifecycleManager.onStopExternal()
plugins.filterIsInstance<AndroidLifecycleAware>().forEach { it.onStop() }
}
Expand All @@ -341,6 +364,7 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
* To be called from the hosting environment (Activity, Fragment, etc.)
*/
fun onResume() {
verifyMainThread { "onResume" }
lifecycleManager.onResumeExternal()
plugins.filterIsInstance<AndroidLifecycleAware>().forEach { it.onResume() }
}
Expand All @@ -349,12 +373,14 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
* To be called from the hosting environment (Activity, Fragment, etc.)
*/
fun onPause() {
verifyMainThread { "onPause" }
lifecycleManager.onPauseExternal()
plugins.filterIsInstance<AndroidLifecycleAware>().forEach { it.onPause() }
}

@CallSuper
open fun handleBackPress(): Boolean {
verifyMainThread { "handleBackPress" }
val subtreeHandlers = plugins.filterIsInstance<SubtreeBackPressHandler>()
val handlers = plugins.filterIsInstance<BackPressHandler>()

Expand All @@ -365,6 +391,7 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
}

fun upNavigation() {
verifyMainThread { "upNavigation" }
when {
isRoot -> integrationPoint.handleUpNavigation()

Expand All @@ -389,18 +416,21 @@ open class Node<V : RibView> @VisibleForTesting internal constructor(
.any { it.handleBackPress() }

open fun onSaveInstanceState(outState: Bundle) {
verifyMainThread { "onSaveInstanceState" }
outState.putSerializable(Identifier.KEY_UUID, identifier.uuid)
plugins.filterIsInstance<SavesInstanceState>().forEach { it.onSaveInstanceState(outState) }
saveViewState()
outState.putBundle(KEY_VIEW_STATE, savedViewState)
}

fun saveViewState() {
verifyMainThread { "saveViewState" }
val view = view ?: return
savedViewState = Bundle().also { view.saveInstanceState(it) }
}

fun onLowMemory() {
verifyMainThread { "onLowMemory" }
plugins.filterIsInstance<SystemAware>().forEach { it.onLowMemory() }
}

Expand Down Expand Up @@ -444,4 +474,15 @@ open class Node<V : RibView> @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}"
)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -25,6 +26,8 @@ class ChildAwareImplTest {
Node<RibView>(
buildParams = BuildParams(payload = null, buildContext = buildContext),
viewFactory = null,
retainedInstanceStore = RetainedInstanceStore,
isOnMainThreadFunc = { true },
plugins = listOf(registry),
)
}.apply {
Expand Down Expand Up @@ -269,6 +272,8 @@ class ChildAwareImplTest {
) : Node<RibView>(
buildParams = BuildParams(payload = null, buildContext = buildContext),
viewFactory = null,
retainedInstanceStore = RetainedInstanceStore,
isOnMainThreadFunc = { true },
), Child1 {
override fun toString(): String = "${this::class.simpleName}@${hashCode()}"
}
Expand All @@ -279,6 +284,8 @@ class ChildAwareImplTest {
) : Node<RibView>(
buildParams = BuildParams(payload = null, buildContext = buildContext),
viewFactory = null,
retainedInstanceStore = RetainedInstanceStore,
isOnMainThreadFunc = { true },
), Child2 {
override fun toString(): String = "${this::class.simpleName}@${hashCode()}"
}
Expand All @@ -289,6 +296,8 @@ class ChildAwareImplTest {
) : Node<RibView>(
buildParams = BuildParams(payload = null, buildContext = buildContext),
viewFactory = null,
retainedInstanceStore = RetainedInstanceStore,
isOnMainThreadFunc = { true },
), Child3 {
override fun toString(): String = "${this::class.simpleName}@${hashCode()}"
}
Expand Down
34 changes: 31 additions & 3 deletions libraries/rib-base/src/test/java/com/badoo/ribs/core/NodeTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,13 @@ class NodeTest {
private fun createNode(
buildParams: BuildParams<Nothing?> = testBuildParams(),
viewFactory: TestViewFactory? = [email protected],
isOnMainThread: Boolean = true,
plugins: List<Plugin> = emptyList()
): Node<TestView> = Node(
buildParams = buildParams,
viewFactory = viewFactory,
plugins = plugins,
isOnMainThreadFunc = { isOnMainThread },
retainedInstanceStore = retainedInstanceStore
)

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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.ErrorHandler>()
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
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -15,6 +16,8 @@ open class TestNode(
) : Node<TestView>(
buildParams = buildParams,
viewFactory = viewFactory,
retainedInstanceStore = RetainedInstanceStore,
isOnMainThreadFunc = { true },
plugins = plugins + listOf(router)
), TestRib {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -20,6 +21,8 @@ class ConnectableTestNode(
) : Node<Nothing>(
buildParams = buildParams ?: getDefaultBuildParams(parent),
viewFactory = null,
retainedInstanceStore = RetainedInstanceStore,
isOnMainThreadFunc = { true },
plugins = plugins
), ConnectableTestRib, Connectable<Input, Output> by connector {

Expand Down