Fix WeakListenerManager and TimberJvmRule bugs

Author: yongce

baseLib/src/main/java/me/ycdev/android/lib/common/utils/WeakListenerManager.kt

@@ -1,5 +1,7 @@
 package me.ycdev.android.lib.common.utils
 
+import androidx.annotation.VisibleForTesting
+import timber.log.Timber
 import java.lang.ref.WeakReference
 import java.util.ArrayList
 
@@ -11,9 +13,9 @@ open class WeakListenerManager<IListener : Any> {
         fun notify(listener: IListener)
     }
 
-    private class ListenerInfo<IListener : Any> internal constructor(listener: IListener) {
-        internal var className: String = listener::class.java.name
-        internal var holder: WeakReference<IListener> = WeakReference(listener)
+    private class ListenerInfo<IListener : Any>(listener: IListener) {
+        var className: String = listener::class.java.name
+        var holder: WeakReference<IListener> = WeakReference(listener)
     }
 
     /**
@@ -76,23 +78,27 @@ open class WeakListenerManager<IListener : Any> {
 
     fun notifyListeners(action: (IListener) -> Unit) {
         synchronized(listeners) {
-            var i = 0
-            while (i < listeners.size) {
-                val listenerInfo = listeners[i]
+            // The listener may unregister itself!
+            val listenersCopied: List<ListenerInfo<IListener>> = ArrayList(listeners)
+            for ((i, listenerInfo) in listenersCopied.withIndex()) {
                 val l = listenerInfo.holder.get()
                 if (l == null) {
-                    LibLogger.e(TAG, "listener leak found: " + listenerInfo.className)
-                    listeners.removeAt(i)
+                    Timber.tag(TAG).w("listener leak found: %s", listenerInfo.className)
+                    listeners.remove(listenerInfo)
                 } else {
-                    LibLogger.d(TAG, "notify: " + listenerInfo.className)
+                    Timber.tag(TAG).d("notify #%d: %s", i, listenerInfo.className)
                     action(l)
-                    i++
                 }
             }
-            LibLogger.d(TAG, "notify done, cur size: " + listeners.size)
+            Timber.tag(TAG).d("notify done, cur size: %d in %s", listeners.size, this)
         }
     }
 
+    @VisibleForTesting
+    internal fun listenersCount(): Int {
+        return listeners.size
+    }
+
     companion object {
         private const val TAG = "WeakListenerManager"
     }

baseLib/src/test/java/me/ycdev/android/lib/common/utils/WeakListenerManagerTest.kt

@@ -0,0 +1,101 @@
+package me.ycdev.android.lib.common.utils
+
+import com.google.common.truth.Truth.assertThat
+import me.ycdev.android.lib.test.rules.TimberJvmRule
+import org.junit.Rule
+import org.junit.Test
+
+class WeakListenerManagerTest {
+    @get:Rule
+    val timberRule = TimberJvmRule()
+
+    @Test
+    fun basic() {
+        val manager = DemoListenerManager()
+        val listener1 = DemoListener(manager)
+        val listener2 = DemoListener(manager)
+
+        manager.addListener(listener1)
+        manager.addListener(listener2)
+
+        assertThat(manager.listenersCount()).isEqualTo(2)
+
+        manager.notifyListeners { l -> l.call(1) }
+        assertThat(listener1.value).isEqualTo(1)
+        assertThat(listener2.value).isEqualTo(1)
+
+        manager.notifyListeners { l -> l.call(2) }
+        assertThat(listener1.value).isEqualTo(2)
+        assertThat(listener2.value).isEqualTo(2)
+
+        assertThat(manager.listenersCount()).isEqualTo(2)
+    }
+
+    @Test
+    fun listenerLeak() {
+        val manager = DemoListenerManager()
+        val listener1 = DemoListener(manager)
+
+        manager.addListener(listener1)
+        addLeakedListener(manager)
+
+        // force GC
+        GcHelper.forceGc()
+
+        // before notify
+        assertThat(manager.listenersCount()).isEqualTo(2)
+
+        manager.notifyListeners { l -> l.call(1) }
+        assertThat(listener1.value).isEqualTo(1)
+
+        // after notify
+        assertThat(manager.listenersCount()).isEqualTo(1)
+
+        manager.notifyListeners { l -> l.call(2) }
+        assertThat(listener1.value).isEqualTo(2)
+    }
+
+    @Test
+    fun listenerRemovedWhenNotify() {
+        val manager = DemoListenerManager()
+        val listener1 = DemoListener(manager, true)
+        val listener2 = DemoListener(manager)
+
+        manager.addListener(listener1)
+        manager.addListener(listener2)
+
+        assertThat(manager.listenersCount()).isEqualTo(2)
+
+        manager.notifyListeners { l -> l.call(1) }
+        assertThat(listener1.value).isEqualTo(1)
+        assertThat(listener2.value).isEqualTo(1)
+
+        assertThat(manager.listenersCount()).isEqualTo(1)
+
+        manager.notifyListeners { l -> l.call(2) }
+        assertThat(listener1.value).isEqualTo(1)
+        assertThat(listener2.value).isEqualTo(2)
+
+        assertThat(manager.listenersCount()).isEqualTo(1)
+    }
+
+    private fun addLeakedListener(manager: DemoListenerManager) {
+        manager.addListener(DemoListener(manager))
+    }
+
+    class DemoListener(
+        private val manager: DemoListenerManager,
+        private val notifyOnce: Boolean = false
+    ) {
+        var value: Int = 0
+
+        fun call(value: Int) {
+            this.value = value
+            if (notifyOnce) {
+                manager.removeListener(this)
+            }
+        }
+    }
+
+    class DemoListenerManager : WeakListenerManager<DemoListener>()
+}

testLib/src/main/java/me/ycdev/android/lib/test/log/TimberJvmTree.kt

@@ -25,4 +25,16 @@ class TimberJvmTree : Timber.Tree() {
         println(log)
         t?.printStackTrace(System.out)
     }
+
+    companion object {
+        fun plantIfNeeded() {
+            // only plant TimberJvmTree once
+            Timber.forest().forEach {
+                if (it is TimberJvmTree) {
+                    return
+                }
+            }
+            Timber.plant(TimberJvmTree())
+        }
+    }
 }

testLib/src/main/java/me/ycdev/android/lib/test/rules/TimberJvmRule.kt

@@ -2,10 +2,9 @@ package me.ycdev.android.lib.test.rules
 
 import me.ycdev.android.lib.test.log.TimberJvmTree
 import org.junit.rules.ExternalResource
-import timber.log.Timber
 
 class TimberJvmRule : ExternalResource() {
     override fun before() {
-        Timber.plant(TimberJvmTree())
+        TimberJvmTree.plantIfNeeded()
     }
 }