search

maplibre / maplibre-navigation-android

Maplibre Navigation SDK for Android
MIT License
92 stars 51 forks source link

Fix memory leak in navigation service #54

Closed skienzl closed 1 year ago

skienzl commented 1 year ago

This PR closes #52

boldtrn commented 1 year ago

Thanks for working on this 👍. You linked a PR from Mapbox that was included already when this repo was forked. The change you implemented here is also different to the changes that were made upstream. All of this is perfectly fine.

I did a bit more digging around and it seems that this memory leak is a well known issue for Android and around for many years.

It is really interesting that the Android documentation actually recommends to implement it this way.

Is it possible that there are downsides when we use the updated code you provided? Could there be issues due to using WeakReference? Would it help to move the binder out of the Service?

Could you provide a way to reproduce the leak on main? To verify the PR fixes this?

skienzl commented 1 year ago

Thanks for working on this 👍. You linked a PR from Mapbox that was included already when this repo was forked. The change you implemented here is also different to the changes that were made upstream. All of this is perfectly fine.

Yeah. I think i was in the wrong branch during the time i checked which mapbox navigation ui version I can migrate to maplibre. Sorry for that.

I did a bit more digging around and it seems that this memory leak is a well known issue for Android and around for many years.

It is really interesting that the Android documentation actually recommends to implement it this way.

I was wondering about that as well when i was looking into the topic. But it seems like the best practice sample was never fixed.

Is it possible that there are downsides when we use the updated code you provided? Could there be issues due to using WeakReference? Would it help to move the binder out of the Service?

Currently it is only used at one place inside the MapboxNavigation ServiceConnection. As far as I can tell it should not cause any trouble there since the usage of the service is check via isServiceAvailable each time as well.

Could you provide a way to reproduce the leak on main? To verify the PR fixes this?

I can reproduce it everytime on my pixel 6 when i start the navigation ui sample in the app. The fix seems to be working and leakcanary ist not detecting a leak anymore.

boldtrn commented 1 year ago

Thanks, I verified the fix locally on my device as well, I think this is almost good to merge. If possible it would be nice if you could add the mentioned null check.

skienzl commented 1 year ago

Thanks, I verified the fix locally on my device as well, I think this is almost good to merge. If possible it would be nice if you could add the mentioned null check.

Which do you mean? The nullable annotation is already implemented and pushed. And the null check in the MapboxNavigation. isServiceAvailable was already implemented before this PR.

boldtrn commented 1 year ago

Which do you mean?

MapboxNavigation#onServiceConnected, here we are getting the service and call startNavigation on the service, but the service could be null. I am not sure if this case could actually happen as in this case the service should be freshly created.

skienzl commented 1 year ago

Which do you mean?

MapboxNavigation#onServiceConnected, here we are getting the service and call startNavigation on the service, but the service could be null. I am not sure if this case could actually happen as in this case the service should be freshly created.

Is that even possible at that moment? Since onServiceConnected gets called when the service is connected and the binder of the service is ready. And a restart would restart the server which the system just "confirmed" as connected and ready. The documentation also mentions that the callback is never received if there is an issue with the service itself: https://developer.android.com/reference/android/content/ServiceConnection#onServiceConnected(android.content.ComponentName,%20android.os.IBinder)

boldtrn commented 1 year ago

I am not sure if this can happen, my thinking was, that checking it is not null makes sure it can't happen.

With services I tend to be overly cautious, as different manufacturers like to introduce their own "battery optimizations".

If you believe this can't happen, I am fine with merging it as it is.

skienzl commented 1 year ago

I am not sure if this can happen, my thinking was, that checking it is not null makes sure it can't happen.

With services I tend to be overly cautious, as different manufacturers like to introduce their own "battery optimizations".

If you believe this can't happen, I am fine with merging it as it is.

I just a bit worried if we get a wired state of we create a new service at this callback. Since we recreate a service which should be already running. Just not sure what would be the right resolution to the null state of the service at this callback.

Yeah. Those custom battery "optimiser" make the handling those long running services quite frustrating...

boldtrn commented 1 year ago

Just not sure what would be the right resolution to the null state of the service at this callback.

Right now the app would crash I guess. Maybe this is even the correct behaviour, then maybe we should add a comment about this?

skienzl commented 1 year ago

@boldtrn i added the null check and an comment to fix the linter warning and add more context to the exception

boldtrn commented 1 year ago

Thank you very much for the great PR :)