airbnb / epoxy

Epoxy is an Android library for building complex screens in a RecyclerView
https://goo.gl/eIK82p
Apache License 2.0
8.51k stars 728 forks source link

IndexOutOfBoundsException using Paging3 #1220

Open KirkBushman opened 3 years ago

KirkBushman commented 3 years ago

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 commented 3 years ago

@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?

elihart commented 3 years ago

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

KirkBushman commented 3 years ago

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 commented 3 years ago

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

elihart commented 3 years ago

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

elihart commented 3 years ago

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

KirkBushman commented 3 years ago

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

KirkBushman commented 3 years ago

@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 commented 3 years ago

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.

alexanderar commented 1 year ago

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

KirkBushman commented 1 year ago

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