Reproducible: ItemKeyedDataSource's loadBefore gets called infinitely

[mwajeeh]

There is a similar issue here #621
I just attempted to convert custom two way paging mechanism to epoxy paging library and this issue is making it impossible. I am attaching the code which can easily reproduce this. PagedListModelCache relies on lastPosition to trigger loadAround() and I think thats where the bug is.

How to reproduce?
When you launch this activity, you will see that first page keeps loading which is expected. Now if you scroll down 1 or 2 pages and then scroll up just enough to trigger loadBefore() then you will see that it will still go in infinite loop invoking loadBefore(). This is because lastPosition is stored indefinitely in PagedListModelCache.

updateCallback.onInserted() -> requestModelBuild()-> modelCache.getModels() -> lastPosition?.let{triggerLoadAround()} -> updateCallback.onInserted()

Following is modified code from epoxy paging sample that can easily produce this issue: https://github.com/airbnb/epoxy/tree/master/epoxy-pagingsample

import android.app.Application
import android.content.Context
import android.os.Bundle
import android.util.Log
import androidx.appcompat.app.AppCompatActivity
import androidx.appcompat.widget.AppCompatTextView
import androidx.lifecycle.AndroidViewModel
import androidx.lifecycle.LiveData
import androidx.lifecycle.MutableLiveData
import androidx.lifecycle.Observer
import androidx.lifecycle.ViewModelProviders
import androidx.paging.DataSource
import androidx.paging.ItemKeyedDataSource
import androidx.paging.LivePagedListBuilder
import androidx.paging.PagedList
import androidx.recyclerview.widget.LinearLayoutManager
import androidx.recyclerview.widget.RecyclerView
import androidx.room.Room
import com.airbnb.epoxy.EpoxyModel
import com.airbnb.epoxy.ModelView
import com.airbnb.epoxy.TextProp
import com.airbnb.epoxy.paging.PagedListEpoxyController
import java.util.UUID
import java.util.concurrent.Executors

class PagingNetworkSampleActivity : AppCompatActivity() {
    public override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_main)
        val pagingController = TestController()
        val recyclerView = findViewById<RecyclerView>(R.id.recycler_view)
        recyclerView.layoutManager = LinearLayoutManager(this)
        recyclerView.adapter = pagingController.adapter

        val viewModel = ViewModelProviders.of(this).get(ActivityViewModel::class.java)
        viewModel.pagedList.observe(this, Observer {
            pagingController.submitList(it)
        })
    }
}

class TestController : PagedListEpoxyController<NetworkUser>() {
    override fun buildItemModel(currentPosition: Int, item: NetworkUser?): EpoxyModel<*> {
        return if (item == null) {
            PagingViewModel_()
                .id(-currentPosition)
                .name("loading $currentPosition")
        } else {
            PagingViewModel_()
                .id(item.uid)
                .name("${item.uid}: ${item.firstName} / ${item.lastName}")
        }
    }

    override fun addModels(models: List<EpoxyModel<*>>) {
        pagingView {
            id("header")
            name("showing ${models.size} items")
        }
        super.addModels(models)
    }

    init {
        isDebugLoggingEnabled = true
    }

    override fun onExceptionSwallowed(exception: RuntimeException) {
        throw exception
    }
}

@ModelView(autoLayout = ModelView.Size.MATCH_WIDTH_WRAP_HEIGHT)
class PagingView(context: Context) : AppCompatTextView(context) {

    @TextProp
    fun name(name: CharSequence) {
        text = name
    }
}

class ActivityViewModel(app: Application) : AndroidViewModel(app) {
    val db by lazy {
        Room.inMemoryDatabaseBuilder(app, PagingDatabase::class.java).build()
    }
    val pagedList: LiveData<PagedList<NetworkUser>> by lazy {
        LivePagedListBuilder<String, NetworkUser>(NetworkDataSource.Factory(), 5)
            .setFetchExecutor(Executors.newFixedThreadPool(2))
            .build()

    }
}

data class NetworkUser(
    var uid: String,

    var firstName: String = "first name $uid",

    var lastName: String = "last name $uid"
)

class NetworkDataSource : ItemKeyedDataSource<String, NetworkUser>() {
    override fun loadInitial(
        params: LoadInitialParams<String>,
        callback: LoadInitialCallback<NetworkUser>
    ) {
        Thread.sleep(1000)
        callback.onResult((0..5).map { NetworkUser(UUID.randomUUID().toString()) })
    }

    override fun loadAfter(params: LoadParams<String>, callback: LoadCallback<NetworkUser>) {
        Log.d(">>", "loadAfter called")
        Thread.sleep(1000)
        callback.onResult((0..5).map { NetworkUser(UUID.randomUUID().toString()) })
    }

    override fun loadBefore(params: LoadParams<String>, callback: LoadCallback<NetworkUser>) {
        Log.d(">>", "loadBefore called")
        Thread.sleep(1000)
        callback.onResult((0..5).map { NetworkUser(UUID.randomUUID().toString()) })
    }

    override fun getKey(item: NetworkUser): String = item.uid

    class Factory : DataSource.Factory<String, NetworkUser>() {
        private val sourceLiveData = MutableLiveData<NetworkDataSource>()
        override fun create(): DataSource<String, NetworkUser> {
            val source = NetworkDataSource()
            sourceLiveData.postValue(source)
            return source
        }
    }
}

[mwajeeh]

Following implementation of ListUpdateCallback for PagedListModelCache.updateCallback seems to fix the issue:
It adjusts the lastPosition if newly inserted data is above that lastPosition.

        @Synchronized
        override fun onInserted(position: Int, count: Int) {
            assertUpdateCallbacksAllowed()
            (0 until count).forEach {
                modelCache.add(position, null)
            } 
            //adjust lastPosition
            lastPosition?.let {
                if (position < it) {
                    lastPosition = it + count
                }
            }
            rebuildCallback()
        }

[mwajeeh]

Any updates on fix?

[elihart]

No update, feel free to submit a PR to fix it.

Does this need to be a separate issue from #621?

[mwajeeh]

Both issue seem same to me.

[elihart]

Closing because #621 was opened first, move communication there