From 989d7b4ea70ab4a84063f7588be379aa9bf00a3d Mon Sep 17 00:00:00 2001 From: Jesse Wilson Date: Tue, 10 Dec 2024 06:21:01 -1000 Subject: [PATCH] Dispose of the composition when we're done with it. (#2494) * Dispose of the composition when we're done with it. This adds a test for DisposableEffect callbacks. https://github.com/cashapp/redwood/issues/2248 * Fixup LeaksTest * Spotless * Track HostApi change * Track HostApi interface change --------- Co-authored-by: Jesse Wilson --- .../app/cash/redwood/compose/PlatformSet.kt | 2 + .../redwood/compose/RedwoodComposition.kt | 1 + .../app/cash/redwood/compose/WidgetApplier.kt | 3 +- .../app/cash/redwood/compose/PlatformSet.kt | 4 + .../app/cash/redwood/compose/PlatformSet.kt | 3 + .../app/cash/redwood/treehouse/FakeHostApi.kt | 11 +++ .../redwood/treehouse/GuestLifecycleTest.kt | 80 +++++++++++++++++++ .../cash/redwood/treehouse/TreehouseTester.kt | 6 +- .../cash/redwood/treehouse/leaks/JvmHeap.kt | 2 + .../ios-uikit/EmojiSearchApp/IosHostApi.swift | 3 + .../testapp/android/views/RealHostApi.kt | 3 + test-app/ios-uikit/TestApp/IosHostApi.swift | 3 + .../redwood/testapp/treehouse/HostApi.kt | 3 + .../testapp/treehouse/RealTestAppPresenter.kt | 3 +- .../testapp/treehouse/TesterTreehouseUi.kt | 53 ++++++++++-- 15 files changed, 167 insertions(+), 13 deletions(-) create mode 100644 redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/GuestLifecycleTest.kt diff --git a/redwood-compose/src/commonMain/kotlin/app/cash/redwood/compose/PlatformSet.kt b/redwood-compose/src/commonMain/kotlin/app/cash/redwood/compose/PlatformSet.kt index dcd1572901..2e33903189 100644 --- a/redwood-compose/src/commonMain/kotlin/app/cash/redwood/compose/PlatformSet.kt +++ b/redwood-compose/src/commonMain/kotlin/app/cash/redwood/compose/PlatformSet.kt @@ -24,3 +24,5 @@ internal expect inline operator fun PlatformSet.plusAssign(element: T) internal expect inline fun PlatformSet.forEach(crossinline block: (T) -> Unit) internal expect inline fun PlatformSet.clear() + +internal expect val PlatformSet.size: Int diff --git a/redwood-compose/src/commonMain/kotlin/app/cash/redwood/compose/RedwoodComposition.kt b/redwood-compose/src/commonMain/kotlin/app/cash/redwood/compose/RedwoodComposition.kt index c45824125c..6fd2e05740 100644 --- a/redwood-compose/src/commonMain/kotlin/app/cash/redwood/compose/RedwoodComposition.kt +++ b/redwood-compose/src/commonMain/kotlin/app/cash/redwood/compose/RedwoodComposition.kt @@ -173,6 +173,7 @@ private class WidgetRedwoodComposition( } override fun cancel() { + composition.dispose() snapshotHandle.dispose() snapshotJob?.cancel() recomposeJob.cancel() diff --git a/redwood-compose/src/commonMain/kotlin/app/cash/redwood/compose/WidgetApplier.kt b/redwood-compose/src/commonMain/kotlin/app/cash/redwood/compose/WidgetApplier.kt index 20d2ba93c8..ba64dc4c9a 100644 --- a/redwood-compose/src/commonMain/kotlin/app/cash/redwood/compose/WidgetApplier.kt +++ b/redwood-compose/src/commonMain/kotlin/app/cash/redwood/compose/WidgetApplier.kt @@ -74,7 +74,8 @@ internal class NodeApplier( } override fun onEndChanges() { - check(!closed) + // When the composition is disposed, onEndChanges() is called after onClear(). + check(!closed || changedWidgets.size == 0) changedWidgets.let { changedWidgets -> changedWidgets.forEach { changedWidget -> diff --git a/redwood-compose/src/jsMain/kotlin/app/cash/redwood/compose/PlatformSet.kt b/redwood-compose/src/jsMain/kotlin/app/cash/redwood/compose/PlatformSet.kt index f841531495..a744244c22 100644 --- a/redwood-compose/src/jsMain/kotlin/app/cash/redwood/compose/PlatformSet.kt +++ b/redwood-compose/src/jsMain/kotlin/app/cash/redwood/compose/PlatformSet.kt @@ -19,6 +19,7 @@ package app.cash.redwood.compose @JsName("Set") internal external class JsSet { + val size: Int fun add(element: T) fun clear() fun forEach(block: (T) -> Unit) @@ -41,3 +42,6 @@ internal actual inline fun PlatformSet.forEach(crossinline block: (T) -> internal actual inline fun PlatformSet.clear() { clear() } + +internal actual val PlatformSet.size: Int + get() = size diff --git a/redwood-compose/src/nonJsMain/kotlin/app/cash/redwood/compose/PlatformSet.kt b/redwood-compose/src/nonJsMain/kotlin/app/cash/redwood/compose/PlatformSet.kt index f87fc19554..2568f7428b 100644 --- a/redwood-compose/src/nonJsMain/kotlin/app/cash/redwood/compose/PlatformSet.kt +++ b/redwood-compose/src/nonJsMain/kotlin/app/cash/redwood/compose/PlatformSet.kt @@ -37,3 +37,6 @@ internal actual inline fun PlatformSet.forEach(block: (T) -> Unit) { internal actual inline fun PlatformSet.clear() { clear() } + +internal actual val PlatformSet.size: Int + get() = size diff --git a/redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/FakeHostApi.kt b/redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/FakeHostApi.kt index b82b886d0f..91c1d26c8a 100644 --- a/redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/FakeHostApi.kt +++ b/redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/FakeHostApi.kt @@ -16,9 +16,20 @@ package app.cash.redwood.treehouse import com.example.redwood.testapp.treehouse.HostApi +import kotlinx.coroutines.channels.Channel class FakeHostApi : HostApi { + private val messagesChannel = Channel(capacity = Int.MAX_VALUE) + override suspend fun httpCall(url: String, headers: Map): String { error("unexpected call") } + + override fun log(message: String) { + messagesChannel.trySend(message) + } + + suspend fun takeMessage(): String { + return messagesChannel.receive() + } } diff --git a/redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/GuestLifecycleTest.kt b/redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/GuestLifecycleTest.kt new file mode 100644 index 0000000000..08e608f806 --- /dev/null +++ b/redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/GuestLifecycleTest.kt @@ -0,0 +1,80 @@ +/* + * Copyright (C) 2024 Square, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package app.cash.redwood.treehouse + +import assertk.assertThat +import assertk.assertions.isEmpty +import assertk.assertions.isEqualTo +import com.example.redwood.testapp.testing.ButtonValue +import com.example.redwood.testapp.testing.TextInputValue +import kotlin.test.Test +import kotlinx.coroutines.test.runTest + +class GuestLifecycleTest { + @Test + fun disposableEffectDisposedWhenRemovedFromComposition() = runTest { + val tester = TreehouseTester(this) + val treehouseApp = tester.loadApp() + val content = tester.content(treehouseApp) + val view = tester.view() + + content.bind(view) + + content.awaitContent(1) + val textInputValue = view.views.single() as TextInputValue + assertThat(textInputValue.text).isEqualTo("what would you like to see?") + textInputValue.onChange!!.invoke("GuestLifecycleTestShowDisposable") + + tester.sendFrame() + assertThat(tester.hostApi.takeMessage()).isEqualTo("DisposableEffect.effect()") + content.awaitContent(2) + val nextButton = view.views.single() as ButtonValue + assertThat(nextButton.text).isEqualTo("Next") + nextButton.onClick!!.invoke() + + tester.sendFrame() + assertThat(tester.hostApi.takeMessage()).isEqualTo("DisposableEffect.dispose()") + content.awaitContent(3) + assertThat(view.views).isEmpty() + + treehouseApp.stop() + treehouseApp.close() + } + + @Test + fun disposableEffectDisposedWhenContentUnbound() = runTest { + val tester = TreehouseTester(this) + val treehouseApp = tester.loadApp() + val content = tester.content(treehouseApp) + val view = tester.view() + + content.bind(view) + + content.awaitContent(1) + val textInputValue = view.views.single() as TextInputValue + assertThat(textInputValue.text).isEqualTo("what would you like to see?") + textInputValue.onChange!!.invoke("GuestLifecycleTestShowDisposable") + + tester.sendFrame() + assertThat(tester.hostApi.takeMessage()).isEqualTo("DisposableEffect.effect()") + + content.unbind() + assertThat(tester.hostApi.takeMessage()).isEqualTo("DisposableEffect.dispose()") + + treehouseApp.stop() + treehouseApp.close() + } +} diff --git a/redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/TreehouseTester.kt b/redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/TreehouseTester.kt index 4fa2c7e24b..88eaf550d5 100644 --- a/redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/TreehouseTester.kt +++ b/redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/TreehouseTester.kt @@ -44,7 +44,7 @@ internal class TreehouseTester( ) { val eventLog = EventLog() - var hostApi: HostApi = FakeHostApi() + var hostApi = FakeHostApi() var eventListenerFactory: EventListener.Factory = FakeEventListener.Factory(eventLog) @@ -98,7 +98,7 @@ internal class TreehouseTester( override fun create(scope: CoroutineScope, dispatchers: TreehouseDispatchers) = frameClock } - val treehouseAppFactory = RealTreehouseApp.Factory( + private val treehouseAppFactory = RealTreehouseApp.Factory( platform = platform, httpClient = httpClient, frameClockFactory = frameClockFactory, @@ -129,7 +129,7 @@ internal class TreehouseTester( treehouseApp: TreehouseApp, zipline: Zipline, ) { - zipline.bind("HostApi", hostApi) + zipline.bind("HostApi", hostApi) } override fun create(zipline: Zipline): TestAppPresenter { diff --git a/redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/leaks/JvmHeap.kt b/redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/leaks/JvmHeap.kt index e22926dfad..b17c4e698f 100644 --- a/redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/leaks/JvmHeap.kt +++ b/redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/leaks/JvmHeap.kt @@ -18,6 +18,7 @@ package app.cash.redwood.treehouse.leaks import androidx.collection.IntObjectMap import androidx.collection.ScatterSet import app.cash.redwood.treehouse.EventLog +import com.example.redwood.testapp.treehouse.HostApi import java.lang.ref.WeakReference import java.lang.reflect.Field import kotlinx.coroutines.CoroutineDispatcher @@ -83,6 +84,7 @@ internal object JvmHeap : Heap { instance is CoroutineDispatcher -> listOf() instance is Enum<*> -> listOf() instance is EventLog -> listOf() + instance is HostApi -> listOf() instance is Int -> listOf() instance is Job -> listOf() instance is Json -> listOf() diff --git a/samples/emoji-search/ios-uikit/EmojiSearchApp/IosHostApi.swift b/samples/emoji-search/ios-uikit/EmojiSearchApp/IosHostApi.swift index 966386daf0..9981f61308 100644 --- a/samples/emoji-search/ios-uikit/EmojiSearchApp/IosHostApi.swift +++ b/samples/emoji-search/ios-uikit/EmojiSearchApp/IosHostApi.swift @@ -48,6 +48,9 @@ class IosHostApi : HostApi { } } + func log(message: String) { + } + func close() { } } diff --git a/test-app/android-views/src/main/kotlin/com/example/redwood/testapp/android/views/RealHostApi.kt b/test-app/android-views/src/main/kotlin/com/example/redwood/testapp/android/views/RealHostApi.kt index 36e35bd491..3a0eacecab 100644 --- a/test-app/android-views/src/main/kotlin/com/example/redwood/testapp/android/views/RealHostApi.kt +++ b/test-app/android-views/src/main/kotlin/com/example/redwood/testapp/android/views/RealHostApi.kt @@ -59,4 +59,7 @@ class RealHostApi( continuation.invokeOnCancellation { call.cancel() } } } + + override fun log(message: String) { + } } diff --git a/test-app/ios-uikit/TestApp/IosHostApi.swift b/test-app/ios-uikit/TestApp/IosHostApi.swift index de2c2e3c29..7610800470 100644 --- a/test-app/ios-uikit/TestApp/IosHostApi.swift +++ b/test-app/ios-uikit/TestApp/IosHostApi.swift @@ -42,6 +42,9 @@ class IosHostApi : HostApi { } } + func log(message: String) { + } + func close() { } } diff --git a/test-app/presenter-treehouse/src/commonMain/kotlin/com/example/redwood/testapp/treehouse/HostApi.kt b/test-app/presenter-treehouse/src/commonMain/kotlin/com/example/redwood/testapp/treehouse/HostApi.kt index 41744807b6..7ee7456a91 100644 --- a/test-app/presenter-treehouse/src/commonMain/kotlin/com/example/redwood/testapp/treehouse/HostApi.kt +++ b/test-app/presenter-treehouse/src/commonMain/kotlin/com/example/redwood/testapp/treehouse/HostApi.kt @@ -22,4 +22,7 @@ import kotlin.native.ObjCName interface HostApi : ZiplineService { /** Decodes the response as a string and returns it. */ suspend fun httpCall(url: String, headers: Map): String + + /** Records a message for any potential consumer. */ + fun log(message: String) } diff --git a/test-app/presenter-treehouse/src/jsMain/kotlin/com/example/redwood/testapp/treehouse/RealTestAppPresenter.kt b/test-app/presenter-treehouse/src/jsMain/kotlin/com/example/redwood/testapp/treehouse/RealTestAppPresenter.kt index 626493771d..d3f9230c04 100644 --- a/test-app/presenter-treehouse/src/jsMain/kotlin/com/example/redwood/testapp/treehouse/RealTestAppPresenter.kt +++ b/test-app/presenter-treehouse/src/jsMain/kotlin/com/example/redwood/testapp/treehouse/RealTestAppPresenter.kt @@ -37,6 +37,7 @@ class RealTestAppPresenter( } override fun launchForTester(): ZiplineTreehouseUi { - return TesterTreehouseUi().asZiplineTreehouseUi(appLifecycle) + val treehouseUi = TesterTreehouseUi(hostApi) + return treehouseUi.asZiplineTreehouseUi(appLifecycle) } } diff --git a/test-app/presenter-treehouse/src/jsMain/kotlin/com/example/redwood/testapp/treehouse/TesterTreehouseUi.kt b/test-app/presenter-treehouse/src/jsMain/kotlin/com/example/redwood/testapp/treehouse/TesterTreehouseUi.kt index db7603edb5..14e847a5cc 100644 --- a/test-app/presenter-treehouse/src/jsMain/kotlin/com/example/redwood/testapp/treehouse/TesterTreehouseUi.kt +++ b/test-app/presenter-treehouse/src/jsMain/kotlin/com/example/redwood/testapp/treehouse/TesterTreehouseUi.kt @@ -16,6 +16,7 @@ package com.example.redwood.testapp.treehouse import androidx.compose.runtime.Composable +import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember @@ -24,19 +25,25 @@ import app.cash.redwood.treehouse.TreehouseUi import com.example.redwood.testapp.compose.Button import com.example.redwood.testapp.compose.TextInput -class TesterTreehouseUi : TreehouseUi { +class TesterTreehouseUi( + private val hostApi: HostApi, +) : TreehouseUi { @Composable override fun Show() { var content by remember { mutableStateOf(Content.InitialValue) } - content.Show { newContent -> - content = newContent - } + content.Show( + changeContent = { newContent -> content = newContent }, + log = { message -> hostApi.log(message) }, + ) } enum class Content { InitialValue { @Composable - override fun Show(changeContent: (Content) -> Unit) { + override fun Show( + changeContent: (Content) -> Unit, + log: (String) -> Unit, + ) { TextInput( text = "what would you like to see?", customType = null, @@ -47,9 +54,33 @@ class TesterTreehouseUi : TreehouseUi { } }, + GuestLifecycleTestShowDisposable { + @Composable + override fun Show( + changeContent: (Content) -> Unit, + log: (String) -> Unit, + ) { + DisposableEffect(log) { + log("DisposableEffect.effect()") + onDispose { + log("DisposableEffect.dispose()") + } + } + Button( + text = "Next", + onClick = { + changeContent(Empty) + }, + ) + } + }, + TreehouseTesterTestHappyPathStep2 { @Composable - override fun Show(changeContent: (Content) -> Unit) { + override fun Show( + changeContent: (Content) -> Unit, + log: (String) -> Unit, + ) { Button( text = "This is TreehouseTesterTestHappyPathStep2", onClick = { @@ -60,13 +91,19 @@ class TesterTreehouseUi : TreehouseUi { Empty { @Composable - override fun Show(changeContent: (Content) -> Unit) { + override fun Show( + changeContent: (Content) -> Unit, + log: (String) -> Unit, + ) { } }, ; @Composable - abstract fun Show(changeContent: (Content) -> Unit) + abstract fun Show( + changeContent: (Content) -> Unit, + log: (String) -> Unit, + ) } }