Fixed
Status Update
No update yet.
Comments
au...@google.com
Yigit, do you have time to fix it?
reemission of the same liveData is racy
reemission of the same liveData is racy
co...@gmail.com
Thanks for the detailed analysis. This may not be an issue anymore since we've started using Main.immediate there but I' not sure; I'll try to create a test case.
co...@gmail.com
just emitting same live data reproduces the issue.
@Test
fun raceTest() {
val subLiveData = MutableLiveData(1)
val subject = liveData(testScope.coroutineContext) {
emitSource(subLiveData)
emitSource(subLiveData) //crashes
}
subject.addObserver().apply {
testScope.advanceUntilIdle()
}
}
@Test
fun raceTest() {
val subLiveData = MutableLiveData(1)
val subject = liveData(testScope.coroutineContext) {
emitSource(subLiveData)
emitSource(subLiveData) //crashes
}
subject.addObserver().apply {
testScope.advanceUntilIdle()
}
}
al...@google.com
With 2.2.0-alpha04 (that use Main.immediate), the issue seems to be still there (I tested it by calling emitSource() twice, like your test case)
al...@google.com
yea sorry immediate does not fix it.
I actually have a WIP fix for it:
https://android-review.googlesource.com/c/platform/frameworks/support/+/1112186
if your case is the one i found (emitting same LiveData multiple times, as shown in #5) you can work around it by adding a dummy transformation.
val subLiveData = MutableLiveData(1)
val subject = liveData(testScope.coroutineContext) {
emitSource(subLiveData.map {it })
emitSource(subLiveData.map {it} )
}
I actually have a WIP fix for it:
if your case is the one i found (emitting same LiveData multiple times, as shown in #5) you can work around it by adding a dummy transformation.
val subLiveData = MutableLiveData(1)
val subject = liveData(testScope.coroutineContext) {
emitSource(subLiveData.map {it })
emitSource(subLiveData.map {it} )
}
al...@google.com
Project: platform/frameworks/support
Branch: androidx-master-dev
commit af12e75e6b4110f48e44ca121466943909de8f06
Author: Yigit Boyar <yboyar@google.com>
Date: Tue Sep 03 12:58:11 2019
Fix coroutine livedata race condition
This CL fixes a bug in liveData builder where emitting same
LiveData source twice would make it crash because the second
emission registry could possibly happen before first one is
removed as source.
We fix it by using a suspending dispose function. It does feel
a bit hacky but we cannot make DisposableHandle.dispose async
and we do not want to block there. This does not mean that there
is a problem if developer disposes it manually since our emit
functions take care of making sure it disposes (and there is
no other way to add source to the underlying MediatorLiveData)
Bug: 140249349
Test: BuildLiveDataTest#raceTest_*
Change-Id: I0b464c242a583da4669af195cf2504e2adc4de40
M lifecycle/lifecycle-livedata-ktx/api/2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/current.txt
M lifecycle/lifecycle-livedata-ktx/api/public_plus_experimental_2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/public_plus_experimental_current.txt
M lifecycle/lifecycle-livedata-ktx/api/restricted_2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/restricted_current.txt
M lifecycle/lifecycle-livedata-ktx/src/main/java/androidx/lifecycle/CoroutineLiveData.kt
M lifecycle/lifecycle-livedata-ktx/src/test/java/androidx/lifecycle/BuildLiveDataTest.kt
https://android-review.googlesource.com/1112186
https://goto.google.com/android-sha1/af12e75e6b4110f48e44ca121466943909de8f06
Branch: androidx-master-dev
commit af12e75e6b4110f48e44ca121466943909de8f06
Author: Yigit Boyar <yboyar@google.com>
Date: Tue Sep 03 12:58:11 2019
Fix coroutine livedata race condition
This CL fixes a bug in liveData builder where emitting same
LiveData source twice would make it crash because the second
emission registry could possibly happen before first one is
removed as source.
We fix it by using a suspending dispose function. It does feel
a bit hacky but we cannot make DisposableHandle.dispose async
and we do not want to block there. This does not mean that there
is a problem if developer disposes it manually since our emit
functions take care of making sure it disposes (and there is
no other way to add source to the underlying MediatorLiveData)
Bug: 140249349
Test: BuildLiveDataTest#raceTest_*
Change-Id: I0b464c242a583da4669af195cf2504e2adc4de40
M lifecycle/lifecycle-livedata-ktx/api/2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/current.txt
M lifecycle/lifecycle-livedata-ktx/api/public_plus_experimental_2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/public_plus_experimental_current.txt
M lifecycle/lifecycle-livedata-ktx/api/restricted_2.2.0-alpha05.txt
M lifecycle/lifecycle-livedata-ktx/api/restricted_current.txt
M lifecycle/lifecycle-livedata-ktx/src/main/java/androidx/lifecycle/CoroutineLiveData.kt
M lifecycle/lifecycle-livedata-ktx/src/test/java/androidx/lifecycle/BuildLiveDataTest.kt
co...@gmail.com
There's a DEBUG field in SimpleArrayMap that could be helpful, but honestly my recommendation would be to avoid SimpleArrayMap unless you can fully validate the safety of your own usages
We actually don't use it at all, the issue seems to be coming from View classes within the Framework that use it.
al...@google.com
Oh. Oh no.
So, for what it's worth, if you don't have any usages in your app then it's down to libraries. I don't think it's feasible for an app developer to determine which libraries.
Given that this class seems to have been specifically designed to crash in the worst way possible for the worst reason possible, I'm going to reopen this to figure out what the best course of action is going to be here. It may be best to simply deprecate this class and avoid using it in any Jetpack libraries.
yb...@google.com
i think it is time to remove that cache. it is really not useful based on our micro benchmarks anymore anyways.
yb...@google.com
measurements for reference: (sorry for the internal links)
tl;dr; it does get slightly slower for the micro benchmark yet that is basically the best case for this cache and even that does not matter much.
While there is a very small increase of number of objects / bytes with the
cache-less version of SimpleArrayMap, remember this is the memory footprint of
the entire test suite (3 individual tests using a SimpleArrayMap of 1000 key-
value pairs each), the delta (208 bytes for 4 more objects) is very small
relative to the number of objects involved.
al...@google.com
I am very okay with removing the cache given the volume of basically-unresolvable issues that it has caused.
sj...@disneystreaming.com
Any updates on the issue/removal of the cache?
au...@google.com
Currently nobody is working on it, but we'd be happy to take your patch
yb...@google.com
I think we are already removing the problematic cache as part of the kotlin migration.
dustin cc'ed to verify.
du...@google.com
I can confirm we're removing the global array cache.
sj...@disneystreaming.com
That's great to hear! Thanks for the updates.
ap...@google.com
Project: platform/frameworks/support
Branch: androidx-main
commit 4e996bf3fc80bb378714d62c9e2308ad10708466
Author: Dustin Lam <dustinlam@google.com>
Date: Mon Mar 14 12:02:46 2022
Remove global array cache from ArraySet
This cache provides dubious performance value, while causing a bunch of
separate headaches from the linked bugs.
Bug: 139401479
Bug: 182813986
Test: ./gradlew collection:collection:test
Change-Id: I36671e94753e6e1a8ccd5b513931614d1235202b
M collection/collection/src/jvmMain/java/androidx/collection/ArraySet.java
https://android-review.googlesource.com/2025044
Branch: androidx-main
commit 4e996bf3fc80bb378714d62c9e2308ad10708466
Author: Dustin Lam <dustinlam@google.com>
Date: Mon Mar 14 12:02:46 2022
Remove global array cache from ArraySet
This cache provides dubious performance value, while causing a bunch of
separate headaches from the linked bugs.
Bug: 139401479
Bug: 182813986
Test: ./gradlew collection:collection:test
Change-Id: I36671e94753e6e1a8ccd5b513931614d1235202b
M collection/collection/src/jvmMain/java/androidx/collection/ArraySet.java
ap...@google.com
Project: platform/frameworks/support
Branch: androidx-main
commit f3ccc7d4febe58d0b861039d5fd0dd45ec00ad41
Author: Dustin Lam <dustinlam@google.com>
Date: Fri Mar 04 10:34:54 2022
Convert SimpleArrayMap to Kotlin
This also removes the global array cache which provided dubious
performance value while allowing unsychronized access to possibly
corrupt other instances of this class.
Relnote: "Convert SimpleArrayMap to Kotlin. This change introduces a few
incompatible changes, as a result of Java-Kotlin interop and the
ability to correctly define nullity of types in the source.
* The package private APIs, `.mSize`, `.mArray`, `.mHashes`,
`.indexOf()`, `.indexOfNull()`, and `.indexOfValue()`, were made
private - this is technically a binary incompatible change, but
reflects the intended visibility of these fields and is the closest we
can achieve in Kotlin since it does not include a way to specifiy
package-private visibility.
* The nullity of some types are now properly defined, the affected
methods are: `.getOrDefault`, `.keyAt`, `.valueAt`, `.setValueAt`,
`.put`, `.putIfAbsent`, `.removeAt`, `.replace`.
* For Kotlin users, `.isEmpty()` is now only available as a function instead of
also through property access."
Bug: 139401479
Bug: 182813986
Test: ./gradlew bOS
Change-Id: I271b70587e94ef166be71c5b60d8c6361b4b1849
M collection/collection/api/current.ignore
M collection/collection/api/current.txt
M collection/integration-tests/testapp/src/main/kotlin/androidx/collection/integration/SimpleArrayMapKotlin.kt
M collection/collection/api/public_plus_experimental_current.txt
M collection/collection/api/api_lint.ignore
M collection/collection/src/jvmMain/java/androidx/collection/ArrayMap.java
M collection/integration-tests/testapp/src/main/java/androidx/collection/integration/SimpleArrayMapJava.java
M collection/collection/api/restricted_current.txt
M collection/collection/api/restricted_current.ignore
M collection/collection/src/jvmMain/kotlin/androidx/collection/SimpleArrayMap.kt
https://android-review.googlesource.com/2008468
Branch: androidx-main
commit f3ccc7d4febe58d0b861039d5fd0dd45ec00ad41
Author: Dustin Lam <dustinlam@google.com>
Date: Fri Mar 04 10:34:54 2022
Convert SimpleArrayMap to Kotlin
This also removes the global array cache which provided dubious
performance value while allowing unsychronized access to possibly
corrupt other instances of this class.
Relnote: "Convert SimpleArrayMap to Kotlin. This change introduces a few
incompatible changes, as a result of Java-Kotlin interop and the
ability to correctly define nullity of types in the source.
* The package private APIs, `.mSize`, `.mArray`, `.mHashes`,
`.indexOf()`, `.indexOfNull()`, and `.indexOfValue()`, were made
private - this is technically a binary incompatible change, but
reflects the intended visibility of these fields and is the closest we
can achieve in Kotlin since it does not include a way to specifiy
package-private visibility.
* The nullity of some types are now properly defined, the affected
methods are: `.getOrDefault`, `.keyAt`, `.valueAt`, `.setValueAt`,
`.put`, `.putIfAbsent`, `.removeAt`, `.replace`.
* For Kotlin users, `.isEmpty()` is now only available as a function instead of
also through property access."
Bug: 139401479
Bug: 182813986
Test: ./gradlew bOS
Change-Id: I271b70587e94ef166be71c5b60d8c6361b4b1849
M collection/collection/api/current.ignore
M collection/collection/api/current.txt
M collection/integration-tests/testapp/src/main/kotlin/androidx/collection/integration/SimpleArrayMapKotlin.kt
M collection/collection/api/public_plus_experimental_current.txt
M collection/collection/api/api_lint.ignore
M collection/collection/src/jvmMain/java/androidx/collection/ArrayMap.java
M collection/integration-tests/testapp/src/main/java/androidx/collection/integration/SimpleArrayMapJava.java
M collection/collection/api/restricted_current.txt
M collection/collection/api/restricted_current.ignore
M collection/collection/src/jvmMain/kotlin/androidx/collection/SimpleArrayMap.kt
Description
Artifact used Happening across every version. Devices/Android versions reproduced on: All versions.
This is our biggest issue on our Crashlytics dashboard. It has been ongoing for months.
Sample stacktraces:
Fatal Exception: java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Object[] at androidx.collection.SimpleArrayMap.allocArrays(:170) at androidx.collection.SimpleArrayMap.put(:458) at androidx.coordinatorlayout.widget.DirectedAcyclicGraph.addNode(:55) at androidx.coordinatorlayout.widget.CoordinatorLayout.prepareChildren(:698) at androidx.coordinatorlayout.widget.CoordinatorLayout.onMeasure(:767) at android.view.View.measure(View.java:25785)RecyclerView:
androidx.collection.SimpleArrayMap.allocArrays (Unknown Source:170) androidx.recyclerview.widget.RecyclerView.onLayout (Unknown Source:4404)Bottom Navigation:
Fatal Exception: java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Object[] at androidx.collection.SimpleArrayMap.allocArrays(:170) at androidx.collection.SimpleArrayMap.put(:458) at androidx.transition.Transition.addViewValues(:1532) at androidx.transition.Transition.captureHierarchy(:1627) at androidx.transition.Transition.captureHierarchy(:1650) at androidx.transition.Transition.captureHierarchy(:1650) at androidx.transition.Transition.captureHierarchy(:1650) at androidx.transition.Transition.captureValues(:1511) at androidx.transition.TransitionManager.sceneChangeSetup(:318) at androidx.transition.TransitionManager.beginDelayedTransition(:418) at com.google.android.material.bottomnavigation.BottomNavigationMenuView.updateMenuView(:590) at com.google.android.material.bottomnavigation.BottomNavigationPresenter.updateMenuView(:69) at androidx.appcompat.view.menu.MenuBuilder.dispatchPresenterUpdate(:292) at androidx.appcompat.view.menu.MenuBuilder.onItemsChanged(:1063) at androidx.appcompat.view.menu.MenuBuilder.startDispatchingItemsChanged(:1090) at androidx.appcompat.view.menu.MenuBuilder.setExclusiveItemChecked(:627) at androidx.appcompat.view.menu.MenuItemImpl.setChecked(:622) at com.google.android.material.bottomnavigation.BottomNavigationMenuView$1.onClick(:128) at android.view.View.performClick(View.java:6291) at android.view.View$PerformClick.run(View.java:24931) at android.os.Handler.handleCallback(Handler.java:808) at android.os.Handler.dispatchMessage(Handler.java:101) at android.os.Looper.loop(Looper.java:166) at android.app.ActivityThread.main(ActivityThread.java:7529) at java.lang.reflect.Method.invoke(Method.java) at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:245) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:921)There are many different variations, but they all have one thing in common:
androidx.collection.SimpleArrayMapIt has happened to me on occasion, there's no pattern to it. Sometimes I'll just start the app, press a button on the BottomNavigationView then it crashes with
java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Object[]To be clear, we are not using
SimpleArrayMapin our code, this is coming from within the Framework. There are several cases in our dashboard, mostly stemming from View methodsmeasure,layout,performClick,dispatchOnPreDraw, etc.