Possible memory leak LOST 3.0.4

[westnordost]

Description

LeakCanary detected a leak in my app, which originates in LOST (current master on StreetComplete). The leak occurs very rarely. It may be that I am just using LOST wrong, but I am happy for any hints what I might be doing wrong. So that's why I will provide a boiled-down code example here. Taken from the actual source, but boiled down to what is relevant:

In a fragment (MapFragment.java):

private LostApiClient lostApiClient;

// Called from outside, may be called multiple times
public void startPositionTracking()
{
    if(!lostApiClient.isConnected()) lostApiClient.connect();
}

@Override public void onConnected() throws SecurityException
{
    LocationRequest request = LocationRequest.create()
        .setInterval(2000)
        .setSmallestDisplacement(5)
        .setPriority(LocationRequest.PRIORITY_HIGH_ACCURACY);
    LocationServices.FusedLocationApi.requestLocationUpdates(lostApiClient, request, this);
}

@Override public void onStop()
{
    super.onStop();
    if(lostApiClient.isConnected())
    {
        LocationServices.FusedLocationApi.removeLocationUpdates(lostApiClient, this);
        lostApiClient.disconnect();
    }
}

Looking at this code, there might be several points where something is going wrong:

1. lostApiClient.connect() can be called several times, even with the if(!lostApiClient.isConnected()) check, because race-condition. This will lead to onConnected() being called several times and thus several LocationRequests being registered for the listener (this). I don't know if this has adverse effects (duplicate calls to the listener?) but I don't see how this can be the source of the memory leak. If it has adverse effects, how can a user of the library safeguard against this race condition?

2. I suppose the second race condition is at onStop(): The location updates are only removed and the lostApiClient only disconnected if the lostApiClient has already been connected. But onStop() may be called while it is still connecting. In other words, onStop is called before onConnected. I guess this is the source of the memory leak. But what is the designated way to safeguard against this?

Of course in both cases, I could introduce some boolean variables (lostAlreadyConnected, fragmentAlreadyStopped), but as any user of the library would have do this kind of stuff to safeguard against memory leaks, it would make more sense to have this handled by the library itself. (I.e. only ever call onConnected once; don't call onConnected when the fragment has been stopped or something.)

Lost & Android Version

3.0.4, Android 5.1.1

[sarahsnow1]

Thanks for the well documented report!

1. We should update Lost to do nothing if the client is already connected and connect() gets called (its odd it doesn't behave like this at the moment bc thats how PlayServices seems to work). The leak you sometimes see is likely resulting from this because what could happen is this flow:

method call	LostApiClient connection state
startPositionTracking()	CONNECTING
onConnected()	CONNECTED
startPostionTracking()	CONNECTING
onStop()	CONNECTING

In this case, you'd leak that listener registered in onConnected because lostApiClient.isConnected() would return false and you wouldnt unregister it

2. You could update your code to always call LostApiClient#disconnect() when stopped or use the LostApiClient#unregisterConnectionCallbacks method (and then ensure balanced call to LostApiClient#registerConnectionCallbacks is added where it makes sense. But, because of what I say in 1, no change should be necessary after we update the callback behavior

We will address this shortly with a PR but if you need it immediately, feel free to submit your own PR!

[westnordost]

1

I am not sure if I understand this. So the connection state can go back from CONNECTED to CONNECTING, when connect() is called a second tiime? Are you sure about this?

2

To leave out the isConnected check seems to not be possible because LostApiClient.disconnect() is documented to throw an exception if called when location updates are still registered (or could be that it is only phrased mistakable). FusedLocationProviderApi.removeLocationUpdates in turn is documented to throw an exception if called while the client is not connected yet. So, it looks like I have to first check for if the client is connected, then remove location updates and then disconnect.

Also, I think your solution for (1) would not fix the race condition I originally explained in (2). Imagine this call order:

1. startPositionTracking() called from outside. lostApiClient connects.
2. onStop() called from Android system
3. lostApiClient successfully connected. onConnected() called

-> The lostApiClient is never disconnected.

[sarahsnow1]

1. Yeah, because the LostApiClient calls the FusedLocationProviderApi#connect method which calls this internal object's connect method and updates the state used when determining if a client is connected

2. I didn't realize those javadocs were outdated, I created a ticket to update them because this method will no longer throw (but you are right that the LocationServices#removeLocationUpdates method will throw) so changing your code to something like this should resolve it:

if (lostApiClient.isConnected()) {
   LocationServices.FusedLocationApi.removeLocationUpdates(lostApiClient, this);
}
lostApiClient.disconnect();

Looking at the code in Lost, it seems like this race condition is not possible if LostApiClient#disconnect is always called because the client and connection callbacks are removed. What does become apparent though is that if this is the only client connected there is a race condition in the underlying service and it will receive connection callbacks/be connected when it should be shutdown. This could be resolved by updating this method to always execute what is in the if (isBound). I will need to give this more thought to make sure there aren't unintended side effects.

[westnordost]

Okay, thank you for the advise and analysis :-)

[msmollin]

Closing as this repo is no longer maintained.