Fix IndexOutOfBounds after getRefreshKey #2477
Conversation
|
I'm not a member of this repo so I can't do it, but could @AlecStrong please request @dlam as a reviewer? I'm curios to see if they ran into similar problems when implementing paging3 with Room. |
kevincianfarini
force-pushed
the
fix-out-of-bounds
branch
from
db8bca5
to
4bd7497
Compare
|
We can't add non-member reviewers, but he'll see it from the @-mention and can always review. |
kevincianfarini
force-pushed
the
fix-out-of-bounds
branch
from
4bd7497
to
a466f0c
Compare
...oid-paging3/src/main/java/com/squareup/sqldelight/android/paging3/OffsetQueryPagingSource.kt
Outdated
Show resolved
Hide resolved
| @@ -34,9 +34,13 @@ internal class OffsetQueryPagingSource<RowType : Any>( | |||
| params: LoadParams<Long> | |||
| ): LoadResult<Long, RowType> = withContext(dispatcher) { | |||
| try { | |||
| val key = params.key ?: 0L | |||
| transacter.transactionWithResult { | |||
| val count = countQuery.executeAsOne() | |||
There was a problem hiding this comment.
Btw, it is expensive to use a transaction on every load because in sqlite (I assume the default for main use-case for sqldelight) it blocks all read/write to any table, so what we do in Room is cache this count and only get it for initial load. Any write into the DB should invalidate anyway so this count should never change while this PagingSource is able to return valid results.
There was a problem hiding this comment.
Good idea.
There was a problem hiding this comment.
On the topic of invalidation, although you use a transaction here, it is possible for a write to happen after PagingSource.load is called, but before you begin your transaction, and in the scenario where you return LoadResult.Page here before invalidation is able to propagate cancellation on this generation, it is possible you will return inconsistent results. Starting from the next Paging release (3.1.0-alpha03), we'll be adding a LoadResult.Invalid return type you can use to get Paging to discard your LoadResult. You will want to check if you are about to be invalidated after you finish loading from DB but before you return and return LoadResult.Invalid instead.
There was a problem hiding this comment.
This is very complicated so here is a more concrete example:
- DB has items [1, 2, 3, 4]
- PagingSource_1 loads [1, 2] and Paging displays those two items
- A write to DB is kicked off to insert item 0
- Paging library asks PagingSource_1 for an APPEND
- Your load starts a transaction for the APPEND which needs to wait for the write to finish
- Write finishes, DB now has items [0, 1, 2, 3, 4]
- PagingSource_1 loads with
params.key == 2, this results in LoadResult.Page(data = [2, 3]) - Paging presents items [1, 2, 2, 3], duplicating one of the rows.
- Invalidation callback finally triggers cancellation and a new generation.
- Now PagingSource_2 is created to load data from scratch.
While you wait for PagingSource_2 to finish loading and finally send to RV to get presented, you have the duplicate row shown to UI incorrectly.
Note that it doesn't matter when you trigger invalidation, even if you are able to synchronously do it, I don't think it's possible to guarantee that cancellation successfully propagates before load completes and sends it off to presenter to show the accidentally duplicated row.
There was a problem hiding this comment.
I factored out the transaction like you mentioned. I think we'll wait until the new API in paging 3.1 is released before we worry about the complicated invalidation.
|
/rebase |
AlecKazakova
force-pushed
the
fix-out-of-bounds
branch
from
a466f0c
to
0e16c84
Compare
|
Thanks for the rebase. Please don't merge anything yet. I'm unable to mark this as a WIP for some reason, but I still have some work to do. |
kevincianfarini
force-pushed
the
fix-out-of-bounds
branch
3 times, most recently
from
acb2368
to
56a3545
Compare
|
FYI: |
|
@dlam I don't believe we'd be able to release a paging extension based on an alpha paging library. @JakeWharton or @AlecStrong please correct me if I'm wrong, and I'll update this PR. |
|
Yep, we'll have to wait for it to go stable. |
|
I mean you can update the PR to the alpha and confirm tests pass and stuff, but I wouldn't merge until stable so that if we do a release we don't accidentally release an unstable API surface. |
|
paging 3.1.0 is released! |
|
FWIW, Room's integration may be a useful example: https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:room/room-paging/src/main/kotlin/androidx/room/paging/LimitOffsetPagingSource.kt;l=85?q=LimitOffsetPagingSource |
kevincianfarini
force-pushed
the
fix-out-of-bounds
branch
from
56a3545
to
8d75de6
Compare
|
/rebase |
AlecKazakova
force-pushed
the
fix-out-of-bounds
branch
from
b38d280
to
7a3e68d
Compare
It's possible that on a paging source that's not in the initial generation, we fail with an `IndexOutOfBoundsException`. This is caused when the underlying data source undergoes a delete, which then invalidates the paging source, which then calls `getRefreshKey`. In the event that the the refresh key is the very last index of the previous paging source, we would fail with the above exception because that index does not exist in the next generation paging source. Closes cashapp#2434.
veyndan
force-pushed
the
fix-out-of-bounds
branch
from
7a3e68d
to
2f7e515
Compare
veyndan
force-pushed
the
fix-out-of-bounds
branch
from
2f7e515
to
5df7c5b
Compare
It's possible that on a paging source that's not in the initial
generation, we fail with an
IndexOutOfBoundsException. Thisis caused when the underlying data source undergoes a delete, which
then invalidates the paging source, which then calls
getRefreshKey. In the event that the the refresh key is thevery last index of the previous paging source, we would fail
with the above exception because that index does not exist
in the next generation paging source.
Closes #2434.