Synchronize paged epoxy controller #656
Conversation
| } | ||
| // we have to reply on this private API, otherwise, paged list might be changed when models are being built, | ||
| // potentially creating concurrent modification problems. | ||
| builder.setMainThreadExecutor {runnable : Runnable -> |
There was a problem hiding this comment.
this should be doing the thread sync.
There was a problem hiding this comment.
@yigit right, I understand that part (apologies for the indentation change from auto-format, it makes this hard to read, but most of the lines are the same.)
But as far as I can tell, this only guarantees synchronization when diffing a new paged list.
the main thing I'm trying to address with the synchronization is changes to the PagedList that get notified to the cache via the paged list's notifyExecutor (since the AsyncPagedListDiffer uses PagedList#addWeakCallback to register mPagedListCallback)
You've said before that having people set their PagedList notifyExecutor to the same thread as model building would fix this, but that seems very fragile to me, and easy for people to miss.
Is there anything wrong with forcibly synchronizing the cache access across threads so we don't have to worry about the notifyExecutor?
elihart
force-pushed
the
eli-thread_safe_model_cache_clear
branch
from
d7a9dcb
to
f7d463d
Compare
|
@yigit could you take a look at this new approach when you get a chance? |
| * use the model cache (to avoid data synchronization issues), but attempt to fill the cache with | ||
| * the models later. | ||
| * | ||
| * 2. When a list is submitted it can trigger update callbacks synchronously. Since we don't control |
There was a problem hiding this comment.
this looks like a paging bug. we should not be notifying on another thread. though paging receives just an executor so probably paging is unable to check that :/.
| * We can't force this to happen, and must instead rely on user's configuration, but we can alert | ||
| * when it is not configured correctly. | ||
| * | ||
| * An exception is if the callback happens due to a new paged list being submitted, which can |
| * Set to true while a new list is being submitted, so that we can ignore the update callback | ||
| * thread restriction. | ||
| */ | ||
| private var inSubmitList: Boolean = false |
There was a problem hiding this comment.
there is a race condition where submit might be called in thread A at the same time we are checking this value. Needs to be thread local
There was a problem hiding this comment.
I think the @Synchronized prevent that from happening, since all access to this variable is behind synchronized functions
| // the UI can be ready immediately. To avoid concurrent modification issues with the PagedList | ||
| // and model cache we can't allow that first build to touch the cache. | ||
| if (Looper.myLooper() != modelBuildingHandler.looper) { | ||
| val initialModels = currentList.mapIndexed { position, item -> |
There was a problem hiding this comment.
hmm we may end up building more than necessary (or too much) but maybe it is OK since paged list wont load too much.
There was a problem hiding this comment.
yes, it's a bit unfortunate, but I don't see a way around this. In most cases the models should be able to be reused I think
| } | ||
|
|
||
| (0 until modelCache.size).forEach { position -> | ||
| if (modelCache[position] != null) { |
There was a problem hiding this comment.
not sure maybe i wrot ethis code but i think it is easier to read if we just say
if (modelCache[position] == null) {
// call build
}
to avoid return@foreach
or if we want to get kotlin fancy, we can do:
modelCache[position] ?: modelBuilder(position, list[position].also { modelCache[position] = it}
elihart
force-pushed
the
eli-thread_safe_model_cache_clear
branch
from
f7d463d
to
2a99f2a
Compare
|
Thanks for the review yigit - I think this is pretty good to go, sorry it is uglier than your original version, but I hope it fixes problems for people. I noticed that the paging sample you setup only does model building on the main thread, and the LivePagedListBuild doesn't seem to have a way to change the notify executor, which means we can't have a sample where the model building happens asynchronously. Since that is probably a very common use case for people and an important thing to test it would be nice to enable that, but I couldn't figure out a great way to do it (I don't have much experience with these arch components) |
|
I’d be great to release these changes as there are many folks, myself included, running into issues analogous to the ones previously reported. |
elihart
force-pushed
the
eli-thread_safe_model_cache_clear
branch
from
2a99f2a
to
5174ad4
Compare
|
@PedroAlvarado working on it - seems to be a compatibility issue with kotlin 1.3.0+ |
There have been a slew of issues with the paged controller, like NPE's and duplicate items, which point to concurrency issues. We have previously relied on model building and the paged controller's model cache using the same thread to prevent this, but it doesn't seem to be working completely.
One issue is that updates to the PagedList get dispatched on it's
notifyExecutor, which is outside of the control of Epoxy, and we were relying on documentation to tell people to make sure this was the same thread as the model building thread. This doesn't seem like a scalable approach.Instead, I have synchronized all access to the model cache