Fatal Exception: java.util.ConcurrentModificationException while adding sections

[erenbakac]

Hi,

I use your library for my app's main screen. I get data from our server with asynctask(via okhttp) and i add sections in this tasks. I update adapter using either notifyDataSetChanged or notifyItemRangeInserted. But sometimes i get following exception. Can you suggest me anything to fix this problem? Thank you.

Fatal Exception: java.util.ConcurrentModificationException at java.util.HashMap$HashIterator.nextEntry(HashMap.java:787) at java.util.HashMap$EntryIterator.next(HashMap.java:824) at java.util.HashMap$EntryIterator.next(HashMap.java:824) at io.github.luizgrp.sectionedrecyclerviewadapter.SectionedRecyclerViewAdapter.onCreateViewHolder(SectionedRecyclerViewAdapter.java:43) at android.support.v7.widget.RecyclerView$Adapter.createViewHolder(RecyclerView.java:6367) at android.support.v7.widget.RecyclerView$Recycler.tryGetViewHolderForPositionByDeadline(RecyclerView.java:5555) at android.support.v7.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:5440) at android.support.v7.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:5436) at android.support.v7.widget.LinearLayoutManager$LayoutState.next(LinearLayoutManager.java:2224) at android.support.v7.widget.GridLayoutManager.layoutChunk(GridLayoutManager.java:556) at android.support.v7.widget.LinearLayoutManager.fill(LinearLayoutManager.java:1511) at android.support.v7.widget.LinearLayoutManager.onLayoutChildren(LinearLayoutManager.java:595) at android.support.v7.widget.GridLayoutManager.onLayoutChildren(GridLayoutManager.java:170) at android.support.v7.widget.RecyclerView.dispatchLayoutStep2(RecyclerView.java:3583) at android.support.v7.widget.RecyclerView.dispatchLayout(RecyclerView.java:3312) at android.support.v7.widget.RecyclerView.onLayout(RecyclerView.java:3844) at android.view.View.layout(View.java:17050) at android.view.ViewGroup.layout(ViewGroup.java:5579) at android.support.v4.widget.SwipeRefreshLayout.onLayout(SwipeRefreshLayout.java:636) at android.view.View.layout(View.java:17050) at android.view.ViewGroup.layout(ViewGroup.java:5579) at android.widget.RelativeLayout.onLayout(RelativeLayout.java:1189) at android.view.View.layout(View.java:17050) at android.view.ViewGroup.layout(ViewGroup.java:5579) at android.widget.FrameLayout.layoutChildren(FrameLayout.java:396) at android.widget.FrameLayout.onLayout(FrameLayout.java:333) at android.view.View.layout(View.java:17050) at android.view.ViewGroup.layout(ViewGroup.java:5579) at android.widget.RelativeLayout.onLayout(RelativeLayout.java:1189) at android.view.View.layout(View.java:17050) at android.view.ViewGroup.layout(ViewGroup.java:5579) at android.widget.FrameLayout.layoutChildren(FrameLayout.java:396) at android.widget.FrameLayout.onLayout(FrameLayout.java:333) at android.view.View.layout(View.java:17050) at android.view.ViewGroup.layout(ViewGroup.java:5579) at android.widget.LinearLayout.setChildFrame(LinearLayout.java:2001) at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1844) at android.widget.LinearLayout.onLayout(LinearLayout.java:1753) at android.view.View.layout(View.java:17050) at android.view.ViewGroup.layout(ViewGroup.java:5579) at android.widget.FrameLayout.layoutChildren(FrameLayout.java:396) at android.widget.FrameLayout.onLayout(FrameLayout.java:333) at android.view.View.layout(View.java:17050) at android.view.ViewGroup.layout(ViewGroup.java:5579) at android.widget.LinearLayout.setChildFrame(LinearLayout.java:2001) at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1844) at android.widget.LinearLayout.onLayout(LinearLayout.java:1753) at android.view.View.layout(View.java:17050) at android.view.ViewGroup.layout(ViewGroup.java:5579) at android.widget.FrameLayout.layoutChildren(FrameLayout.java:396) at android.widget.FrameLayout.onLayout(FrameLayout.java:333) at com.android.internal.policy.PhoneWindow$DecorView.onLayout(PhoneWindow.java:2794) at android.view.View.layout(View.java:17050) at android.view.ViewGroup.layout(ViewGroup.java:5579) at android.view.ViewRootImpl.performLayout(ViewRootImpl.java:2568) at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:2268) at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:1335) at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:6804) at android.view.Choreographer$CallbackRecord.run(Choreographer.java:894) at android.view.Choreographer.doCallbacks(Choreographer.java:696) at android.view.Choreographer.doFrame(Choreographer.java:631) at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:880) at android.os.Handler.handleCallback(Handler.java:815) at android.os.Handler.dispatchMessage(Handler.java:104) at android.os.Looper.loop(Looper.java:207) at android.app.ActivityThread.main(ActivityThread.java:5902) at java.lang.reflect.Method.invoke(Method.java) at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:948) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:809)

[luizgrp]

I will take a look, try to reproduce the issue and get back to you!

[yccheok]

@erenbakac

May I know, do you perform SectionedRecyclerViewAdapter.addSection in user thread or UI thread?

[erenbakac]

in UI Thread

[yccheok]

As, my initial guess is that, ConcurrentModificationException will happen when

UI thread is perform iteration in onCreateViewHolder

for (Map.Entry<String, Integer> entry : sectionViewTypeNumbers.entrySet()) {

and another user thread is performing sectionViewTypeNumbers modification via addSection

this.sectionViewTypeNumbers.put(tag, viewTypeCount);

But, since you mention addSection is performed by UI thread, my initial guess is no longer valid.

If you can post your code snippet on addSection usage, that might help to figure out the root cause.

[luizgrp]

Hi @erenbakac,

I'm working on improving thread-safety and will soon release a new version that will hopefully address this issue.

[yccheok]

Hi @luizgrp

May I know regarding your plan on thread-safety, is it changing sectionViewTypeNumbers to ConcurrentHashMap?

[luizgrp]

Hey @yccheok,

ConcurretHashMap couldn't be used as we need to preserve the order that the sections were added.

I changed sections and sectionViewTypeNumbers to be created with synchronizeMap and syncronized the code to get a copy of the map before every iteration. You can check the commit here. Please let me know your thoughts.

[yccheok]

Thanks. I will take a look on them.

I have a different thought on this. I feel that we shouldn't promise thread safety in SectionedRecyclerViewAdapter. As I feel that it is very difficult to achieve thread safety correctness, unless it is the main goal of the project.

Also, introduce thread safetyness in SectionedRecyclerViewAdapter, will have possible impact on performance? For developer who is using this library in thread safety manner, or thread safety isn't their concern, this seems like a penalty to them.

I fee that we should either let developer takes responsible of the thread safety-ness.

Or, maybe we can provide an additional wrapper (Like SectionedRecyclerViewAdapter adapter = synchronizedAdapter(sectionedRecyclerViewAdapter), similar to Collections.synchronizedMap(new HashMap());)

In the wrapper, the wrapper will wrap out every public function in SectionedRecyclerViewAdapter. For optimisation, instead of using synchronized, we can use reader/writer lock. For instance, reader lock for onBindViewHolder. writer lock for removeSection

[erenbakac]

Hi @luizgrp

Have you fixed this?

Thank you

[luizgrp]

Hi @yccheok,

Thanks for your input, they are valid concerns.

Yes, there will be a possible impact on performance.

Could you please provide an example of how the develop can make it thread safe for the scenario described by @erenbakac ?

Also, could you please provide a branch with your suggestion of a wrapper so we can see how it works?

Thanks!

[yccheok]

Hi @luizgrp

Please give me some time. I will draft out a proof-of-concept. Objective is to have 0 impact on existing users, yet provide thread safety option for new users who are having such requirement.

Hope I can do it in 1 or 2 weeks time.

Thanks.

[yccheok]

Hi @luizgrp

Here's my proposal - https://github.com/yccheok/SectionedRecyclerViewAdapter/commit/3e09d90000f90c1ffa3dde150366b5a7f667aec3

1. synchronized around some notify... methods might seem unnecessary. But, I do not want to make any assumption on their current (and future) implementation details. That's why my rule of thumb is to synchronized every public methods.

2. Beside

   public static SectionedRecyclerViewAdapter synchronizedSectionedRecyclerViewAdapter(SectionedRecyclerViewAdapter sectionedRecyclerViewAdapter)

We might also provide

public SectionedRecyclerViewAdapter synchronizedSectionedRecyclerViewAdapter()

But, I prefer static version, as most developers already familiar with Collections.synchronizedList. Using similar API style as standard Java will create less confusion.

3. Possible improvement is using ReadWriteLock (Apply write lock on write method, and read lock on read method). But, I prefer not to jump into that stage in this moment.

Hope you will like my proposal. Thank you.

[luizgrp]

Hi @yccheok,

Thanks for your proposal. I think the way you are suggesting would work as well, but it would degrade way more the performance than my proposal. The reason is that it's synchronising loads of methods and synchronising all their code instead of just the necessary parts of the code. For example, in my commit, I don't sync the whole onBindViewHolder method but just the part where it's necessary to obtain a copy of the sections map.

Another problem that I see with your proposal is that inside onCreateViewHolder we call alien methods like Section.getItemViewHolder. The problem of calling alien methods in synchronized blocks is that we don't have control of them and they can cause deadlocks.

I think my proposal would definitely degrade the performance but it can be minimal. Especially on your case when not using in a multi-thread context. There is a good discussion about it here.

If I don't implement my proposal, I won't provide your proposal either, I will let the users create the wrapper so they wrap only the methods that they need, in order to have less impact.

Let me know your thoughts.

[luizgrp]

@erenbakac Meanwhile, you can test if the problem gets fixed using this snapshot of the library: https://jitpack.io/#luizgrp/SectionedRecyclerViewAdapter/v1.2.0-RC1-SNAPSHOT Please check "How to" section, and note that you will have to add jitpack to your repositories.

@yccheok you can also check the performance of your app using this snapshot?

[yccheok]

Another problem that I see with your proposal is that inside onCreateViewHolder we call alien methods like Section.getItemViewHolder. The problem of calling alien methods in synchronized blocks is that we don't have control of them and they can cause deadlocks.

Sorry. I can't imagine how deadlock can happen as the lock is performed on private monitor (Not this). Can you provide an example?

I think my proposal would definitely degrade the performance but it can be minimal. Especially on your case when not using in a multi-thread context. There is a good discussion about it here.

I think I can slightly improve the performance, by using reader/writer lock.

When using synchronizedSectionedRecyclerViewAdapter(), user should fully aware they are going to use it in multi-thread context and will suffer performance penalty.

This is same analogy, for developer to choose using Collections.synchronizedList(new ArrayList<>()) over new ArrayList<>(). They know they are going to gain some, and loss some.

If they choose to use Collections.synchronizedList(new ArrayList<>()) when thread safety is not a concern, and suffer performance penalty, they are solely responsible for that (Not Josh Bloch) :)

If I don't implement my proposal, I won't provide your proposal either, I will let the users create the wrapper so they wrap only the methods that they need, in order to have less impact.

I'm totally fine with that :)

Most collection classes or adapter classes designed for general use case (ArrayAdapter, ArrayList, ...), will not promised thread safety feature. I think once we promise SectionedRecyclerViewAdapter is a thread safe class, the consequence will be

1. How can we ensure the thread safety correctness?
2. How can we unit test thread safety feature?
3. How can we ensure we didn't break thread safety correctness in the future, when adding new features?
4. Is it fair to user who doesn't have thread safety concern?

From https://stackoverflow.com/a/6244952/72437 , it seem like developer shall hold responsible, to ensure they are using adapter in UI thread.

I still hold suspicious that java.util.ConcurrentModificationException happens because the user is calling SectionedRecyclerViewAdapter.addSection (Or removeSection) from user thread. If not, I can't think of other reason. Do you have any guess what is the actual root cause for this exception?

Maybe, we can follow the thread checking as in notifyDataSetChanged - https://stackoverflow.com/questions/8665676/how-to-use-notifydatasetchanged-in-thread (Throw runtime exception when user calls it from non UI thread)

@yccheok you can also check the performance of your app using this snapshot? OK. I will test.

[luizgrp]

