Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IndexOutOfBoundsException In OffsetQueryPagingSource #2434

Closed
kkovach opened this issue Jun 10, 2021 · 17 comments · Fixed by julioromano/mooviez#19
Closed

IndexOutOfBoundsException In OffsetQueryPagingSource #2434

kkovach opened this issue Jun 10, 2021 · 17 comments · Fixed by julioromano/mooviez#19

Comments

@kkovach
Copy link

kkovach commented Jun 10, 2021

SQLDelight version: 1.5.0
Application OS: Android

I am loading a database from the network in pages and using the android-paging3 extension to feed a RecyclerView with the data from the database. On occasion I am seeing the following exception and crash. I believe it does appear to be some kind of timing issue but I cannot tell how/why it is happening? I was hoping that someone could help with some advice or tips on what to look at as far as loading page size and PagingConfig (page size, prefetchsize, etc...) where something is off and creating this exception.

java.lang.IndexOutOfBoundsException
    at com.squareup.sqldelight.android.paging3.OffsetQueryPagingSource$load$2$1.invoke(OffsetQueryPagingSource.kt:40)
    at com.squareup.sqldelight.android.paging3.OffsetQueryPagingSource$load$2$1.invoke(OffsetQueryPagingSource.kt:24)
    at com.squareup.sqldelight.TransacterImpl.transactionWithWrapper(Transacter.kt:218)
    at com.squareup.sqldelight.TransacterImpl.transactionWithResult(Transacter.kt:204)
    at com.squareup.sqldelight.Transacter$DefaultImpls.transactionWithResult$default(Transacter.kt:71)
    at com.squareup.sqldelight.android.paging3.OffsetQueryPagingSource$load$2.invokeSuspend(OffsetQueryPagingSource.kt:38)

Thanks for the help!

@kkovach kkovach added the bug label Jun 10, 2021
@kkovach
Copy link
Author

kkovach commented Jun 12, 2021

if (count != 0L && key >= count) throw IndexOutOfBoundsException()

It appears to be happening when key == count every time. I'm not sure I understand the relationship between key and count and why they cannot be equal?

@kevincianfarini
Copy link
Collaborator

kevincianfarini commented Jun 12, 2021

Hi @kkovach. Are you able to provide a sample reproduction that I can inspect? keys are 0 indexed. This is similar to a bounds check when accessing an element in a list. I would be curious to see where the problem stems from.

@kkovach
Copy link
Author

kkovach commented Jun 21, 2021

Hey @kevincianfarini. I've been working with this a little more and I am wondering if this has to do with the getRefreshKey implementation? Could it be that I'm getting an anchor position that is outside the bounds of the query count when scrolling because of timing or something? It always seems to happen when I am scrolling and loading more data. Also, it always happens that key is equal to count when it fails.

I went ahead and copied the class locally and change the getRefreshKey to the following...

override fun getRefreshKey(state: PagingState<Long, RowType>): Long? {
        return minOf(state.anchorPosition?.toLong() ?: 0, countQuery.executeAsOne().minus(1))
}

I believe this makes sense? And I am no longer getting any index out of bounds exceptions. Thoughts? Thanks.

@kevincianfarini
Copy link
Collaborator

@kkovach I'm not sure! It's difficult to debug without a small sample project that has a reproduction of the bug. Do you think you'd be able to isolate one?

@kkovach
Copy link
Author

kkovach commented Jun 22, 2021

@kevincianfarini I'll see what I can do when I get some time.

@jakeaaron
Copy link

I'm also seeing this error. Curious if i'm implementing something incorrectly, or something else is up..

@jakeaaron
Copy link

Ahh, I think it was because the initial page size was different than subsequent page sizes. Setting initial and page size in the PagingConfig fixed the issue for me.

This is a quote from the sqldelight docs:

The AndroidX paging library allows for the first page fetch to differ in size from the subsequent page fetches with PagingConfig.initialLoadSize. This functionality should be avoided, as the pageBoundariesProvider callback is invoked a single time on the first page fetch. Failing to have matching PagingConifg.initialLoadSize and PagingConfig.pageSize will result in unexpected page boundary generation.

@kevincianfarini
Copy link
Collaborator

Hi @jakeaaron! The piece you've quoted from the documentation should only apply to the keyset paging functionality. If you're using offset based paging and still seeing this issue, I would be extremely appreciative if you could conjure a reproduction sample for me that I could investigate.

@kevincianfarini
Copy link
Collaborator

kevincianfarini commented Jul 14, 2021

@kkovach I suspect that getRefreshKey is also a likely culprit. From the docs:

Provide a [Key] used for the initial [load] for the next [PagingSource] due to invalidation of this [PagingSource]. The [Key] is provided to [load] via [LoadParams.key].

It's possible that the underlying data source is experiencing a deletion event, which invalidates the data source. Upon invalidation, the refresh key that's grabbed off of the current source is out of bounds of the newly created source. I can try to write an integration test to confirm this behavior, and hopefully fix it.

kevincianfarini added a commit to kevincianfarini/sqldelight that referenced this issue Jul 15, 2021
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.
kevincianfarini added a commit to kevincianfarini/sqldelight that referenced this issue Jul 15, 2021
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.
kevincianfarini added a commit to kevincianfarini/sqldelight that referenced this issue Jul 15, 2021
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.
AlecKazakova pushed a commit to kevincianfarini/sqldelight that referenced this issue Jul 16, 2021
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.
@tiagonuneslx
Copy link

I'm experiencing this in 1.5.1

kevincianfarini added a commit to kevincianfarini/sqldelight that referenced this issue Jul 28, 2021
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.
rsookram added a commit to rsookram/rss that referenced this issue Nov 5, 2021
to reduce the number of notifications, which mitigates the impact of
cashapp/sqldelight#2434.
kevincianfarini added a commit to kevincianfarini/sqldelight that referenced this issue Dec 13, 2021
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.
AlecKazakova pushed a commit to kevincianfarini/sqldelight that referenced this issue Jan 4, 2022
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.
@bubenheimer
Copy link

bubenheimer commented Jan 16, 2022

I've run into the same problem. It's caused by the special logic in OffsetQueryPagingSource that lets the prevKey go negative in a misaligned query situation.

It's a pretty easy repro, actually. For example, with Compose LazyPagingItems, a constant row count of 2, a large pageSize of 100, and an even larger initialLoadSize of 150, here's a debug trace on

The trace shows execution state from a few consecutive query invalidations before the exception ends up getting triggered. There are no changes to the table, all this happens purely via invalidations:

Count: 2  key: 0  params.loadSize: 150  params: androidx.paging.PagingSource$LoadParams$Refresh@7051d4a
Count: 2  key: 0  params.loadSize: 150  params: androidx.paging.PagingSource$LoadParams$Refresh@5679dd8
Count: 2  key: 1  params.loadSize: 150  params: androidx.paging.PagingSource$LoadParams$Refresh@a714d4e
Count: 2  key: -149  params.loadSize: 100  params: androidx.paging.PagingSource$LoadParams$Prepend@75f28b
Count: 2  key: 2  params.loadSize: 150  params: androidx.paging.PagingSource$LoadParams$Refresh@1719274

The state on the final line triggers the exception. The prior line already shows the mess, and can cause duplicate key errors in the UI, with 3 rows being displayed instead of 2. It's easy to walk through the code with this info to pinpoint the issue.

It seems to me that the code and tests are based on a false understanding of how prevKey and Prepend work. My understanding is that prevKey is intended to be the key right before the first key returned in the LoadResult, not the key minus current page offset that the current implementation seems to use. A Prepend load needs to be fulfilled by taking its key and going backwards from it. The key of a Prepend is not intended to be offset already, a Prepend reverses the paging direction. I don't mean reversing the row order of the LoadResult, just reversing the offset direction.

@bubenheimer
Copy link

bubenheimer commented Jan 30, 2022

Also, getRefreshKey() really should not return state.anchorPosition(). I know the paging documentation offers that as a suggestion or for illustration, but unless the user was scrolling up most recently, state.anchorPosition() is a bad choice to use as the next refresh key. At least with Compose LazyPagingItems this causes flicker and multiple queries being run at the wrong offsets where a single one would suffice.

I've found that short of some complex custom strategy, returning null works much better, at least for LazyPagingItems. LazyPagingItems seems to have a reasonable strategy in place to figure out what to do in the absence of additional information.

@BassirouRabo
Copy link

i got the same error when testing the code bellow

LaunchedEffect(key1 = true) {
        scope.launch {
            while (true) {
                delay(10000)
                lazyPagingItems.refresh()
            }
        }
    }
    ```
 he data is being updated in the background every 5seconds.
the crash happens whenever i scroll to the bottom

E/AndroidRuntime: FATAL EXCEPTION: main
Process: com.esdiac.android, PID: 7011
java.lang.IndexOutOfBoundsException
at com.squareup.sqldelight.android.paging3.OffsetQueryPagingSource$load$2$1.invoke(OffsetQueryPagingSource.kt:40)
at com.squareup.sqldelight.android.paging3.OffsetQueryPagingSource$load$2$1.invoke(OffsetQueryPagingSource.kt:38)
at com.squareup.sqldelight.TransacterImpl.transactionWithWrapper(Transacter.kt:235)
at com.squareup.sqldelight.TransacterImpl.transactionWithResult(Transacter.kt:221)
at com.squareup.sqldelight.Transacter$DefaultImpls.transactionWithResult$default(Transacter.kt:61)
at com.squareup.sqldelight.android.paging3.OffsetQueryPagingSource$load$2.invokeSuspend(OffsetQueryPagingSource.kt:38)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:571)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:678)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:665)

@AlecKazakova AlecKazakova added this to the 2.0 milestone Apr 16, 2022
veyndan pushed a commit to kevincianfarini/sqldelight that referenced this issue Jul 19, 2022
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
Copy link
Collaborator

veyndan commented Dec 7, 2022

This should be resolved by #3396 which is in 2.0.0-alpha04.

@veyndan veyndan closed this as completed Dec 7, 2022
@levon93
Copy link

levon93 commented Feb 1, 2023

@veyndan Is it possible to release this fix with 1.5.* ? Facing this issue when using paging3.QueryPagingSource.

@veyndan
Copy link
Collaborator

veyndan commented Feb 9, 2023

@veyndan Is it possible to release this fix with 1.5.* ? Facing this issue when using paging3.QueryPagingSource.

@AlecStrong Thoughts? I suppose it depends on how soon 2.0.0 will be out.

@AlecKazakova
Copy link
Collaborator

No target date, we're not going to target bugfixes for the 1.5.* branches in general, only compatibility fixes. If you need the bugfix I'd recommend switching to the alpha. You could switch to the alpha for only the paging artifact potentially, you would have to write a shim between the 1.5 runtime and 2.x runtime but i dont think it would be impossible

julioromano added a commit to julioromano/mooviez that referenced this issue Mar 17, 2023
Unfortunately it crashes due to a sqldelight bug. Must upgrade to 2.0.0 alpha

cashapp/sqldelight#2434
julioromano added a commit to julioromano/mooviez that referenced this issue Mar 17, 2023
Unfortunately it crashes due to a sqldelight bug. Must upgrade to 2.0.0 alpha

cashapp/sqldelight#2434
julioromano added a commit to julioromano/mooviez that referenced this issue Mar 17, 2023
- Uses RemoteMediator to page the movie list from the database while fetching new data from the network.
- Upgrades SQLDelight to 2.0.0-alpha05 to fix cashapp/sqldelight#2434
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants