I'm wondering if this lambda use creates a potenti...

# android

[Michael Pohl]

I'm wondering if this lambda use creates a potential memory leak:

class SomeFragment: Fragment() {
    
    val myVal: SomeClass by lazy { 
        SomeClass { requireContext().packageName }
    }

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        myVal.runLambda()
        
    }
}

class SomeClass(
    private val willBeRunSomeTimeInTheFuture: () -> String) {
    
    fun runLambda() {
        Thread.sleep(1000)
        println(willBeRunSomeTimeInTheFuture())
    }
}

Trying to wrap my head around if I have to worry when using requireContext in a lambda that might get executed at a point the fragment is potentially already gone again, and what to do about it. If this is problematic (happy to hear why exactly), would a WeakReference be enough to mitigate this, like below?

WeakReference(requireContext()).get()?.packageName ?: ""

Or do I completely misunderstand something here?
Let me know if this example or my question is not sufficient to understand the problem.

[ephemient]

yes, you may have that issue. if you change it to

lazy {
    val context = requireContext()
    SomeClass { context.packageName }
}

or

lazy {
    SomeClass(requireContext()::packageName)
}

then you'll leak the context instead, but it will be present

[ephemient]

if you want to be safe, why not use the nullable getContext()?

[ephemient]

or since the package name never changes, just get it from the application context earlier

[Michael Pohl]

The packageName is just an example. I chose it since it's a String and thus helped me write up something faster 🙂

I would like to leak nothing, ideally - how do I do it?

[Juliane Lehmann]

I was just confused for a bit and took a minute to sort out what happens, and maybe I'm not the only one:
1. The original version has no memory leak, but will crash when the fragment is not attached anymore at the point where the lambda executes. Ways around that as suggested by ephemient.
2. The alternate versions that bind the context immediately won't crash, but leak it. That's not good.
3. You could mitigate the leak in 2 in general by using the WeakReference-construction around the context bound in the lambda - that works.
4. For the specific example, I'd go with 1. In general, 3 is a solution.

[Juliane Lehmann]

No wait, I'm confused - didn't think about the effect of by lazy. It doesn't matter at all whether you bind the context to a separate name inside the lambda or not.

[Juliane Lehmann]

If I understand your general example correctly, I'd go with:

class SomeClass(
  private val contextRef: WeakReference<Context>
) 
  fun runLambda() {
    Thread.sleep(1000)
    contextRef.get()?.let {
      // do something with the context if it's still there. If not, we don't care anymore.
    }
  } 

class SomeFragment : Fragment() {
  lateinit var myVal: SomeClass

  override fun onAttach(context: Context) {
    myVal = SomeClass(WeakReference(context))
  }
}

[Michael Pohl]

But if I pass in requireContext() , doesn't my SomeClass instance need to keep a reference to my fragment, possibly preventing it from being garbage collected?

[ephemient]

if the only thing holding onto SomeClass is the fragment, then they can all be GC'ed at once. cycles aren't a problem

[ephemient]

but there's really no benefit to using WeakReference(requireContext()) over a simple getContext() (the nullable one, not requireContext())

[Juliane Lehmann]

If it were an Activity, I'd agree, but can't Fragments detach from context without dying and reattach later?

[Robert Williams]

Assuming you wanted to start a thread in SomeClass, that Thread will also leak the Context (currently it’s just blocking main thread which I guess is not the expected behaviour)

[Robert Williams]

But yeah, WeakReference is completely useless if you’re just going to call get() immediately

[Robert Williams]

Would suggest using Lifecycle or Coroutines with lifecycleScope to cancel the work when you no longer need it

[Michael Pohl]

Ok, I thought a simplified example might help figure out the problem better, but I'm not sure that's the case.
This is my actual use case:

fun <T> ViewModelStoreOwner.componentStore(
    componentProvider: () -> T
) = lazy {
    ViewModelProvider(
        this,
        object : ViewModelProvider.Factory {
            override fun <VM : ViewModel> create(modelClass: Class<VM>): VM {
                return ComponentStore(
                    componentProvider(),
                    doOnClear
                ) as VM
            }
        }
    )[ComponentStore::class.java].component as T
}

private class ComponentStore<T> constructor(
    val component: T,
) : ViewModel()

class MyFragment: Fragment() {

    private val component: SomeComponent by componentStore(
        componentProvider = {
            requireContext().getAppComponentAs<SomeComponent.Factory>()
                .createSomeComponent()
        },
        doOnClear = {
            MyFragmentViewModel().destroy()
        }
    )
    
}

I have this delegate function by componentStore() that stores a Dagger component in a generic store (which extends ViewModel, so it potentially lives longer than my fragment.

There is a extension Context.getAppComponentAs() (which is why I use requireContext() ) that retrieves the application component as a factory interface - that's so I have access to the application component without actually knowing it.

There also is a doOnClear() function that gets run

So I want to understand if this use of requireContext() is safe. If it is not, how can I make it safe? Not sure I can safely hook this up to any kind of LifeCycle-related things...?

[Robert Williams]

In terms of memory, no issues because ComponentStore and component don’t retain Context and everything else is Fragment scoped

[Robert Williams]

In terms of crashing it’s probably fine as long as you first reference component between onAttach and onDetach

[Robert Williams]

In terms of doing what you expect you might run into problems because ComponentStore is generic but you can’t actually have more than one in the same lifecycle component

[Robert Williams]

In terms of if it’s the best solution, Hilt will be much simpler to do the same thing unless you have some very specific constraints on your dagger implementation

[Michael Pohl]

Your 2nd and 3rd points are under control, so I'm not worried there. Just trying to wrap my head around the memory thing. So I won't have a reference to my context or fragment unless I store it in a field in my store?

[Michael Pohl]

And yes, Hilt is no option, very specific constraints indeed 😄

[Robert Williams]

This shouldn’t be that surprising because you’re extending ViewModel and by design ViewModels don’t reference the Context precisely to avoid leaking it

[Robert Williams]

If Hilt isn’t an option is it possible to have a custom Dagger Component which is created and destroyed with the Lifecycle?

[Michael Pohl]

That might be a better option, but this is a pattern that is all over a really large app, so com ing up with a new solution is out of the question for this problem...
I'm just trying to understand if it is memory safe. I was actually not worried about the store, since, as you say it extends ViewModel, but I'm capuring these two functions that I pass in to my lazy function and from what I understand (which might be wrong) this creates an implicit reference to my fragment. Obviously I don't understand it all fully 🙂

[Robert Williams]

Yep, the Delegate created by by lazy will have a reference to the Fragment but it’s also only referenced from the Fragment so will be cleaned up when the Fragment is destroyed

[Michael Pohl]

That makes sense. Thank you for helping me see this a bit clearer!