Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Peer list concurrent access crash (fix #71) #81

Merged
merged 1 commit into from Apr 17, 2013

Conversation

rom1v
Copy link
Contributor

@rom1v rom1v commented Mar 10, 2013

I had the same error as you had in bug #71.

Here is my analysis.

When we need to access and modify an ArrayList from two different threads, there are typically two solutions:

  1. synchronize access (mutex);
  2. serialize the calls in a FIFO queue for making them happen in one thread.

As peers variable is used as PeerListAdapter backend, we must choose the second solution, and the thread must be the UI thread.

Currently, wrong access to peers occur at two places:

  • in listener, peerChanged(...) modifies peers from outside the UI thread;
  • in refresh(), the AbstractJniResults.putBlob(...) method does the same.

The comments on the sort() method suggests the problem already happened and has been workarounded:

    // list sorter that preserves new items added to the end in a different
    // thread, unlike Collections.sort which just crashes.
    private void sort() {
        IPeer array[] = peers.toArray(new IPeer[peers.size()]);
        Arrays.sort(array, new PeerComparator());
        for (int i = 0; i < peers.size() && i < array.length; i++)
            peers.set(i, array[i]);
    }

This kind of workaround avoids to crash, but can result in an incorrect display (a missing item for example).

The fix I propose consists in always accessing/modifying peers in the UI thread.
It removes the need of the handler, the refreshing variable and the custom sort() method.
It also allows to sort and notify only when needed (when an item is added).

@rom1v
Copy link
Contributor Author

rom1v commented Mar 10, 2013

By the way, I don't see any piece of code which removes an item when a peer is not reachable anymore. Did I miss something?

@lakeman lakeman merged commit aa36438 into servalproject:development Apr 17, 2013
@lakeman
Copy link
Member

lakeman commented Jun 5, 2013

Looks like this issue is cropping up again in the latest build on some phones;

java.lang.IllegalStateException: The content of the adapter has changed but ListView did not receive a notification. Make sure the content of your adapter is not modified from a background thread, but only from the UI thread. [in ListView(16908298, class android.widget.ListView) with Adapter(class org.servalproject.PeerListAdapter)]
at android.widget.ListView.layoutChildren(ListView.java:1492)
at android.widget.AbsListView.onTouchModeChanged(AbsListView.java:1960)
at android.view.ViewTreeObserver.dispatchOnTouchModeChanged(ViewTreeObserver.java:591)
at android.view.ViewRoot.ensureTouchModeLocally(ViewRoot.java:2021)
at android.view.ViewRoot.ensureTouchMode(ViewRoot.java:2005)
at android.view.ViewRoot.handleMessage(ViewRoot.java:1774)
at android.os.Handler.dispatchMessage(Handler.java:99)
at android.os.Looper.loop(Looper.java:123)
at android.app.ActivityThread.main(ActivityThread.java:4627)
at java.lang.reflect.Method.invokeNative(Native Method)
at java.lang.reflect.Method.invoke(Method.java:521)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:868)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:626)
at dalvik.system.NativeStart.main(Native Method)

@rom1v
Copy link
Contributor Author

rom1v commented Jun 5, 2013

@lakeman In PeerList, a notification was missing after peers change.
I added a commit on top of bug71 that you can merge in, but github don't add it to this pull request automatically.

While searching for the problem, I also read CallDirector (because it uses PeerListAdapter too), and this piece of code seems strange:

@Override
protected void onProgressUpdate(DnaResult... values) {
    if (adapter.getPosition(values[0]) < 0)
        adapter.add(values[0]);
    }
}

I think this method should iterate over each DnaResult in values, no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants