AsyncListDiffer not always committing changes on the Main thread, sometimes causing a CalledFromWrongThreadException [232464455]

Assigned

Bug

Status Update

No update yet.

Description

ti...@gmail.com

created issue #1

May 13, 2022 04:43AM

Component used: ListAdapter, along with a RecyclerView Version used: 1.2.1 Devices/Android versions reproduced on: All of them.-

Recently got the following crash due to submitting a list on a ListAdapter from a background thread:

E/AndroidRuntime: FATAL EXCEPTION: DefaultDispatcher-worker-3 @coroutine#8817 android.view.ViewRootImpl$CalledFromWrongThreadException: Only the original thread that created a view hierarchy can touch its views. at android.view.ViewRootImpl.checkThread(ViewRootImpl.java:11131) at android.view.ViewRootImpl.requestLayout(ViewRootImpl.java:2456) at android.view.View.requestLayout(View.java:27048) at android.view.View.requestLayout(View.java:27048) at android.view.View.requestLayout(View.java:27048) at android.view.View.requestLayout(View.java:27048) at android.view.View.requestLayout(View.java:27048) at android.view.View.requestLayout(View.java:27048) at android.widget.RelativeLayout.requestLayout(RelativeLayout.java:380) at android.view.View.requestLayout(View.java:27048) at android.widget.RelativeLayout.requestLayout(RelativeLayout.java:380) at android.view.View.requestLayout(View.java:27048) at android.view.View.requestLayout(View.java:27048) at android.widget.RelativeLayout.requestLayout(RelativeLayout.java:380) at android.view.View.requestLayout(View.java:27048) at android.view.View.requestLayout(View.java:27048) at android.widget.RelativeLayout.requestLayout(RelativeLayout.java:380) at android.view.View.requestLayout(View.java:27048) at androidx.recyclerview.widget.RecyclerView.requestLayout(RecyclerView.java:4586) at androidx.recyclerview.widget.RecyclerView$RecyclerViewDataObserver.triggerUpdateProcessor(RecyclerView.java:5747) at androidx.recyclerview.widget.RecyclerView$RecyclerViewDataObserver.onItemRangeInserted(RecyclerView.java:5722) at androidx.recyclerview.widget.RecyclerView$AdapterDataObservable.notifyItemRangeInserted(RecyclerView.java:12684) at androidx.recyclerview.widget.RecyclerView$Adapter.notifyItemRangeInserted(RecyclerView.java:7722) at androidx.recyclerview.widget.AdapterListUpdateCallback.onInserted(AdapterListUpdateCallback.java:42) at androidx.recyclerview.widget.AsyncListDiffer.submitList(AsyncListDiffer.java:283) at androidx.recyclerview.widget.ListAdapter.submitList(ListAdapter.java:146)

Seems like the problem is on the AsyncListDiffer class, in the submitList(@Nullable final List<T> newList, @Nullable final Runnable commitCallback) method. This method does a good job at computing list changes in a background thread, and then committing the result in the main thread. But if the newList argument is null, or the mList attribute on the class is null, no diff computing happens (as expected), and the result is commiting on the thread of the caller (not expected). The result should be committed on the main thread for both of this cases, so the method can be considered thread safe.

Current implementation of the method is:

    public void submitList(@Nullable final List<T> newList,
            @Nullable final Runnable commitCallback) {
        // incrementing generation means any currently-running diffs are discarded when they finish
        final int runGeneration = ++mMaxScheduledGeneration;
        if (newList == mList) {
            // nothing to do (Note - still had to inc generation, since may have ongoing work)
            if (commitCallback != null) {
                commitCallback.run();
            }
            return;
        }
        final List<T> previousList = mReadOnlyList;
        // fast simple remove all
        if (newList == null) {
            //noinspection ConstantConditions
            int countRemoved = mList.size();
            mList = null;
            mReadOnlyList = Collections.emptyList();
            // notify last, after list is updated
            mUpdateCallback.onRemoved(0, countRemoved);
            onCurrentListChanged(previousList, commitCallback);
            return;
        }
        // fast simple first insert
        if (mList == null) {
            mList = newList;
            mReadOnlyList = Collections.unmodifiableList(newList);
            // notify last, after list is updated
            mUpdateCallback.onInserted(0, newList.size());
            onCurrentListChanged(previousList, commitCallback);
            return;
        }
        final List<T> oldList = mList;
        mConfig.getBackgroundThreadExecutor().execute(new Runnable() {
            @Override
            public void run() {
                final DiffUtil.DiffResult result = DiffUtil.calculateDiff(new DiffUtil.Callback() {
                    @Override
                    public int getOldListSize() {
                        return oldList.size();
                    }
                    @Override
                    public int getNewListSize() {
                        return newList.size();
                    }
                    @Override
                    public boolean areItemsTheSame(int oldItemPosition, int newItemPosition) {
                        T oldItem = oldList.get(oldItemPosition);
                        T newItem = newList.get(newItemPosition);
                        if (oldItem != null && newItem != null) {
                            return mConfig.getDiffCallback().areItemsTheSame(oldItem, newItem);
                        }
                        // If both items are null we consider them the same.
                        return oldItem == null && newItem == null;
                    }
                    @Override
                    public boolean areContentsTheSame(int oldItemPosition, int newItemPosition) {
                        T oldItem = oldList.get(oldItemPosition);
                        T newItem = newList.get(newItemPosition);
                        if (oldItem != null && newItem != null) {
                            return mConfig.getDiffCallback().areContentsTheSame(oldItem, newItem);
                        }
                        if (oldItem == null && newItem == null) {
                            return true;
                        }
                        // There is an implementation bug if we reach this point. Per the docs, this
                        // method should only be invoked when areItemsTheSame returns true. That
                        // only occurs when both items are non-null or both are null and both of
                        // those cases are handled above.
                        throw new AssertionError();
                    }
                    @Nullable
                    @Override
                    public Object getChangePayload(int oldItemPosition, int newItemPosition) {
                        T oldItem = oldList.get(oldItemPosition);
                        T newItem = newList.get(newItemPosition);
                        if (oldItem != null && newItem != null) {
                            return mConfig.getDiffCallback().getChangePayload(oldItem, newItem);
                        }
                        // There is an implementation bug if we reach this point. Per the docs, this
                        // method should only be invoked when areItemsTheSame returns true AND
                        // areContentsTheSame returns false. That only occurs when both items are
                        // non-null which is the only case handled above.
                        throw new AssertionError();
                    }
                });
                mMainThreadExecutor.execute(new Runnable() {
                    @Override
                    public void run() {
                        if (mMaxScheduledGeneration == runGeneration) {
                            latchList(newList, result, commitCallback);
                        }
                    }
                });
            }
        });
    }

The following changes would come to solve the issue:

    public void submitList(@Nullable final List<T> newList,
            @Nullable final Runnable commitCallback) {
        // incrementing generation means any currently-running diffs are discarded when they finish
        final int runGeneration = ++mMaxScheduledGeneration;
        if (newList == mList) {
            // nothing to do (Note - still had to inc generation, since may have ongoing work)
            if (commitCallback != null) {
                commitCallback.run();
            }
            return;
        }
        final List<T> previousList = mReadOnlyList;
        // fast simple remove all
        if (newList == null) {
            //noinspection ConstantConditions
            int countRemoved = mList.size();
            mList = null;
            mReadOnlyList = Collections.emptyList();
            // notify last, after list is updated
            mMainThreadExecutor.execute(new Runnable() {
                @Override
                public void run() {
                    if (mMaxScheduledGeneration == runGeneration) {
                        mUpdateCallback.onRemoved(0, countRemoved);
                    }
                }
            });
            onCurrentListChanged(previousList, commitCallback);
            return;
        }
        // fast simple first insert
        if (mList == null) {
            mList = newList;
            mReadOnlyList = Collections.unmodifiableList(newList);
            // notify last, after list is updated
            mMainThreadExecutor.execute(new Runnable() {
                @Override
                public void run() {
                    if (mMaxScheduledGeneration == runGeneration) {
                        mUpdateCallback.onInserted(0, newList.size());
                    }
                }
            });
            onCurrentListChanged(previousList, commitCallback);
            return;
        }
        final List<T> oldList = mList;
        mConfig.getBackgroundThreadExecutor().execute(new Runnable() {
            @Override
            public void run() {
                final DiffUtil.DiffResult result = DiffUtil.calculateDiff(new DiffUtil.Callback() {
                    @Override
                    public int getOldListSize() {
                        return oldList.size();
                    }
                    @Override
                    public int getNewListSize() {
                        return newList.size();
                    }
                    @Override
                    public boolean areItemsTheSame(int oldItemPosition, int newItemPosition) {
                        T oldItem = oldList.get(oldItemPosition);
                        T newItem = newList.get(newItemPosition);
                        if (oldItem != null && newItem != null) {
                            return mConfig.getDiffCallback().areItemsTheSame(oldItem, newItem);
                        }
                        // If both items are null we consider them the same.
                        return oldItem == null && newItem == null;
                    }
                    @Override
                    public boolean areContentsTheSame(int oldItemPosition, int newItemPosition) {
                        T oldItem = oldList.get(oldItemPosition);
                        T newItem = newList.get(newItemPosition);
                        if (oldItem != null && newItem != null) {
                            return mConfig.getDiffCallback().areContentsTheSame(oldItem, newItem);
                        }
                        if (oldItem == null && newItem == null) {
                            return true;
                        }
                        // There is an implementation bug if we reach this point. Per the docs, this
                        // method should only be invoked when areItemsTheSame returns true. That
                        // only occurs when both items are non-null or both are null and both of
                        // those cases are handled above.
                        throw new AssertionError();
                    }
                    @Nullable
                    @Override
                    public Object getChangePayload(int oldItemPosition, int newItemPosition) {
                        T oldItem = oldList.get(oldItemPosition);
                        T newItem = newList.get(newItemPosition);
                        if (oldItem != null && newItem != null) {
                            return mConfig.getDiffCallback().getChangePayload(oldItem, newItem);
                        }
                        // There is an implementation bug if we reach this point. Per the docs, this
                        // method should only be invoked when areItemsTheSame returns true AND
                        // areContentsTheSame returns false. That only occurs when both items are
                        // non-null which is the only case handled above.
                        throw new AssertionError();
                    }
                });
                mMainThreadExecutor.execute(new Runnable() {
                    @Override
                    public void run() {
                        if (mMaxScheduledGeneration == runGeneration) {
                            latchList(newList, result, commitCallback);
                        }
                    }
                });
            }
        });
    }

As you can see, the important bit of the change is that when no diff computation is done the mUpdateCallback.onRemoved(0, countRemoved); and mUpdateCallback.onInserted(0, newList.size()); calls are being done on the main thread on the changes I shared on this ticket. I would gladly submit the changes myself, but sadly I don't have the possibility of cloning the repo and doing it. Due to it I'm raising this ticket, with a detailed explanation of what's going on, and how to solve the problem. Hope you find it useful.

Comments

yb...@google.com <yb...@google.com> #2May 13, 2022 05:43AM

We don't support cross-process invalidations yet.

ti...@gmail.com <ti...@gmail.com> #3May 13, 2022 02:03PM

At least , add a "refresh all observables" function to alpha4 ? i am trying to implement it by myself with your InvalidationTracker class

yb...@google.com <yb...@google.com> #4May 13, 2022 04:44PM

Okey, I made it and created a method to invalidate all observables below;

private void refreshAllLiveData() {
AppDataBase YOUR_DATABASE_WHICH_YOU_BUILD = .....
SupportSQLiteDatabase writableDatabase = YOUR_DATABASE_WHICH_YOU_BUILD.getOpenHelper().getWritableDatabase();
//get the database count;
Cursor cursor = writableDatabase.query("SELECT count(*) FROM sqlite_master WHERE type = 'table' AND name != 'android_metadata' AND name != 'room_master_table';");
int tableCount = 0;
while(cursor.moveToNext()) {
tableCount = cursor.getInt(0);
}

for (int i = 0; i < tableCount; i++) {
//update version table with the incremented count because room modification log stores tables with ids instead of names
writableDatabase.execSQL("INSERT OR REPLACE INTO room_table_modification_log VALUES(null, "+(i)+")");
}
YOUR_DATABASE_WHICH_YOU_BUILD.getInvalidationTracker().refreshVersionsAsync();
}

