servalproject/batphone — pull request #81 by rom1v (2013-03-10 22:53:24+00:00)
Title: Peer list concurrent access crash (fix #71)

I had the same error as you had in bug https://github.com/servalproject/batphone/issues/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*:
~~~java
// 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).