Status Update
No update yet.
Comments
du...@google.com
Initial loads in LimitOffsetPagingSource run in transaction but subsequent loads do not because of cost of transaction. Loads rely on count query to return correct LoadResult.
If count query is outdated, i.e. large amount of data deleted, LimitOffsetPagingSource can return i.e. empty/wrong data and incorrect itemsBefore/itemsAfter/prevKey/nextKey. This can happen if outdated PagingSource doesn't get invalidated in time.
du...@google.com
Branch: androidx-main
commit 6bd2347ff259c28b34789c8569b7a298d2a8fd8d
Author: Clara Fok <clarafok@google.com>
Date: Fri Jul 09 12:53:22 2021
Implement Invalid return type in PagingSource
Added third return type for LoadResult sealed class in PagingSource. It
was added as a support feature to handle race scenarios between data loads
and database invalidations, such as if Room's InvalidationTracker does
not propagate invalidation signal to the PagingSource in time and the
PagingSource continues querying an updated database. This can result in
data being loaded onto a stale PageFetcherSnapshot causing duplicated or
missing data. Related bug regarding race scenario in Room
For example, if data was deleted and items shifted positions,
positionally-keyed paging sources can end up loading duplicated items.
Paging handles this return type by discarding the loaded data and
invalidating the paging source. This will trigger a new paging source to
be generated. This return type is also supported for Paging2 APIs
leveraging PagingSource such as LivePagedList and RxPagedList.
Aside from handling invalid data due to races, this return type can be
leveraged in general where the database or network returns potentially
invalid or stale data that needs to be discarded.
Bug: 192013267
Test: ./gradlew :paging:paging-common:test
Relnote: "A third LoadResult return type LoadResult.Invalid is added to
PagingSource. When a PagingSource.load returns
LoadResult.Invalid, paging will discard the loaded data and
invalidate the PagingSource. This return type is designed to
handle potentially invalid or stale data that can be returned
from the database or network.
For example, if the underlying database gets written into but
the PagingSource does not invalidate in time, it may return
inconsistent results if its implementation depends on the
immutability of the backing dataset it loads from (e.g., LIMIT
OFFSET style db implementations). In this scenario, it is
recommended to check for invalidation after loading and to
return LoadResult.Invalid, which causes Paging to discard any
pending or future load requests to this PagingSource and
invalidate it.
This return type is also supported by Paging2 API that leverages
LivePagedList or RxPagedList. When using a PagingSource with
Paging2's PagedList APIs, the PagedList is immediately detached,
stopping further attempts to load data on this PagedList and
triggers invalidation on the PagingSource.
LoadResult is a sealed class, which means this is a
source-incomptabile change such that use cases directly using
PagingSource.load results will have to handle LoadResult.Invalid
at compile time. For example, Kotlin users leveraging
exhaustive-when to check return type will have to add a check
for Invalid type."
Change-Id: Id6bd3f2544f1ba97a95a0d590353438a95fedf2a
M paging/common/api/current.txt
M paging/common/api/public_plus_experimental_current.txt
M paging/common/api/restricted_current.txt
M paging/common/src/main/kotlin/androidx/paging/PageFetcherSnapshot.kt
M paging/common/src/main/kotlin/androidx/paging/PagedList.kt
M paging/common/src/main/kotlin/androidx/paging/PagingSource.kt
M paging/common/src/test/kotlin/androidx/paging/PageFetcherSnapshotTest.kt
M paging/common/src/test/kotlin/androidx/paging/PageFetcherTest.kt
M paging/common/src/test/kotlin/androidx/paging/PagedListTest.kt
M paging/common/src/test/kotlin/androidx/paging/PagingDataDifferTest.kt
M paging/common/src/test/kotlin/androidx/paging/PagingSourceTest.kt
M paging/integration-tests/testapp/src/main/java/androidx/paging/integration/testapp/v3room/V3RemoteMediator.kt
M paging/integration-tests/testapp/src/main/java/androidx/paging/integration/testapp/v3room/V3RoomViewModel.kt
lo...@gmail.com
Branch: androidx-main
commit 3882ac8364fd905c9935d38c93f84599009226d6
Author: Clara Fok <clarafok@google.com>
Date: Fri Jul 09 14:17:48 2021
Implement LoadResult.Invalid to handle race
For non-initial loads in LimitOffsetPagingSource, loads will run Room
InvalidationTracker's refreshVersionSync to synchronously check if
tables have been invalidated and notify observers. If invalidated,
load will return LoadResult.Invalid.
Paging3 will handle Invalid return type by invalidating the paging
source and closing the page event flow, such that no more loads will be
attempted on this paging source.
Note that refreshVersionSync is a temporary solution for
LimitOffsetPagingSource to check table invalidation. Another solution
that does not require a transaction is in development. Related
Bug: 191806126
Test: ./gradlew :room:integration-tests:room-testapp-kotlin:cC
Test: ./gradlew room:room-paging:cC
Change-Id: I48634ab95949bae608374f5156a4edd0500a74a3
M room/integration-tests/kotlintestapp/src/androidTest/java/androidx/room/integration/kotlintestapp/test/PagingSourceTest.kt
M room/room-paging/build.gradle
M room/room-paging/src/androidTest/kotlin/androidx/room/paging/LimitOffsetPagingSourceTest.kt
M room/room-paging/src/main/kotlin/androidx/room/paging/LimitOffsetPagingSource.kt
du...@google.com
I meant that adding @RequiredOptIn doesn't prevent java callers from calling into the API because as far as I know there is a java-compatible way to opt-in.
lo...@gmail.com
Yes, Java code doesn't see @RequiresOpIn annotated annotations, and in that case, that's exactly what we need to keep this API for Java callers only.
du...@google.com
Sorry - I think I'm understanding you now. What I'm trying to say is that because we also publish an experimental annotation linter + annotation for java (that picks up kotlin experimental), we treat experimental api the same for both java and kotlin callers.
cc...@google.com
Dustin, the suggestion is to use a separate annotation, like @RequiresOptIn(PagingSyncMethodsInKotlin). Would that be enforced for java callers as well, due to the linter hooks, or can it differentiate experimental vs something else that's opt-in?
I think my issue with the optin for the blocking methods is that it's extra project complexity (admittedly small) for not much gain. I think the suffix is deterrent enough for Kotlin callers. If anyone tries to call a suspending method it won't work, and I think it's pretty easy to find that you need to remove "Sync" to call suspending methods.
One other potential problem (I haven't checked) - I'm also not sure if @RequiresOptIn(PagingSyncMethodsInKotlin) would cause confusion during a Java->Kotlin project migration. Could be that Idea/Studio would be too clever and convert an old java filter() call to filterBlocking(), which would confusingly tell you to add an opt-in.
du...@google.com
The annotation-experimental-lint artifact that we publish will enforce it for java callers
du...@google.com
Experimental / RequiresOptIn are interchangeable here as one was deprecated in favor of the other in Kotlin, I think for the java annotation that we publish to mimic this behavior, we stuck with the Experimental naming, but I could be misremembering..
The idea of adding friction for sync method usage in Kotlin I'm not against though - I'm just not sure OptIn is the right way to do that.
lo...@gmail.com
I'm also not sure if
@RequiresOptIn(PagingSyncMethodsInKotlin)would cause confusion during a Java->Kotlin project migration. Could be that Idea/Studio would be too clever and convert an old javafilter()call tofilterBlocking(), which would confusingly tell you to add an opt-in.
If the @PagingSyncMethodsInKotlin annotation has internal visibility, it'll be impossible to opt-in.
The message defined in that annotation will be shown by the IDE as the error message, so it'd not be confusing for long, granted the message directs toward the non Sync or Blocking suffixed equivalents.
lo...@gmail.com
About the annotation-experimental-lint artifact: Does it enforce it only for a given set of annotations explicitly opting-in to that lint, or from a special list of these annotations, or does it apply to all @RequiresOptIn annotations in AndroidX, or even beyond?
du...@google.com
It's supposed to pick up any usages of Kotlin's RequiresOptIn and androidx.annotation.experimental.Experimental, expose that as a lint error and allow Java users to opt in by annotating with androidx.annotation.experimenetal.UseExperimental
lo...@gmail.com
Shouldn't this become configurable, or have the action scope reduced, then?
du...@google.com
Rather than add a filter to the java experimental annotation lint, it's probably more correct to introduce a new lint that picks up a separate @JavaApi or @KotlinApi annotation (for either direction).
Description
Component used: Paging Version used: 3.0.0-alpha08
Hello!
Since the
filterSync,mapSyncandflatMapSyncfunctions onPagingDataare not bringing any optimizations compared to the nonSyncprefixed ones that take suspending lambdas, I think it'd be great to discourage their use in Kotlin code, or simply make it impossible to use via an internalRequiresOptInannotation (Java doesn't seem them) that'd direct towardsfilterand friends.If you agree with me, I can make a PR on GitHub for this.