-----

This is a workaroud for refreshing all live datas, I still prefer to use a proper API you implemented.

Thanx

ti...@gmail.com <ti...@gmail.com> #5May 13, 2022 05:21PM

Cross process invalidation is now support in Room 2.1.0 with the enableMultiInstanceInvalidation() API.

yb...@google.com <yb...@google.com> #6May 13, 2022 05:25PM

In fact, the current implementation (without my fixes) doesn't honor the rule that says that on multiple submittions only the last one should be committed.

Is this only when called from different thread or even when called from the main thread? I might be missing something but I'm not sure how that will happen unless this method is called from multiple threads (which is not supported).

ti...@gmail.com <ti...@gmail.com> #7May 13, 2022 05:36PM

Even when called from the main thread. Let me put a simple scenario so you can better understand what's going on" 1 - I call submitList with a list containint 5 elements. (Computing will start, and the result will be queued to be committed or not on the main thread in the future) 2 - I call submitList with an empty list. This will be committed ASAP (wrong) 3 - Right away I submitList with a list containint 10 elements. (Computing will start, and the result will be queued to be committed or not on the main thread in the future)

Listo on step number 2 should or should have not been committed, due to list 3 kicking in. But due to the unconditional nature of the call in the block that handles newList being null, it'll be committed no matter what.

Calling submitList from a thread other than the main thread only makes this worse... Also, submitList does operate with collections (outside of the diff computing)... Forcing this on the main thread is bad. Submittions should be thread safe

Another thing to add is that you don't really need complex locking mechanisms to synchronize the commit operation. If you just make sure any and all commit operations are queued on the main thread, and you make sure you check for the mMaxScheduledGeneration counter before executing, things will happen the right way. The fact that there's only one Main thread per application means there's no parallelism, so commits will happen in the order they were queued. The mMaxScheduledGeneration check allows to not execute old (not needed operations). This is a good example of how limited parallelism can sometimes help control concurrent access to resources.

Message last modified on May 13, 2022 05:44PM

yb...@google.com <yb...@google.com> #8May 13, 2022 05:52PM

So to better understand: You list is at ListVersion0 (ListV0) You call

submitList(listV1)// 5 items) - async computing

submitList(listV2)// 0 items) - immediate

submitList(listV3)// 10 items) - async computing

Not sure why you think listV2 shouldn't be committed immediately. submitList do not guarantee that all intermediate lists are applied, it will commit V2 immediately and cancel V1 (when it arrives, it will realize there was another call after).

And when listV3's computation comes back, it will override listV2. So your visible list goes from V0 -> V2 -> V3. That is working as intended.

As for "we could simply hop onto the main thread" argument, it is not very straightforward.

If you call submitList from ThreadA and ThreadB, even if your code to call submitList from ThreadA was executed before the call from ThreadB, the code running inside has no guarantees to be executed first A then B (since they are in different threads). In theory, you could call ThreadA first but the main thread object could be enqueued after ThreadB's submitList call. The only want to ensure ordering there is to add a sync barrier.

Furthermore, even when you are making these calls from a background thread, you will have to ensure you are calling them in the right order, hence you'll need some locking anyways, which we get from forcing on the main thread in the first place. e.g. you still needed to make sure you never call submitList before the previous call to submitList returned, otherwise, there will be no guarantees on which one executes first.

Does that makes sense?

au...@gmail.com <au...@gmail.com> #9Aug 21, 2024 12:51PM

Dominate the battlefield with Master Multiplayer 2 Mod APK, unlocking mod apk unlimited money resources and powerful upgrades. Experience enhanced gameplay and lead your team to victory with ease. For more information you can visit

https://greenapk.me/parking-master-multiplayer-2-mod-apk/