stephanenicolas / toothpick

A scope tree based Dependency Injection (DI) library for Java / Kotlin / Android.
Apache License 2.0
1.12k stars 115 forks source link

Memory leak in Sample #416

Open afaucogney opened 4 years ago

afaucogney commented 4 years ago

Hi, I'm trying to combine KTP with LeakCanary to secure that there is no leak in project, even with KTP. Based on the sample, I just add LeakCanary dep, and it founds a leak on AddNewActivity because of BackpackApplication.scope.

Here is the message:

┬─── │ GC Root: System class │ ├─ android.provider.FontsContract class │ Leaking: NO (BackpackApplication↓ is not leaking and a class is never leaking) │ ↓ static FontsContract.sContext ├─ com.example.toothpick.BackpackApplication instance │ Leaking: NO (Application is a singleton) │ ↓ BackpackApplication.scope │ ~ ├─ toothpick.ScopeImpl instance │ Leaking: UNKNOWN │ ↓ ScopeImpl.childrenScopes │ ~~~~~~ ├─ java.util.concurrent.ConcurrentHashMap instance │ Leaking: UNKNOWN │ ↓ ConcurrentHashMap.table │ ~ ├─ java.util.concurrent.ConcurrentHashMap$Node[] array │ Leaking: UNKNOWN │ ↓ ConcurrentHashMap$Node[].[0] │ ~~~ ├─ java.util.concurrent.ConcurrentHashMap$Node instance │ Leaking: UNKNOWN │ ↓ ConcurrentHashMap$Node.key │ ~~~ ╰→ com.example.toothpick.activity.AddNewActivity instance ​ Leaking: YES (ObjectWatcher was watching this because com.example.toothpick.activity.AddNewActivity received Activity#onDestroy() callback and Activity#mDestroyed is true) ​ key = 712a718b-30c6-441c-a03e-52fba6adfdbb ​ watchDurationMillis = 51699 ​ retainedDurationMillis = 46697

METADATA

Build.VERSION.SDK_INT: 27 Build.MANUFACTURER: Google LeakCanary version: 2.4 App process name: com.example.kotlin Analysis duration: 11393 ms

Is this normal ?

dlemures commented 4 years ago

That report is valid, the other 3 activities on the sample are fine. But we forgot to close the scope on that one:

https://github.com/stephanenicolas/toothpick/blob/master/toothpick-sample/src/main/java/com/example/toothpick/activity/AddNewActivity.java#L30

Will keep the issue open open as a bug to fix.

Thx Anthony!