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

Synthetic MviPresenter generated classes instances not GC cleared after Activity/fragment destruction #343

Open ruieduardosoares opened 3 years ago

ruieduardosoares commented 3 years ago

Mosby Version: 3.1.1

Expected behavior

MviPresenter$classes instances should be garbage collected after leaving fragment and its activity host and not bound to any INSTANCE object

Actual behavior (include a stacktrace if crash) No crash associated.

The problem is that the generated synthetic classes instances in presenter are somehow bounded to an INSTANCE that is not garbage collected after leaving the screen despite several attempts to force GC on Android profiler.

Most certain the synthetic classes can probably be the anonymous functions in the flatMap calls

See image image

Snippet of source code of presenter

image

This presenter lifecycle is being handled by mosby and not reference nor it is being passed to any other component outside the Fragment it belongs to.

The TakePhotoInteractor instance is created on demand also

Steps to reproduce the behavior or link to a sample repository

Just implement this library and follow http://hannesdorfmann.com/android/mosby3-mvi-2

Doesnt mosby disposes that observables created from the view intents?

Any thoughts about this?

sockeqwe commented 3 years ago

Hm, need to check this more in detail but that issue is new or it broke because of missing androidx compatability.

ruieduardosoares commented 3 years ago

Not sure androidx compatibility is an issue because only the package name were changed from android.support to androidx.

But this is just a guess, I am only aware of what android documentation states

Also I have jetifier enabled so mosby byte code is change to access the correct package from android.support to androidx.

I want to continue to use this library and with a few tweaks I am able to use it with Android Navigation component by only using fragments.

Thanks for the reply.

ruieduardosoares commented 3 years ago

More digging i found out the INSTANCE might be a "constant" referenced somewhere that is the Synthetic class itself

See image

I just hope this is not kotlin's fault. Neither mosby, but i still dont know how is this happening.

ruieduardosoares commented 3 years ago

At first sight this seems to be the View interface object which is the fragment :thinking:

image

But this is not possible because the subscription to View is being canceled when fragment is onStop().

And TakePhotoFragment already was GC

image

I can only see companion objects here

And the TakePhotoFragment$Companion doesnt have any reference to any presenter except to a TAG string field image

sockeqwe commented 3 years ago

No idea what this companion object is. Do you use any kind of dependency injection or could share the source code somehow?

Rui Eduardo Soares notifications@github.com schrieb am Mi., 9. Dez. 2020, 10:45:

At first sight this seems to be the View interface object which is the fragment 🤔

[image: image] https://user-images.githubusercontent.com/13103244/101611862-f08bbc80-3a01-11eb-8974-6d3cf4a3e9f9.png

But this is not possible because the subscription to View is being canceled when fragment is onStop().

And TakePhotoFragment already was GC

[image: image] https://user-images.githubusercontent.com/13103244/101612064-2630a580-3a02-11eb-8a64-900f4b63be1a.png

I can only see companion objects here

And the TakePhotoFragment$Companion doesnt have any reference to any presenter except to a TAG string field [image: image] https://user-images.githubusercontent.com/13103244/101612695-d6061300-3a02-11eb-81c5-d79ea9b9f242.png

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sockeqwe/mosby/issues/343#issuecomment-741657679, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEOPLVDR44NQA3LDYB7HRDST5BKXANCNFSM4UQLZ2PA .

ruieduardosoares commented 3 years ago

The companion object is the one declared in the TakePhotoFragment, it only has a TAG property to use in logs.

There is no big issue with this companion object

My concern is only those synthetic classes from bindintent() method from the previous screenshots I posted, that have an object allocation for each view method.

Like I said, not sure if is kotlin or mosby the source of this issue, but the profiler is telling that those synthetic classes have one corresponding instance and are using 40 bytes on shallow size and 40 on retained size.

By themselves they are not referencing any other object in the heap, but are stuck there still don't know why.

They must be being referenced somewhere as the GC cannot collect them.

This is some dark magic.

I will need to dig more on this

ruieduardosoares commented 3 years ago

Checked kotlin issue tracker and found this.

ruieduardosoares commented 3 years ago

Instead of using lambdas with SAM conversion, I will use object expression for the intent() ViewIntentBinder and check what happens

ruieduardosoares commented 3 years ago

My suspicions are true after all.

Converted all Lambda SAM conversions into object expressions making the code a bit ugly and larger (Changed Android studio Theme BTW) image

But after GC all previous synthetic classes instances are no longer allocated :confused:

image

So this seems to by a kotlin issue rather than mosby.

ruieduardosoares commented 3 years ago

Now, to reduce the code size i will try to use Anonymous functions instead of Object Expressions and see if this also works

ruieduardosoares commented 3 years ago

And their are back with Anonymous functions :cry:

image

image

ruieduardosoares commented 3 years ago

Never mind

image

The issue continue....