Fixed
Status Update
No update yet.
Comments
li...@gmail.com
Yigit, do you have time to fix it?
reemission of the same liveData is racy
reemission of the same liveData is racy
du...@google.com
yea i'll take it.
du...@google.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.
du...@google.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()
}
}
ta...@monzo.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)
yb...@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} )
}
ta...@monzo.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
yb...@google.com
Without stable list, jumping won't really be feasible. I think the only way it can work is to create a new pager w/ an initial key to restart the data from that location (combined w/ scrollToPosition(0) )
du...@google.com
While playing with my workaround I noticed it only works in some cases and as there is a separate bug causing multiple jumps to happen on scroll to top. This is why it ends up scrolling downwards after jumping to the top due to jumping.
I believe the root cause is due to conflated hints causing new generation to reload a second time.
du...@google.com
So it looks like the second jump up is due to the differ events out of diffutil, it doesn't understand the overlap since the list sizes are mismatches so we need to add some special case handling in paging for this.
ap...@google.com
Project: platform/frameworks/support
Branch: androidx-main
commit 4eabdc68446c5268a9a6f1d80546703eba18d3b1
Author: Yigit Boyar <yboyar@google.com>
Date: Fri Mar 05 10:47:07 2021
Improve NullPaddedList diffing
This CL updates NullPaddedList diffing heuristic to better optimize for
common cases.
There two common cases we optimize for:
a) For completely distinct lists, we dispatch them as change animations
whenever possible based on positions (as long as there is no item in the
same global position in the old and new list)
b) For cases where lists partially overlap, we use available
placeholders to dispatch inserts or convert removals to placeholders
WHEN those updates happen at the boundaries of the list. This especially
handles cases where placeholders are loaded and prevents recyclerview
from scrolling to keep placeholders on screen.
See NullPaddedDiffing.md document for more details.
Bug: 170027529
Bug: 177338149
Test: NullPaddedListDiffWithRecyclerViewTest
Test: AsyncPagedListDifferTest
Test: NullPaddedListDiffHelperTest
Relnote: "We've rewamped how placeholders are handled when list is
reloaded to prevent unexpected jumps in RecyclerView. See
NullPaddedDiffing.md for details."
Change-Id: If1490f5bc833a61793d27eeaae9b37b26153df87
M paging/common/api/public_plus_experimental_3.0.0-beta03.txt
M paging/common/api/public_plus_experimental_current.txt
M paging/common/src/main/kotlin/androidx/paging/PagingDataDiffer.kt
M paging/runtime/build.gradle
M paging/runtime/src/androidTest/java/androidx/paging/AsyncPagedListDifferTest.kt
M paging/runtime/src/androidTest/java/androidx/paging/AsyncPagingDataDifferTest.kt
M paging/runtime/src/androidTest/java/androidx/paging/NullPaddedListDiffHelperTest.kt
A paging/runtime/src/androidTest/java/androidx/paging/NullPaddedListDiffWithRecyclerViewTest.kt
M paging/runtime/src/main/java/androidx/paging/AsyncPagedListDiffer.kt
A paging/runtime/src/main/java/androidx/paging/NullPaddedDiffing.md
M paging/runtime/src/main/java/androidx/paging/NullPaddedListDiffHelper.kt
https://android-review.googlesource.com/1621325
Branch: androidx-main
commit 4eabdc68446c5268a9a6f1d80546703eba18d3b1
Author: Yigit Boyar <yboyar@google.com>
Date: Fri Mar 05 10:47:07 2021
Improve NullPaddedList diffing
This CL updates NullPaddedList diffing heuristic to better optimize for
common cases.
There two common cases we optimize for:
a) For completely distinct lists, we dispatch them as change animations
whenever possible based on positions (as long as there is no item in the
same global position in the old and new list)
b) For cases where lists partially overlap, we use available
placeholders to dispatch inserts or convert removals to placeholders
WHEN those updates happen at the boundaries of the list. This especially
handles cases where placeholders are loaded and prevents recyclerview
from scrolling to keep placeholders on screen.
See NullPaddedDiffing.md document for more details.
Bug: 170027529
Bug: 177338149
Test: NullPaddedListDiffWithRecyclerViewTest
Test: AsyncPagedListDifferTest
Test: NullPaddedListDiffHelperTest
Relnote: "We've rewamped how placeholders are handled when list is
reloaded to prevent unexpected jumps in RecyclerView. See
NullPaddedDiffing.md for details."
Change-Id: If1490f5bc833a61793d27eeaae9b37b26153df87
M paging/common/api/public_plus_experimental_3.0.0-beta03.txt
M paging/common/api/public_plus_experimental_current.txt
M paging/common/src/main/kotlin/androidx/paging/PagingDataDiffer.kt
M paging/runtime/build.gradle
M paging/runtime/src/androidTest/java/androidx/paging/AsyncPagedListDifferTest.kt
M paging/runtime/src/androidTest/java/androidx/paging/AsyncPagingDataDifferTest.kt
M paging/runtime/src/androidTest/java/androidx/paging/NullPaddedListDiffHelperTest.kt
A paging/runtime/src/androidTest/java/androidx/paging/NullPaddedListDiffWithRecyclerViewTest.kt
M paging/runtime/src/main/java/androidx/paging/AsyncPagedListDiffer.kt
A paging/runtime/src/main/java/androidx/paging/NullPaddedDiffing.md
M paging/runtime/src/main/java/androidx/paging/NullPaddedListDiffHelper.kt
du...@google.com
After this change - setting jump threshold now works as expected :)
Description
Version used: 3.0.0-alpha11
Devices/Android versions reproduced on: All devices
I've attached a modified version of the PagingSample app, which demonstrates the problem.
The only changes I made where to:
- Add a button at the bottom of the layout which is supposed to scroll the list of cheeses to the top
- Add separators in random intervals between cheeses (whenever the previous cheese name has more than 9 characters)
- Prefixed cheese rows with the adapter position. I know that this is not always correct due to paging, but it helps a bit to know where you are at each point.
Steps to reproduce the problem:
- Scroll all the way to bottom of the list (or at least more than maxSize)
- Press the button to scroll to the top
What happens:
- You are positioned higher in the list, but not at the top
What should happen:
- You should be positioned to the top of the list
My theory is that the placeholders only appear for the rows that come from the DB, but not for the date separators. That’s why all the calculations are wrong when a page is dropped from the top. If you remove the separators, the problem goes away.
Of course the problem is that in order to calculate whether a placeholder should appear, you need the information from the db. In a real world scenario, think of date separators in a timeline feed. So I don't know whether there is a solution to this problem, but I'd love to hear it.
PS: In my real app, all I really want to do is scroll to the top. Ideally we'd need a solution that works for every position, but if you have a workaround for just going all the way to the top, that's good enough for me for now.