-
Notifications
You must be signed in to change notification settings - Fork 344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Kotlin - lateinit memory leak issues #234
Comments
This is a tough problem to solve, and I've run into it too. How would you like to see this called out? In regards to an annotation processor nulling out the property, that's no more possible than it is for you to set it to null yourself. You just can't set a property to null unless it's an optional. One way to get around this would be to mark it as a |
FWIW, I believe that these properties should simply be optional. Even if you have a good understanding of when it would be null and when it wouldn't, you'd be bypassing the null-safety of the language by declaring something as non-nullable when it isn't. It would be pretty easy to accidentally allow a callback from a long-running operation try to access these properties after they've been nulled out. This seems like something that subtly lowers the maintainability of your codebase, as a new developer coming in wouldn't know about that unexpected gotcha. |
You're right, my bad
Using delegates like below sounds like a pretty interesting solution, I'll have to try it out and report back. http://stackoverflow.com/a/42398736/891242 If this approach ends up being the best way to handle it - could we add a conductor-specific delegate as an optional artifact? |
For my navigation drawer, I have a I disagree that all properties should be nullable in Kotlin, because then there's no distinction between what can be nonnull/nullable within the scope of |
Why would you want to set them back to null on I make my component initialization (mutable) lazy and enforce initialization in In my Controller I'd just access my dependencies by Not sure why you'd still want to reset your references because in the case the controller goes through another No nullable types, only for actual nullable dependencies. |
This will cause a memory leak on back stack navigation if another
controller is pushed on top because component will only be instantiated
again (and garbage collected the old) once the user navigates back
Simon Vergauwen <notifications@github.com> schrieb am Mi., 22. Feb. 2017,
22:11:
… Why would you want to set them back to null on onDestroyView? I'm also
not sure why you'd want to inject the values, I just use dagger 2 component
based functionality.
I make my component initialization (mutable) lazy and enforce
initialization in onCreateView (A lateinit would work as well here). The
reason I enforce initialization of my component in onCreateView is that
on a configuration change the parent dependency (Activity) will also be
different and thus a new initialization is required. (in the case your
component depends on the activities component at least).
In my Controller I'd just access my dependencies by component.presenter
for some more syntactic sugar you could just use val presenter:
SalesPresenter get()= component.presenter and with kotlin 1.1 you could
even make the getter inline.
Not sure why you'd still want to reset your references because in the case
the controller goes through another onCreateView/onDestroyView the
controller holds a reference to the new component and the previous one will
be garbage collected.
No nullable types, only for actual nullable dependencies.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#234 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAjnrhmTGcxIQPxPrvIQFVIsuR0d_6BPks5rfKSCgaJpZM4MI43d>
.
|
Wouldn't it just get garbage collected with the controller? And if that's not the case that could be solved with a Conductor aware delegate something like this class ControllerLazy<out V>(private val initializer: (Controller, KProperty<*>) -> V) : ReadOnlyProperty<Controller, V> {
private object EMPTY
private var value: Any? = EMPTY
override fun getValue(thisRef: Controller, property: KProperty<*>): V {
if (value == EMPTY) {
value = initializer(thisRef, property)
thisRef.addLifecycleListener(object : Controller.LifecycleListener() {
override fun postDestroyView(controller: Controller) {
super.postDestroyView(controller)
value = EMPTY
thisRef.removeLifecycleListener(this)
}
})
}
@Suppress("UNCHECKED_CAST")
return value as V
}
} |
Maybe I'm wrong or overlooking something, but I was thinking about the
following workflow:
1. Start app in Portrait
2. Controller A starts
3. Controller A onCreateView() creates Component with internally holding a
reference to PortraitActivity.
4. Push Controller B on top of the backstack.
5. Controller B starts, Controller B onCreateView(), create Controller B's
dagger component with a refernce to PortaritActivity.
6. Controller A onDestroView() but no cleanup of references, i.e.
Controller A's dagger component is not set to null.
7. User rotates the screen
8. Controller B onCreateView(), create new Dagger component with internal
reference to LandscapeActivity. Previous dagger component can noe be
garbage collected. Controller A, however, still holds a reference to dagger
component with PortraitActivity -> Memory leak
9. User presse back button. Pop backstack, Controller B can be garbage
collected (also B's dagger component can be garbage collected)
10. Controller A onCreateView() creates a new dagger component with
referencs to LandscapeActivity (replaces previous dagger component
referencing PortraitActivity).
So the time between step 7. and step 10. Controller A is holding a
reference to PortraitActivity preventing PortraitActivity from being
garbage collected -> Memory leak.
Please note that this is exactly the same with retaining Fragments. Should
that be mentioned in the docs: yes. Should conductor deal with that
internally: I don't think so.
Simply use kotlin's !! or use kotlin delegates or have separate nullable
field for the baking property, but at the end you have to cleanup your
resources manually (setting them null) somehow no matter the programming
language.
Btw. I'm not sure how kotlin-android plugin (that you dont have to write
findViewById() manually anymore, but rather use an extension property) is
doing that, I wilk take a look tomorrow but I guess its using a weak
reference and a delegating KProperty.
Simon Vergauwen <notifications@github.com> schrieb am Mi., 22. Feb. 2017,
22:28:
… Wouldn't it just get garbage collected with the controller?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#234 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAjnrjbum1qiVCnIpwY2nvdvHSjXRUIpks5rfKiDgaJpZM4MI43d>
.
|
no, conductor keeps every controller in memory as long as it is on the back stack (doesn't have to be on the top of the back stack), only the View (xml layout) is removed and garbage collected because the UI widget are the heavy weighted things (regarding memory consumption), not the controller itself. You can simply add a lifecycle listener to log the lifecycle events a controller and you will see that when coming back from back stack the same controller instance is used (no new controller instance created). Furthermore, you will see that
The bundle constructor is only used to restore a controller after a process death. |
Yeah, I took a look at the code the make sure. And I was wrong. That is why I deleted my comment :) Thanks for pointing this out @sockeqwe! What are your thoughts on the approach I mentioned with a delegate that clears the reference on I realise it is just avoiding dealing with nullable dependencies, but those dependencies shouldn't be used in a longer lifecycle than I use the same delegate for Kotterknife, but I feel that throwing an IllegalStateException when trying to use a view outside of the |
And I hope it will stay this way, it makes working with controllers so much more predictable, especially when you have some MVP stuff built on top, like presenters which live for as long as backstack lives... |
Ultimately, this is the solution I've implemented, and I'm happy to report it works quite well. If I want a property to survive a configuration change, I just don't add the
|
Nice solution @ZakTaccardi, and thanks for making it public, I had this issue myself and solved it pretty much the same way. Although, I'm not a very big fan of that |
@mradzinski yeah I wasn't sure what to name it - trying to keep it as terse as possible. |
Just throwing another thank you on there @ZakTaccardi. I had another solution going that I was stubborn about swapping out until today. Your resettable reference made things so much nicer! |
One could also write a |
What about this?
|
If you remove the Instead I'd directly couple it to the instance by passing it. (Else it is also some kind of error prone because you could pass the property around and use it in multiple controllers. import com.bluelinelabs.conductor.Controller
import kotlin.properties.ReadWriteProperty
import kotlin.reflect.KProperty
/**
* A property that clears the reference after [Controller.onDestroyView]
*/
class ClearAfterDestroyView<T : Any>(controller: Controller) : ReadWriteProperty<Controller, T> {
private var value: T? = null
init {
controller.addLifecycleListener(object : Controller.LifecycleListener() {
override fun postDestroyView(@Suppress("NAME_SHADOWING") controller: Controller) {
value = null
}
})
}
override fun getValue(thisRef: Controller, property: KProperty<*>) = value
?: throw IllegalStateException("Property ${property.name} should be initialized before get and not called after postDestroyView")
override fun setValue(thisRef: Controller, property: KProperty<*>, value: T) {
this.value = value
}
}
fun <T : Any> Controller.clearAfterDestroyView(): ReadWriteProperty<Controller, T> = ClearAfterDestroyView(this)
This has also the advantage that it is not coupled to a base class but can be used through the extension function from any controller. |
Hi @PaulWoitaschek, thanks for the feedback. Do you mean the controller is leaked? I don't see how |
You create the property only once. When now When you now push another controller on top, the view gets destroyed, but the reference stays. |
Good news. Half of the proposal is going to be implemented in Kotlin 1.2 and the rest in 1.3. |
Until we can deinit we're still stuck with the same memory leak ( Waiting till 1.3 to come out some day next year to get rid of this hacks, lol |
Tee property that is tied to the like cycle is actually pretty nice so I'm not sure if I'd de-init the property manually. |
If you follow an MVVM pattern instead of MVP you should not have this issue at all because in MVVM the injected components (view model) won't hold a reference to the views and thus even if the reference in the controller isn't nulled it won't leak a reference to the Activity. In MVVM the view will hold a reference to the view model (e.g. injected by Dagger) but that's ok since the views will be garbage collected once they detach from the window/activity/controller. In other words, conductor might not be as architecture-agnostic as it claims to be, at least not in combination with Kotlin. |
@1gravity are you referring to a databinding trick where you don't have a hard reference to a view? In MVVM you generally do reference your views from your activity/fragment/controller, so.. |
The conductor controller (as well as fragments and activities) together with the layout and custom views are the View in an MVVM design and hold a reference to the view model but not vice versa. This article explains what I mean quite accurately: https://medium.com/upday-devs/android-architecture-patterns-part-3-model-view-viewmodel-e7eeee76b73b. The controller holds references to the layout/view and the ViewModel. In onDestroyView the references to the views are destroyed severing the reference to the Activity context while the references to the ViewModel won't leak anything since these should imo be singletons. All that matters is that rx subscriptions are cancelled or disposed (I dispose in onDetach) or you might end up with multiple subscriptions to the same event stream. |
Conductor should boldly advertise (I don't think it's made obvious enough for new users) that its instances survive configuration changes (stark contrast to
Activity/Fragment/View
) - this has huge implications for dagger + kotlin users.My
lateinit
properties are injected by dagger, and I need to set them tonull
inonDestroyView
- or have a memory leak. This however is not possible in kotlin, as far as I am aware (without reflection). I could make these properties nullable, but that would defeat the purpose of Kotlin's null safety.I'm not quite sure how to solve this. Ideally there could be some type of annotation processor that would null out specific variables automatically in
onDestroyView
The text was updated successfully, but these errors were encountered: