ConcurrentModificationException when disconnecting on onLocationChanged

[westnordost]

I need a possibility to request one location update, so I wrote this class: SingleLocationRequest.java.txt. Basically, when started, it requests location updates and on the first onLocationChanged() it receives, it disconnects again.

In the current LOST release 2.1.2, this can lead to the following exception

E/AndroidRuntime: FATAL EXCEPTION: main
                  Process: de.westnordost.streetcomplete, PID: 32694
                  java.util.ConcurrentModificationException
                      at java.util.HashMap$HashIterator.nextEntry(HashMap.java:787)
                      at java.util.HashMap$KeyIterator.next(HashMap.java:814)
                      at com.mapzen.android.lost.internal.FusedLocationServiceConnectionManager.onServiceConnected(FusedLocationServiceConnectionManager.java:83)
                      at com.mapzen.android.lost.internal.FusedLocationProviderApiImpl.onServiceConnected(FusedLocationProviderApiImpl.java:65)
                      at android.app.LoadedApk$ServiceDispatcher.doConnected(LoadedApk.java:1214)
                      at android.app.LoadedApk$ServiceDispatcher$RunConnection.run(LoadedApk.java:1231)
                      at android.os.Handler.handleCallback(Handler.java:739)
                      at android.os.Handler.dispatchMessage(Handler.java:95)
                      at android.os.Looper.loop(Looper.java:211)
                      at android.app.ActivityThread.main(ActivityThread.java:5389)
                      at java.lang.reflect.Method.invoke(Native Method)
                      at java.lang.reflect.Method.invoke(Method.java:372)
                      at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1020)
                      at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:815)

I researched a bit as for the reason why and found the problem. See this figure of the stacktrace. It is likely that it is only reproducible with at least 2 clients that connect at the same time.

Actually, a "working example" of this issue could be reduced: A client that disconnects again right on onConnect() will spark the same problem as the posted class of mine.

Workaround

The workaround for users is to defer disconnecting from the lost api client, i.e.

Handler h = new Handler(Looper.getMainLooper());
h.post(new Runnable()
{
    @Override public void run()
    {
        LocationServices.FusedLocationApi.removeLocationUpdates(lostApiClient, this);
        lostApiClient.disconnect();
    }
});

Solution

I think the most straightforward and easiest solution is to copy the connectionCallbacks in FusedLocationServiceConnectionManager and iterate on the copy in onServiceConnected and onServiceDisconnected. Also, it might make sense to check this race condition for other callbacks that the lost api makes to its clients.

[ecgreb]

Thanks for the detailed report!

While the exception you have documented and proposed solution looks logical for some reason I'm still unable to reproduce the issue locally.

I put together a test project that tries to recreate the situation you have described using multiple location clients and the SingleLocationRequest class you posted here.

https://github.com/ecgreb/lost-test

First I connect one location client in MainActivity then when the FAB is clicked I trigger a single request with a new client using the SingleLocationRequest class. The second client is connected, the single update is received, and the second client is disconnected all with no issues.

However then I added some breakpoints and discovered that when the second client is disconnected there is only one set of ConnectionCallbacks registered with the FusedLocationServiceConnectionManager which maybe points at a deeper underlying issue with how callbacks are registered?

It seems like at the time the second client is disconnected in the location callback there should be two sets of connection callbacks registered.

[westnordost]

I created a minimal working example of the issue. The difference is that several clients need to connect at the same time.

[ecgreb]

That did the trick! Fix here https://github.com/mapzen/lost/pull/163.

[carlos-mg89]

@ecgreb Just to clarify, was the fix to this issue released on version 3.0.2 or after that one? I'm having the ConcurrentModificationException at com.mapzen.android.lost.internal.LostClientManager.iterateAndNotify(LostClientManager.java:286).

Full log here:

java.util.ConcurrentModificationException
        at java.util.HashMap$HashIterator.nextNode(HashMap.java:1441)
        at java.util.HashMap$KeyIterator.next(HashMap.java:1465)
        at com.mapzen.android.lost.internal.LostClientManager.iterateAndNotify(LostClientManager.java:286)
        at com.mapzen.android.lost.internal.LostClientManager.reportLocationChanged(LostClientManager.java:153)
        at com.mapzen.android.lost.internal.FusedLocationServiceCallbackManager.onLocationChanged(FusedLocationServiceCallbackManager.java:33)
        at com.mapzen.android.lost.internal.FusedLocationProviderApiImpl$1$1.run(FusedLocationProviderApiImpl.java:60)
        at android.os.Handler.handleCallback(Handler.java:789)
        at android.os.Handler.dispatchMessage(Handler.java:98)
        at android.os.Looper.loop(Looper.java:164)
        at android.app.ActivityThread.main(ActivityThread.java:6541)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:240)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:767)

I have version 3.0.2, and I have been experiencing this exception for some time while on development. I usually have only 1 listener, but at certain moment I have 2 attached listeners.

I've tried for a couple of days now with 3.0.4 (which actually was great to update anyway) and seems to be working fine, but would like to double check if the issue happened on 3.0.2 or in an earlier version than that one.

Thanks in advance.

[ecgreb]

The original issue reported here was fixed in version 2.2.0. Comparing the stack traces this appears to be a different (but possibly related) issue.

There were a number of change to the LostClientManager between 3.0.2 and 3.0.4 that could account for the differences you are seeing.

https://github.com/lostzen/lost/commit/20fc93ec0bb9a1d92427a2dadcaa13f1b086f018#diff-d5ddc527f6c3bcb25442830fa846b8f0

If you are still able to reproduce in version 3.0.4 can you open a separate issue? Thanks.

[carlos-mg89]

@ecgreb I have been doing plenty of changes to the code (meaning, that I should be getting that exception but now I'm not in 3.0.4), and now I'm unable to reproduce it under 3.0.4. But that exception was being thrown in 3.0.2. However, as I just said in this comment and in my previous comment, everything seems to be working perfectly fine in 3.0.4, and I can call requestLocationUpdates and removeLocationUpdates without troubles.

If you think it's still worthy to open that separate issue although I'm not able to reproduce it in 3.0.4, I could definitely do it.

[ecgreb]

@carlos-mg89 thanks for the follow up. If you see the exception in 3.0.4 feel free to open a new issue otherwise it sounds like the LostClientManager refactor sorted it out.