gouline / kapsule

Minimalist Kotlin dependency injection
MIT License
164 stars 7 forks source link

Possible leak in CallerMap? #4

Closed sindrenm closed 7 years ago

sindrenm commented 7 years ago

Hi!

Really great stuff here, love how simple it is to use and how concise the code base is compared to other dependency injection libraries out there.

However, looking through your code, I noticed you’re extending WeakHashMap to allow for a simple caching mechanism of the last value stored, and I couldn’t help myself but wondering if keeping this reference outside of the “actual” map implementation could result in a leak somehow. Imagine the following:

class HomeModule(private val activity: HomeActivity) {
  // EDITED: Typo had this return a HomeModule before ...
  val model by lazy { HomeModel(activity) }
}

Now, in our activity:

class HomeActivity : Activity, Injects<HomeModule> {
  private val model by required { model }
}

So far so good. However, when we move away from the HomeActivity, and given that we don’t do any injection in our next activity, then there will be a hard reference to the HomeModule, and therefore also the HomeActivity, which won’t get collected by the garbage collector.

I suppose it's only a valid issue for the last stored key/value pair, seeing as you're extending WeakHashMap.I suspect that's also why you did just that.

Now, if you already thought about this for the last key/value pair as well and have a solution already in your codebase, then please correct me. :smile:

gouline commented 7 years ago

Hi @sindrenm! Thanks for your issue.

Correct me if misunderstood your question, but the CallerMap doesn't actually store a reference to the HomeModule itself, it only stores a reference to the Kapsule<HomeModule>, which is just a lightweight injector. It never stores the module during injection, it's only used once in the inject() method, to initialise the values in the delegates, and then released.

Not entirely sure why your model is returning HomeModule type though. Is that a typo or am I misunderstanding your example?

sindrenm commented 7 years ago

Hey!

Not entirely sure why your model is returning HomeModule type though.

Yeah, sorry, that was a typo. I've edited the original post. Sorry about that. :stuck_out_tongue:

Regarding the reference issue, you're absolutely right. I don't know how I misunderstood that before, but looking at the code now it's pretty clear. I did have a memory leak, however, and I traced it back to being kept by the lastKey (or possibly lastValue, can't quite recall), but then that is probably all my fault. All you're doing with the module is looping through delegates and initializing them.

I'll close this and fix up my code. :stuck_out_tongue:

Thanks for your reply. :grinning: