search

sockeqwe / mosby

A Model-View-Presenter / Model-View-Intent library for modern Android apps
http://hannesdorfmann.com/mosby/
Apache License 2.0
5.49k stars 841 forks source link

LeakCanary pointing to leak on PresenterManager #275

Closed mradzinski closed 6 years ago

mradzinski commented 6 years ago

I'm currently developing an app using Mosby 3.0.4 and LeakCanary once in a while tells me there's a 2 or 3 MB leak here.

There aren't steps to reproduce it since it happens randomly (mostly upon closing the app), but it's not a constant and can't seem to find a reason why it might be leaking there.

Any ideas why this might be happening?

sockeqwe commented 6 years ago

This should be fixed with upcoming 3.1.0 version. Please try 3.1.0-SNAPSHOT and let me know if that fixes this issue.

On So., 17. Sep. 2017, 01:34 Matias Radzinski notifications@github.com wrote:

I'm currently developing an app using Mosby 3.0.4 and LeakCanary once in a while tells me there's a 2 or 3 MB leak here https://github.com/sockeqwe/mosby/blob/e9462d1011b2acb3e3d6b6605fc17cc5496778c6/presentermanager/src/main/java/com/hannesdorfmann/mosby3/PresenterManager.java#L40 .

There aren't steps to reproduce it since it happens randomly (mostly upon closing the app), but it's not a constant and can't seem to find a reason why it might be leaking there.

Any ideas why this might be happening?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/sockeqwe/mosby/issues/275, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnrsfjwmMgAeFxGcWix9k2icQVxbh0ks5sjFrygaJpZM4PZ-4b .

mradzinski commented 6 years ago

I'm also using mosby-conductor. Will upgrading to 3.1.0 have any side effects? Just wondering if I'll need to update that one too.

sockeqwe commented 6 years ago

It's not breaking anything (like crashing), but with Mosby Conductor the new internal changes introduced in 3.1.0 are not applied yet as it requires some Mosby conductor internal changes too. I can provide you a snapshot for Mosby conductor 3.1.0 on Monday.

On So., 17. Sep. 2017, 03:13 Matias Radzinski notifications@github.com wrote:

I'm also using mosby-conductor. Will upgrading to 3.1.0 have any side effects? Just wondering if I'll need to update that one too.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/sockeqwe/mosby/issues/275#issuecomment-330005451, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnrhlUZhSAQ6yZaDiun---y_qtXWKKks5sjHI3gaJpZM4PZ-4b .

mradzinski commented 6 years ago

That would be absolutely fantastic. Just drop me a line here whenever you get some time to update mosby-conductor and I'll give both a try. Thanks mate!

sockeqwe commented 6 years ago

Hm, I had an error in reasoning. PresenterManager shouldn't be used at all with conductor. Do you use multiple Activities and MviActivity or the like?

mradzinski commented 6 years ago

I do have more than one activity, but only one of them uses Mosby (the other one is just a Splash with no presenter, so nothing to do with this). No MVI activity.

harrylefit commented 6 years ago

When is 3.1.0 version released ? My app's leaked on PresenterManager

sockeqwe commented 6 years ago

I hope this week. Mosby Conductor mist likely next week.

siavashabdoli commented 6 years ago

@sockeqwe the real problem is your Delegate classes Implementation can't recognized the view(for example fragment) is totally destroyed or it's destroyed just for orientation change. if they store anything like context in static field they get leak from LeakCanary but what about when you just don't need the presenter class anymore and it remained in PresenterManager? the presenter is not needed anymore. I think for fragment case you should check if onDestory called you should delete the presenter instance. and if developers want to get presenter from cache in case of orientation change they should setRetainInstance to true

sockeqwe commented 6 years ago

In 3.1.0 it can distinguish between not being usee anymore and temporarily detached.

There are two things to note here:

  1. Mosby-Conductor is not using 3.1.0 internally even if you update the Mosby dependency itself to 3.1.0 because​ Mosby-Conductors delegate is not migrated to 3.1.0 yet.
  2. Mosby-Conductor is not using PresenterMangaer at all, hence it is very strange that you see leak canary reports. I will investigate if there is a unwanted reference in Mosby-Conductor.
  3. There is a little chance that leak canary is reporting a false positive, because depending on how you set up leak canary, it might be the case that leak carany is checking to early and the presenter is not removed yet, but will be removed later.
  4. Its totally possible that it is indeed a bug, but I wasnt able to reproduce it yet in isolation.

Siavash A notifications@github.com schrieb am So., 12. Nov. 2017, 13:39:

@sockeqwe https://github.com/sockeqwe the real problem is your Delegate classes Implementation can't recognized the view(for example fragment) is totally destroyed or it's destroyed just for orientation change. if they store anything like context in static field they get leak from LeakCanary but what about when you just don't need the presenter class anymore and it remained in PresenterManager? the presenter is not needed anymore.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/sockeqwe/mosby/issues/275#issuecomment-343734345, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnroWw7oDnrwpYUIc3CgBq46JSYQ_aks5s1ubmgaJpZM4PZ-4b .

siavashabdoli commented 6 years ago

when it will be released? I didn't get any leak report from LeakCanary. I just checking the implementation and find out it can be leaked whether it report by Canary or not. So I decide to write an issue but I see there is already a thread about that.

sockeqwe commented 6 years ago

@siavashabdoli which classes / code do you think can create a leak?