diff --git a/baseLib/src/main/java/me/ycdev/android/lib/common/utils/WeakListenerManager.kt b/baseLib/src/main/java/me/ycdev/android/lib/common/utils/WeakListenerManager.kt index 30b1d3a..1589890 100644 --- a/baseLib/src/main/java/me/ycdev/android/lib/common/utils/WeakListenerManager.kt +++ b/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 { fun notify(listener: IListener) } - private class ListenerInfo internal constructor(listener: IListener) { - internal var className: String = listener::class.java.name - internal var holder: WeakReference = WeakReference(listener) + private class ListenerInfo(listener: IListener) { + var className: String = listener::class.java.name + var holder: WeakReference = WeakReference(listener) } /** @@ -76,23 +78,27 @@ open class WeakListenerManager { 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> = 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" } diff --git a/baseLib/src/test/java/me/ycdev/android/lib/common/utils/WeakListenerManagerTest.kt b/baseLib/src/test/java/me/ycdev/android/lib/common/utils/WeakListenerManagerTest.kt new file mode 100644 index 0000000..e888170 --- /dev/null +++ b/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() +} \ No newline at end of file diff --git a/testLib/src/main/java/me/ycdev/android/lib/test/log/TimberJvmTree.kt b/testLib/src/main/java/me/ycdev/android/lib/test/log/TimberJvmTree.kt index e6692b8..dbaaf06 100644 --- a/testLib/src/main/java/me/ycdev/android/lib/test/log/TimberJvmTree.kt +++ b/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()) + } + } } diff --git a/testLib/src/main/java/me/ycdev/android/lib/test/rules/TimberJvmRule.kt b/testLib/src/main/java/me/ycdev/android/lib/test/rules/TimberJvmRule.kt index a5718ae..0e75d0b 100644 --- a/testLib/src/main/java/me/ycdev/android/lib/test/rules/TimberJvmRule.kt +++ b/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() } }