Skip to content
This repository has been archived by the owner on Aug 16, 2023. It is now read-only.

Mention (or fix) locationCallback leak #7

Closed
bubenheimer opened this issue Apr 25, 2020 · 14 comments
Closed

Mention (or fix) locationCallback leak #7

bubenheimer opened this issue Apr 25, 2020 · 14 comments

Comments

@bubenheimer
Copy link

The codelab might mention the longstanding memory leak issue regarding locationCallback, or, even better, bring about a fix; the locationCallback object and any (potentially inadvertently) referenced objects will never actually be garbage collected after calling removeLocationUpdates(). In the case of the codelab it's just the (already eternal) foreground service, but the consequences tend to be more pronounced for other uses:

android/location-samples#26
https://issuetracker.google.com/issues/37126862

@codingjeremy
Copy link
Contributor

Let me look into the status and follow up.

@bubenheimer
Copy link
Author

bubenheimer commented Jan 31, 2021

Thanks, much appreciated! I just tried my repro again, leak is still present, with latest versions of all dependencies:

https://github.com/bubenheimer/requestlocationupdatesleak

Adding & removing the LocationCallback 4 times crashes the repro app on Pixel 3 XL Android 11 test device with OutOfMemoryError. (This particular LocationCallback allocates a 50MB buffer for leak repro purposes.)

@codingjeremy
Copy link
Contributor

I will pass this along. Thanks for the extra information + code+

@codingjeremy
Copy link
Contributor

Talked to the team. They are going to look into it after the rush over Android Dev Previews is done. I hope to have an update in mid-April to late-April.

@bubenheimer
Copy link
Author

bubenheimer commented Mar 5, 2021

Thank you so much for your efforts. It would be wonderful to get this longstanding problem resolved, depending on the use case an appropriate workaround can be a real architectural pain point. In case I can offer additional assistance in the resolution, please let me know.

@bubenheimer
Copy link
Author

It's been a while. Has there been any progress? I've updated the sample to use the latest versions of everything, but the leak situation is unchanged. Thanks.

@codingjeremy
Copy link
Contributor

Sorry for the delay as I moved off location awhile back, but I wanted to follow up when I had more information.

I got an updated from ENG and they completely rewrote the way they store listeners and a bunch of other stuff. They closed the internal bug on this saying it should be resolved.

Do you mind verifying with the latest version if you are still seeing the issue? I can follow up again if not. Thanks again for you patience!

@bubenheimer
Copy link
Author

bubenheimer commented Feb 7, 2022

I've updated to latest versions (location 19.0.1) and the problem is unchanged. I tested on API 31 emulator and Samsung S21 with latest updates, which is January 1 update level for Google Play system update & security update. Play Services used to print client & service library versions to the log, but I don't see that anymore (maybe that was just Maps).

@codingjeremy
Copy link
Contributor

Shoot, that isn't good news, thank you so much for testing again.

Let me reach out to them and point to this bug.

@codingjeremy
Copy link
Contributor

codingjeremy commented Feb 9, 2022

Ok, so I have some possibly good news, the rewrite I mentioned hasn't gone live yet. That's my fault for misunderstanding that it was fixed in production.

It goes live in version 20. The engineer who wrote the rewrite checked and wasn't seeing the same issue, i.e., it should be resolved.

However, that does mean we have to wait until 20 is released. Do you mind checking it again on the release to double check? It shouldn't be too long... hopefully maybe in a month or so. I can update this thread when I see it goes out for you to check.

@bubenheimer
Copy link
Author

Sounds good, I will check it once I become aware of the new release. Thanks!

@codingjeremy
Copy link
Contributor

Just to follow up, they are still working on the releasing version 20.x.x (with fix) as FYI. Once I see it go out, I will let you know (if you don't see it first).

@bubenheimer
Copy link
Author

The release is out and I've verified that the leak indeed looks fixed. It's like 7 christmases combined after this many years of ugly workarounds and much energy spent to get it resolved.

Thank you @codingjeremy for making a difference and bringing about the fix 🙏🙏🙏

@codingjeremy
Copy link
Contributor

Awesome, I'm glad it is resolved! You beat me to it. :)

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

No branches or pull requests

2 participants