Fix clearing sequence from ViewModel closeables
* Set clearing sequence as `keyToCloseable + closeable`.
* Include a unit test to ensure the closing ordering is always respected.
* The clearing sequence was wrongly changed to `closeable + keyToCloseable` by aosp/2997876.
Test: ViewModelTest
Fixes: 333573506
Change-Id: I87ee7a0649cf9702790d7e6a0e31558584c826d0
diff --git a/lifecycle/lifecycle-viewmodel/src/commonMain/kotlin/androidx/lifecycle/ViewModel.kt b/lifecycle/lifecycle-viewmodel/src/commonMain/kotlin/androidx/lifecycle/ViewModel.kt
index cb1bb1a..10913aa 100644
--- a/lifecycle/lifecycle-viewmodel/src/commonMain/kotlin/androidx/lifecycle/ViewModel.kt
+++ b/lifecycle/lifecycle-viewmodel/src/commonMain/kotlin/androidx/lifecycle/ViewModel.kt
@@ -145,10 +145,7 @@
* prevent a memory leak, as the subscriptions might hold a reference to the [ViewModel] even
* after it is no longer needed.
*
- * **Clearing Sequence:**
- * 1. [AutoCloseable.close] resources added **without** a key via [addCloseable].
- * 2. [AutoCloseable.close] resources added **with** a key via [addCloseable].
- * 3. Invoke the [onCleared] callback.
+ * For specifics about the clearing sequence, refer to the [clear] method.
*/
protected open fun onCleared()
@@ -159,9 +156,10 @@
* immediately closed.
*
* **Clearing Sequence:**
- * 1. [AutoCloseable.close] resources added **without** a key via [addCloseable].
- * 2. [AutoCloseable.close] resources added **with** a key via [addCloseable].
- * 3. Invoke the [onCleared] callback.
+ * 1. [Close][AutoCloseable.close] resources added **with** a key via [addCloseable].
+ * 2. [Close][AutoCloseable.close] resources added via `constructor`.
+ * 3. [Close][AutoCloseable.close] resources added **without** a key via [addCloseable].
+ * 4. Invoke the [onCleared] callback.
*/
@MainThread
internal fun clear()
diff --git a/lifecycle/lifecycle-viewmodel/src/commonMain/kotlin/androidx/lifecycle/viewmodel/internal/ViewModelImpl.kt b/lifecycle/lifecycle-viewmodel/src/commonMain/kotlin/androidx/lifecycle/viewmodel/internal/ViewModelImpl.kt
index dd79e0c..00efd4b 100644
--- a/lifecycle/lifecycle-viewmodel/src/commonMain/kotlin/androidx/lifecycle/viewmodel/internal/ViewModelImpl.kt
+++ b/lifecycle/lifecycle-viewmodel/src/commonMain/kotlin/androidx/lifecycle/viewmodel/internal/ViewModelImpl.kt
@@ -42,13 +42,10 @@
* The associated resources will be [AutoCloseable.close] right before the [ViewModel.onCleared]
* is called. This provides automatic resource cleanup upon [ViewModel] release.
*
- * The clearing order is:
- * 1. [keyToCloseables][AutoCloseable.close]
- * 2. [closeables][AutoCloseable.close]
- * 3. [ViewModel.onCleared]
+ * For specifics about the clearing sequence, refer to the [ViewModel.clear] method.
*
- * **Note:** Manually [SynchronizedObject] is necessary to prevent issues on Android API 21 and 22.
- * This avoids potential problems found in older versions of `ConcurrentHashMap`.
+ * **Note:** Manually [SynchronizedObject] is necessary to prevent issues on Android API 21
+ * and 22. This avoids potential problems found in older versions of `ConcurrentHashMap`.
*
* @see <a href="https://issuetracker.google.com/37042460">b/37042460</a>
*/
@@ -84,9 +81,10 @@
isCleared = true
synchronized(lock) {
- // 1. Closes resources added without a key.
- // 2. Closes resources added with a key.
- for (closeable in closeables + keyToCloseables.values) {
+ for (closeable in keyToCloseables.values) {
+ closeWithRuntimeException(closeable)
+ }
+ for (closeable in closeables) {
closeWithRuntimeException(closeable)
}
// Clear only resources without keys to prevent accidental recreation of resources.
diff --git a/lifecycle/lifecycle-viewmodel/src/commonTest/kotlin/androidx/lifecycle/ViewModelTest.kt b/lifecycle/lifecycle-viewmodel/src/commonTest/kotlin/androidx/lifecycle/ViewModelTest.kt
index 570bf8e..fc82ddb 100644
--- a/lifecycle/lifecycle-viewmodel/src/commonTest/kotlin/androidx/lifecycle/ViewModelTest.kt
+++ b/lifecycle/lifecycle-viewmodel/src/commonTest/kotlin/androidx/lifecycle/ViewModelTest.kt
@@ -154,12 +154,50 @@
}
//endregion
+ @Test
+ fun clear_closesResources_inCleaningSequenceOrder() {
+ val clearedInOrderCloseables = mutableListOf<AutoCloseable>()
+ val closeableInConstructor1 = CloseableResource { clearedInOrderCloseables += this }
+ val closeableInConstructor2 = CloseableResource { clearedInOrderCloseables += this }
+ val closeableWithoutKey1 = CloseableResource { clearedInOrderCloseables += this }
+ val closeableWithoutKey2 = CloseableResource { clearedInOrderCloseables += this }
+ val closeableWithKey1 = CloseableResource { clearedInOrderCloseables += this }
+ val closeableWithKey2 = CloseableResource { clearedInOrderCloseables += this }
+
+ val viewModel = TestViewModel(closeableInConstructor1, closeableInConstructor2)
+ viewModel.addCloseable(closeableWithoutKey1)
+ viewModel.addCloseable(key = "customKey1", closeableWithKey1)
+ viewModel.addCloseable(closeableWithoutKey2)
+ viewModel.addCloseable(key = "customKey2", closeableWithKey2)
+ viewModel.clear()
+
+ // The clearing order is:
+ val expectedCloseables = listOf(
+ // 1. Resources added **with** a key via `addCloseable`.
+ closeableWithKey1,
+ closeableWithKey2,
+
+ // 2. Resources added **without** a key via `constructor`.
+ closeableInConstructor1,
+ closeableInConstructor2,
+
+ // 3. Resources added **without** a key via `addCloseable`.
+ closeableWithoutKey1,
+ closeableWithoutKey2,
+ )
+ assertThat(clearedInOrderCloseables).isEqualTo(expectedCloseables)
+ }
+
//region test helpers
private class TestViewModel(vararg closeables: AutoCloseable) : ViewModel(*closeables)
- private class CloseableResource(var isClosed: Boolean = false) : AutoCloseable {
+ private class CloseableResource(
+ var isClosed: Boolean = false,
+ val onClose: CloseableResource.() -> Unit = {},
+ ) : AutoCloseable {
override fun close() {
isClosed = true
+ onClose(this)
}
}
//endregion
