epoxy icon indicating copy to clipboard operation
epoxy copied to clipboard

IndexOutOfBoundsException using Paging3

Open KirkBushman opened this issue 4 years ago • 11 comments
trafficstars

Hey, I've moved one of my projects from Paging2 to Paging3 and I'm noticing an Exception.

E/AndroidRuntime: FATAL EXCEPTION: main
    Process: my_process, PID: 26595
    java.lang.IndexOutOfBoundsException: Index: 1, Size: 1
        at java.util.ArrayList.get(ArrayList.java:437)
        at androidx.paging.PagePresenter.getFromStorage(PagePresenter.kt:87)
        at androidx.paging.PagePresenter.get(PagePresenter.kt:61)
        at androidx.paging.PagingDataDiffer.get(PagingDataDiffer.kt:249)
        at androidx.paging.AsyncPagingDataDiffer.getItem(AsyncPagingDataDiffer.kt:220)
        at com.airbnb.epoxy.paging3.PagedDataModelCache.triggerLoadAround(PagedDataModelCache.kt:200)
        at com.airbnb.epoxy.paging3.PagedDataModelCache.getModels(PagedDataModelCache.kt:152)
        at com.airbnb.epoxy.paging3.PagingDataEpoxyController.buildModels(PagingDataEpoxyController.kt:69)
        at com.airbnb.epoxy.EpoxyController$1.run(EpoxyController.java:281)
        at android.os.Handler.handleCallback(Handler.java:938)
        at android.os.Handler.dispatchMessage(Handler.java:99)
        at android.os.Looper.loop(Looper.java:223)
        at android.app.ActivityThread.main(ActivityThread.java:7656)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)

I updated to the very latest version of Epoxy (4.6.2), but I can see this issue with 4.5.0 as well. I'm guessing it's some edge case with the paging3 integration, it's happening while reloading the list content and showing the loading item on the screen. It's not happening all the time, seemly at random.

Can anyone guess what's the problem? or a potential quick fix?

Thanks in advance.

KirkBushman avatar Aug 15 '21 21:08 KirkBushman

@elihart

Ok, I think I got the problem.

I moved the Controller from the paging integration library to the paging3 library. The addModels() function looks like this. I'm adding extra models based on the state, it looks like this is causing an IndexOutOfBoundsException on paging 3.0.0.

override fun addModels(models: List<EpoxyModel<*>>) {

        when (state) {
            STATE_LOADING -> getLoadingModel()
            STATE_EMPTY -> getEmptyModel()
            STATE_ERROR -> getErrorModel()
        }

        if (state == STATE_NORMAL || state == STATE_UPDATING) {
            super.addModels(models)
        }

        if (state == STATE_UPDATING) {
            getUpdatingModel()
        }
    }

Moving the function to this one, makes the crash disappear:

override fun addModels(models: List<EpoxyModel<*>>) {

        val tempList = ArrayList<EpoxyModel<*>>()

        when (state) {
            STATE_LOADING -> tempList.add(getLoadingModel2())
            STATE_EMPTY -> tempList.add(getEmptyModel2())
            STATE_ERROR -> tempList.add(getErrorModel2())
        }

        if (state == STATE_NORMAL || state == STATE_UPDATING) {
            tempList.addAll(models)
        }

        if (state == STATE_UPDATING) {
            tempList.add(getUpdatingModel2())
        }

        super.addModels(tempList)
}

Should we exchange this approach for a function that returns a list, to make it more clear?

KirkBushman avatar Aug 20 '21 17:08 KirkBushman

In your first example, what is getUpdatingModel()? I can't tell what side effect that has, it looks like it isn't doing anything.

If it is error prone to use this API correctly then it does seem like we should do something to improve it. Changing the function signature is a breaking change though which is not ideal - another possibility is to add internal safety checks that make the error message much more clear so you can easily understand what you've done wrong and what to do instead

elihart avatar Aug 20 '21 17:08 elihart

In the first example, I'm using the Kotlin builder to add the model

loading { 
    id("model_loading") 
}

On the second one, I'm using the standard way to return the model

LoadingModel_()
            .id("model_loading")

KirkBushman avatar Aug 20 '21 17:08 KirkBushman

So are you suggesting in some way, to check that the total models on the controller are the same as the added ones?

KirkBushman avatar Aug 20 '21 18:08 KirkBushman

oh, I see, yeah you can't add models directly like that in the paging controller.

elihart avatar Aug 20 '21 18:08 elihart

Actually, I'm rusty on this, but it does seem like your original method should work fine. It's hard to see why your updated function changes anything.

The exception is in

if (asyncDiffer.itemCount > 0) {
            asyncDiffer.getItem(position.coerceIn(0, asyncDiffer.itemCount - 1))
        }

I believe, which is strange because bounds checks are done, which make it seem like this is a concurrency issue

elihart avatar Aug 20 '21 18:08 elihart

If I remember correctly there was a similar concurrency issue with paging2... We should replicate the fixes in this new integration library

KirkBushman avatar Aug 20 '21 18:08 KirkBushman

@elihart Something similar happened for paging2: https://github.com/airbnb/epoxy/issues/986

Do you want me to make a PR synchronizing that method?

KirkBushman avatar Aug 21 '21 11:08 KirkBushman

I tried to synchronize a few methods, but I started getting an IndexOutOfBoundsException at:

(0 until modelCache.size).forEach { position ->
    if (modelCache[position] == null) {
        modelCache[position] = modelBuilder(position, currentList[position])
    }
}

and in other places in the code. I reverted to using paging2 and PagedList<> for now.

KirkBushman avatar Aug 22 '21 22:08 KirkBushman

I am getting the same crash on v5.1.1. Are there any potential fixes to this issue?

alexanderar avatar Mar 31 '23 17:03 alexanderar

@alexanderar I moved directly from paging2 epoxy to compose, but this problem was never fixed 🫤🫤

KirkBushman avatar Apr 01 '23 17:04 KirkBushman