Fix infinite loadBefore calls

[mwajeeh]

Adjust last position if inserted items are before last position.
Fixes #816 and #621

[elihart]

I'm not convinced by this approach, it doesn't seem to address the root issue, which is "why does telling the paged list to load around an index (which should be already loaded) cause it to insert new data?"

Once we've loaded around a position the data should be inserted, so calling to load at the same index again should be a no-op. This churn points to the page size not being large (which is the key recommendation in #621)

I noticed in your sample code that page size is only 5. This also sets the prefetch distance to 5. I don't know how many items are on your screen, but it seems like the page size is much to small. It should be several times (maybe 10x?) the expected number of items on screen (according to the PagedList.Config javadoc). Have you tried increasing your page size?

Of course, it would be nice to have robust behavior even when config sizes are not set correctly. Given that the infinite loop involves the getModels -> triggerLoadAround path, it may be simplest and cleanest to remove or modify how triggerLoadAround is called from getModels.

I'm not clear on why we need to call triggerLoadAround from there in the first place. It seems unnecessary, but may be needed for some edge case of loading data. I would recommend looking into figuring out why/when that is needed, and possibly removing it or limiting when it is called.

[elihart]

what if it + count is greater than position? you compare these two but don't look at the value of count

[mwajeeh]

Why does it matter if it + count is greater than position?
If lastPosition was 5 and we just inserted 50 items before 5th position e.g. onInserted(position = 0, count = 50) then your "lastPosition" MUST be adjusted to 50 + 5.

[elihart]

lastPosition is the last index that was bound, not the last index that was added. They are too different things, and using the same property to represent two different things is confusing and dangerous.

[mwajeeh]

lastPosition can be renamed to nextLoadAroundPosition. When we insert new data in the list before this position, this variable no longer holds true. so renaming it may help. This way nextLoadAroundPosition acts like a command argument that should be used in next call to loadAround. This is exactly how it is being used now btw.

[mwajeeh]

"why does telling the paged list to load around an index (which should be already loaded) cause it to insert new data?"

@elihart Lets assume that prefetchDistance = 10

LoadAfter Case:
Now if you invoke loadAround(20) and list size is 25, because (loadAround + prefetchDistance + 1 - listSize) > 0, ContiguousPagedList will load next page. After loading next 10 items loadAround(20) will be a no op because (20 + 10 + 1 - 35) < 0. So calling to load at the same index again should be a no-op in this case, which is not true for loadBefore case.
See androidx.paging.ContiguousPagedList#getAppendItemsRequested() for these calculations.

LoadBefore Case:
If you invoke loadAround(5) no matter how large your list is, because (prefetchDistance - index) > 0, ContiguousPagedList will load previous page. After loading previous 10 items loadAround(5) will cause loadBefore to be invoked again (10 - 5) > 0. So calling to load at the same index again will never be a no-op in this case. It will go in infinite loop.
See androidx.paging.ContiguousPagedList#getPrependItemsRequested().

This is exactly what is happening here. Page size doesn't matter. I have tried it with very large page size as well.

So the only solution is to detect if items are inserted before last loadAround position and adjust for that.

[elihart]

@mwajeeh Thanks for explaining. Two questions:

1. Based on your explanation, and the logic in ContiguousPagedList , loadAround(0) will always load at least prefetchDistance. This seems like a bug, if we are loading around 0 how and why would we prepend data to 0? Is this a problem with the paging library?
2. Even if it is valid to schedule the prepend, why does this result in onInserted being called back? If those items were already loaded, then it seems like onInserted should not be called, since they already existed, and were not truly inserted. In fact, if it was calling back onInserted with non zero count then it would break out model cache, since we do modelCache.add(position, null) for each insertion call - it would grow this cache infinitely if it kept looping - is this the case? Because that would be a bad bug all on its own.

[mwajeeh]

@elihart

1. Paging library supports data loading in both directions. There can be use cases where you want to load more data in either direction of scroll. In my case, its a messaging app which loads earlier messages on scrolling up and prepends them to the list. Let me know if I misunderstood your question here.

2. Those items were not already loaded, thats why onInserted is called. So in this use case, user scrolls up to top, paging library loads new messages around 0, we get new messages from server, prepend them, onInserted gets called. Epoxy lib checks that lastPosition is not null so it invokes loadAround again with 0 index. We fetch more messages, prepend them and this keeps looping. We do insert new data though. It is not calling onInserted with non zero count. So either epoxy lib should not invoke loadAround again or if it does then it should adjust last bound position aka lastPosition accordingly so that ContiguousPagedList will ignore next call to loadAround. I prefer later solution because you are supposed to invoke loadAround as many times as you want and PagesList should be able to take care of it.

The sample code that I have attached is very simple and is almost identical to epoxy paging sample app. Method to reproduce this behaviour is also very easy. Running that sample will clear up lot things and you might find something that I have missed.

[elihart]

I understand data can load in both directions, but at index 0 there should not be any data to load before, so why does the paging library still do so?

Those items were not already loaded, thats why onInserted is called
Your original issue says that there is an infinite loop, where this loop callback keeps happening lastPosition?.let{triggerLoadAround()} -> updateCallback.onInserted() -> requestModelBuild().

Why is this infinite loop possible once data is already loaded? Once data at that position is loaded, the loadAround index will not change and triggerLoadAround at that index should load the same data that was already loaded.

[mwajeeh]

I understand data can load in both directions, but at index 0 there should not be any data to load before, so why does the paging library still do so?

@elihart How can we assume that at local 0th index there is no more data to be loaded?

Let's assume you have a RecyclerView with LinearLayoutManager (stackFromEnd = true). Now let's say you are showing a list of countries using ItemKeyedDataSource with country names as Key. Backend has this list: https://github.com/samayo/country-json/blob/master/src/country-by-name.json.
You fetch last 5 countries initially so you have

0. "Western Sahara"
1. "Yemen"
2. "Yugoslavia"
3. "Zambia"
4. "Zimbabwe"

When user scrolls up, loadAround(0) gets called because ContiguousPagedList detects that there is no more data before 0 so -> loadBefore('Western Sahara'). You fetch more items from backend and prepend items. Your list is now:

0. "Vietnam"
1. "Virgin Islands, British"
2. "Virgin Islands, U.S."
3. "Wales"
4. "Wallis and Futuna"
5. "Western Sahara"
6. "Yemen"
7. "Yugoslavia"
8. "Zambia"
9. "Zimbabwe"

Now ideally because user hasn't reached to the top yet, epoxy shouldn't be invoking loadAround(0) and as we can see that previous loadAroundPosition is now 5. So at best epoxy should invoke loadAround(5), if it has to in onInserted() which will now be a no-op. But it invokes loadAround(0), ContiguousPagedList detects there is no more data before 0, causes loadBefore('Vietnam') to be invoked. We load more data and hence the loop...

[tusacheva-flo]

We faced exactly same issue and it's quite critical for us. @elihart could please clarify if there is any decision about the fix? Is it going to be approved and merged soon?

[tusacheva-flo]

@elihart @mwajeeh any updates on it?

[mwajeeh]

@tusacheva-flo I ended up changing my approach to pagination after this issue. I am now using PagedList.BoundaryCallback along with ItemKeyedDataSource which is working fine for me. For now you can clone this repo and add this change locally until @elihart has any update.

[agarciaz-dev]

@mwajeeh @elihart Hi guys! Is there any update regarding the fix for this issue... or a temporary solution? any help would be really appreciated.. thanks