Hi @yccheok,

Many thanks for your reply. I will answer one question for now, I need some more time to think about your other questions, you have fair points that should be considered. Thanks for the links too.

Can you provide an example?

I can see it happening in this scenario:

• onCreateViewHolder gets called and locks the object;
• it calls Section.getItemViewHolder (which is implemented by the user of the library - we don't have control);
• user implemented some logic in Section.getItemViewHolder that calls getItemCount (or any of the wrapped methods);
• getItemCount will wait object to be unlocked - which will never happen;

Do you think this is a possible scenario?

[yccheok]

getItemCount will wait object to be unlocked - which will never happen;

getItemCount wouldn't wait the object to be unlocked. It already acquired the lock

For instance,

void func1() {
    synchronized(the_only_monitor_instance) {
        func2();
    }
}

void func2() {
    synchronized(the_only_monitor_instance) {
    }
}

When func1 called func2, it doesn't need to wait the_only_monitor_instance to be unlocked in func2, as it already acquire the since the beginning of func1.

I wrote a proof of concept

public class SectionedRecyclerViewAdapter {
    private final Object monitor = new Object();
    private final Section section;

    public SectionedRecyclerViewAdapter() {
        section = new Section(this);
    }

    public void onCreateViewHolder() {
        synchronized (monitor) {
            System.out.println("onCreateViewHolder");
            section.getItemViewHolder();
        }

        System.out.println("onCreateViewHolder DONE");
    }

    public int getItemCount() {
        synchronized (monitor) {
            System.out.println("getItemCount DONE");
            return 1;            
        }
    }

    public static class Section {
        private final SectionedRecyclerViewAdapter sectionedRecyclerViewAdapter;
        public Section(SectionedRecyclerViewAdapter sectionedRecyclerViewAdapter) {    
            this.sectionedRecyclerViewAdapter = sectionedRecyclerViewAdapter;
        }

        // Implemented by user
        public void getItemViewHolder() {
            this.sectionedRecyclerViewAdapter.getItemCount();
        }
    }

    public static void main(String[] args) {
        SectionedRecyclerViewAdapter sectionedRecyclerViewAdapter = new SectionedRecyclerViewAdapter();
        sectionedRecyclerViewAdapter.onCreateViewHolder();
    }
}

The outcome are

onCreateViewHolder
getItemCount DONE
onCreateViewHolder DONE

[luizgrp]

You need more than one thread, please try:

public class LockPoc {

    public static void main(String[] args) {

        final SectionedRecyclerViewAdapter srva = new SectionedRecyclerViewAdapter();

        Section section = new Section(() -> {
            System.out.println("execute @ " + Thread.currentThread().getName());
            srva.getItemCount();
        });

        srva.setSection(section);

        srva.onCreateViewHolder();
    }

    static class SectionedRecyclerViewAdapter {

        private final Object lock = new Object();

        private Section section;

        void setSection(Section section) {
            this.section = section;
        }

        void onCreateViewHolder() {
            synchronized (lock) {
                System.out.println("onCreateViewHolder @ " + Thread.currentThread().getName());
                section.getItemViewHolder();
            }
        }

        void getItemCount() {
            synchronized (lock) {
                System.out.println("getItemCount @ " + Thread.currentThread().getName());
            }
        }

    }

    static class Section {
        private final Command command;

        Section(Command command) {
            this.command = command;
        }

        void getItemViewHolder() {
            System.out.println("getItemViewHolder @ " + Thread.currentThread().getName());

            Thread thread = new Thread(command::execute);
            thread.start();
            try {
                thread.join();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }

    interface Command {
        void execute();
    }

}

[yccheok]

@luizgrp

You're right in such case. Deadlock will happen.

[luizgrp]

Hi @yccheok,

After reading carefully your concerns, I'm with you on this: the user of the lib should be responsible for ensuring thread safety, as it's not fair to degrade the performance for others that don't need it. This is the first issue raised regarding this, so doesn't seem to be common.

Hi @erenbakac,

My suggestion to you would be to create a synchronised method, responsible for updating the adapter (add sections, call notify), that will be called from your AsyncTasks, so multiple tasks won't be able to update the sections at the same time.