Kotlin - lateinit memory leak issues

[ZakTaccardi]

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.

class SalesController : BaseController, SalesView {
    @Inject lateinit var viewBinder: SalesController.ViewBinder
    @Inject lateinit var renderer: SalesRenderer
    @Inject lateinit var presenter: SalesPresenter

    lateinit private var component: SalesScreenComponent

    override var state = SalesScreen.State.INITIAL  //only property that I want to survive config changes

    fun onCreateView(): View {  /** lateinit variables are set here */ }
    fun onDestroyView() { /** lateinit variables need to be dereferenced here, or we have a memory leak  */ }
}

My lateinit properties are injected by dagger, and I need to set them to null in onDestroyView - 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

[EricKuck]

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 @JvmField, then use java to set the backing field itself to null without Kotlin's knowledge, but that feels very hacky.

[EricKuck]

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.

[ZakTaccardi]

You just can't set a property to null unless it's an optional.

You're right, my bad

mark it as a @JvmField

@JvmField cannot be applied to lateinit property

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?

[ZakTaccardi]

I believe that these properties should simply be optional

For my navigation drawer, I have a drawer: DrawerLayout? = null because it will exist in a phone configuration, but not exist in the tablet configuration.

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 onCreateView/onDestoryView.

[nomisRev]

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 activity's 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. (correct me if I'm missing something)

No nullable types, only for actual nullable dependencies.

[sockeqwe]

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 https://github.com/bluelinelabs/Conductor/issues/234#issuecomment-281805315, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnrhmTGcxIQPxPrvIQFVIsuR0d_6BPks5rfKSCgaJpZM4MI43d .

[nomisRev]

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
    }
}

[sockeqwe]

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 https://github.com/bluelinelabs/Conductor/issues/234#issuecomment-281809672, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnrjbum1qiVCnIpwY2nvdvHSjXRUIpks5rfKiDgaJpZM4MI43d .

[sockeqwe]

Instead in the cause that controller A was removed by gc, Conductor would simply instate a new controller using reflection.

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 controller.onDestroy() is only called when the Controller has been removed permanently from back stack (i.e. the top most controller is removed once the user presses the back button).

And for that reason we use no-arg and bundle constructors

The bundle constructor is only used to restore a controller after a process death.

[nomisRev]

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 onDestroyView?

I realise it is just avoiding dealing with nullable dependencies, but those dependencies shouldn't be used in a longer lifecycle than onCreateView/onDestroyView. Now I'm quite unsure what the best approach is.

I use the same delegate for Kotterknife, but I feel that throwing an IllegalStateException when trying to use a view outside of the onCreateView/onDestroyView lifecycle makes sense for views.

[dimsuz]

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.

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...

[ZakTaccardi]

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 by Ref(ref) delegate.

abstract class BaseController : Controller() {

    val ref = ResettableReferencesManager()

    @CallSuper
    override fun onDestroyView(view: View) {
        ref.reset()
        super.onDestroyView(view)
    }
}

/**
 * Manages the list of references to reset. This is useful in Conductor's controllers to automatically clear
 * references in [Controller.onDestroyView]. References scoped to the view's lifecycle must be cleared
 * because Conductor's [Controller]s survive configuration changes
 */
class ResettableReferencesManager {
    private val delegates = mutableListOf<Ref<*, *>>()

    fun register(delegate: Ref<*, *>) {
        delegates.add(delegate)
    }

    fun reset() {
        delegates.forEach {
            it.reset()
        }
    }
}

/**
 * Kotlin delegate to automatically clear (nullify) strong references.
 */
class Ref<in R, T : Any>(manager: ResettableReferencesManager) {
    init {
        manager.register(this)
    }

    private var value: T? = null

    operator fun getValue(thisRef: R, property: KProperty<*>): T =
            value ?: throw UninitializedPropertyAccessException(property.name)

    operator fun setValue(thisRef: R, property: KProperty<*>, t: T) {
        value = t
    }

    fun reset() {
        value = null
    }
}

abstract class ExampleController : BaseController() {
    @set:Inject var presenter: HistoryPresenter by Ref(ref)

    /**
     * Binds views w/ butterknife. Lateinit won't work with a delegate.
     */
    private var viewBinder: ExampleController.ViewBinder by Ref(ref)

    /**
     * Named injections won't work. use kotlin property `get()` to workaround it
     */
    private val swipeRefresh get() = viewBinder.swipeRefresh

    private val recycler get() = viewBinder.recycler

    val state: State = State.INITIAL_VALUE //will survive config changes
}

[mradzinski]

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 by Ref(ref) naming xD (RefRef is a DDoS attack tool, lol).

[ZakTaccardi]

@mradzinski yeah I wasn't sure what to name it - trying to keep it as terse as possible. by Resettable(manager) seems a little verbose - but I'm open to any ideas that you may have

[EricKuck]

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!

[sockeqwe]

One could also write a ResetableReference without a "manager". Just add a Controller LifecycleListener internally in ResetableReference.

[sevar83]

What about this?

abstract class BaseController(args: Bundle) : Controller(args) {

    /**
     * Kotlin delegate to automatically clear (nullify) strong references.
     */
    protected inner class Ref<in R, T : Any> : Controller.LifecycleListener() {

        init {
            this@BaseController.addLifecycleListener(this)
        }

        private var value: T? = null

        operator fun getValue(thisRef: R, property: KProperty<*>): T =
                value ?: throw UninitializedPropertyAccessException(property.name)

        operator fun setValue(thisRef: R, property: KProperty<*>, t: T) {
            value = t
        }

        override fun postDestroyView(controller: Controller) {
            reset()
        }

        fun reset() {
            value = null
            this@BaseController.removeLifecycleListener(this)
        }
    }
}

class ExampleController(args: Bundle) : BaseController(args) {
    @set:Inject var presenter: ExamplePresenter by Ref()
    // ...
}

[ZakTaccardi]

https://youtrack.jetbrains.com/issue/KT-19621

[PaulWoitaschek]

@sevar83

If you remove the LifeCycleListener upon postDestroyView, then when the view gets re-created, it will cause a memory leak the next time. So instead you should just leave the listener attached and let it get carbage collected with the controller.

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.

[sevar83]

Hi @PaulWoitaschek, thanks for the feedback. Do you mean the controller is leaked? I don't see how removeLifecycleListener() causes the controller to leak. Both the controller and the property hold references to each other, so GC can release them both. A call to removeLifecycleListener() shouldn't make a difference. I've just added it for symmetry. I've done a simple rotation test with MAT. Only 1 instance of the controller is held after the rotation.

[PaulWoitaschek]

You create the property only once. When now postDestroyView gets called, you remove the listener, but the property stays the same. Now the view gets re-created and the reference gets set. When you now change the orientation again, postDestroyView won't get called because you already removed the listener. Therefore the reference is hold.

When you now push another controller on top, the view gets destroyed, but the reference stays.

[sevar83]

Good news. Half of the proposal is going to be implemented in Kotlin 1.2 and the rest in 1.3. https://blog.jetbrains.com/kotlin/2017/09/kotlin-1-2-beta-is-out/ Check the lateinit improvements section.

[mradzinski]

@sevar83

the remaining part (the deinitialize method) has been postponed, tentatively until 1.3.

Until we can deinit we're still stuck with the same memory leak (lateinit var = null will not be allowed until the deinit method exists). Good news is that we can at least check if a lateinit has been initialized which is absolutely awesome :)

Waiting till 1.3 to come out some day next year to get rid of this hacks, lol

[PaulWoitaschek]

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.

[1gravity]

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.

[ZakTaccardi]

@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..

[1gravity]

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.