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 using Paging3 #1220

Open
KirkBushman opened this issue Aug 15, 2021 · 11 comments
Open

IndexOutOfBoundsException using Paging3 #1220

KirkBushman opened this issue Aug 15, 2021 · 11 comments

Comments

@KirkBushman
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

elihart commented Aug 20, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@elihart
Copy link
Contributor

elihart commented Aug 20, 2021

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

@elihart
Copy link
Contributor

elihart commented Aug 20, 2021

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
Copy link
Contributor Author

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

@KirkBushman
Copy link
Contributor Author

@elihart
Something similar happened for paging2:
#986

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

@KirkBushman
Copy link
Contributor Author

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
Copy link

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

@KirkBushman
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants