From 9b298e58dc3c22790559aa35dd313640e3ae79f0 Mon Sep 17 00:00:00 2001 From: Marc Lefrancois Date: Mon, 8 Jan 2024 14:35:34 -0500 Subject: [PATCH] Fix possibly huge memory leak caused by long lived CancellableManagerProvider (#213) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix forever growing cancellable Manager A long lived CancellableManagerProvider could easily end up with very many “cancelled” Cancellable in it’s internal CancellableManager because this is only cleared when the Provider itself is cancelled. This can cause huge memory leaks in some cases, for example in the DebounceProcessor Since there is only ever one “not cancelled” CancellableManager provided by the CancellableManagerProvider, there is actually no need to keep the previously provided, now cancelled, CancellableManager. We now simply use the internalCancellableManagerRef. Also took the oportunity to make cancelPreviousAndCreate hopefully atomic be exposing and using getAndSet on AtomicReference * Add test for “already cancelled” contract and implement * remove another hole * Update public API declaration --- .../api/android/trikotFoundation.api | 1 + .../api/jvm/trikotFoundation.api | 1 + .../foundation/concurrent/AtomicReference.kt | 4 ++++ .../cancellable/CancellableManagerProvider.kt | 21 +++++++++++++------ .../CancellableManagerProviderTests.kt | 11 ++++++++++ 5 files changed, 32 insertions(+), 6 deletions(-) diff --git a/trikot-foundation/trikotFoundation/api/android/trikotFoundation.api b/trikot-foundation/trikotFoundation/api/android/trikotFoundation.api index 659d0b07e..e2f4d48e5 100644 --- a/trikot-foundation/trikotFoundation/api/android/trikotFoundation.api +++ b/trikot-foundation/trikotFoundation/api/android/trikotFoundation.api @@ -203,6 +203,7 @@ public final class com/mirego/trikot/foundation/concurrent/AtomicReference { public fun (Ljava/lang/Object;)V public final fun compareAndSet (Ljava/lang/Object;Ljava/lang/Object;)Z public final fun compareAndSwap (Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object; + public final fun getAndSet (Ljava/lang/Object;)Ljava/lang/Object; public final fun getValue ()Ljava/lang/Object; public final fun setOrThrow (Ljava/lang/Object;Ljava/lang/Object;)V public final fun setOrThrow (Ljava/lang/Object;Ljava/lang/Object;Lkotlin/jvm/functions/Function0;)V diff --git a/trikot-foundation/trikotFoundation/api/jvm/trikotFoundation.api b/trikot-foundation/trikotFoundation/api/jvm/trikotFoundation.api index 659d0b07e..e2f4d48e5 100644 --- a/trikot-foundation/trikotFoundation/api/jvm/trikotFoundation.api +++ b/trikot-foundation/trikotFoundation/api/jvm/trikotFoundation.api @@ -203,6 +203,7 @@ public final class com/mirego/trikot/foundation/concurrent/AtomicReference { public fun (Ljava/lang/Object;)V public final fun compareAndSet (Ljava/lang/Object;Ljava/lang/Object;)Z public final fun compareAndSwap (Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object; + public final fun getAndSet (Ljava/lang/Object;)Ljava/lang/Object; public final fun getValue ()Ljava/lang/Object; public final fun setOrThrow (Ljava/lang/Object;Ljava/lang/Object;)V public final fun setOrThrow (Ljava/lang/Object;Ljava/lang/Object;Lkotlin/jvm/functions/Function0;)V diff --git a/trikot-foundation/trikotFoundation/src/commonMain/kotlin/com/mirego/trikot/foundation/concurrent/AtomicReference.kt b/trikot-foundation/trikotFoundation/src/commonMain/kotlin/com/mirego/trikot/foundation/concurrent/AtomicReference.kt index 8ceee6c0b..56a69a945 100644 --- a/trikot-foundation/trikotFoundation/src/commonMain/kotlin/com/mirego/trikot/foundation/concurrent/AtomicReference.kt +++ b/trikot-foundation/trikotFoundation/src/commonMain/kotlin/com/mirego/trikot/foundation/concurrent/AtomicReference.kt @@ -23,6 +23,10 @@ class AtomicReference(value: T) { fun compareAndSwap(expected: T, new: T): T { return if (compareAndSet(expected, new)) new else value } + + fun getAndSet(new: T): T { + return atomicValue.getAndSet(new) + } } fun AtomicReference.setOrThrow(new: T) = setOrThrow(value, new) diff --git a/trikot-streams/streams/src/commonMain/kotlin/com/mirego/trikot/streams/cancellable/CancellableManagerProvider.kt b/trikot-streams/streams/src/commonMain/kotlin/com/mirego/trikot/streams/cancellable/CancellableManagerProvider.kt index e1e27ccde..ae521ef4e 100644 --- a/trikot-streams/streams/src/commonMain/kotlin/com/mirego/trikot/streams/cancellable/CancellableManagerProvider.kt +++ b/trikot-streams/streams/src/commonMain/kotlin/com/mirego/trikot/streams/cancellable/CancellableManagerProvider.kt @@ -1,20 +1,29 @@ package com.mirego.trikot.streams.cancellable import com.mirego.trikot.foundation.concurrent.AtomicReference +import com.mirego.trikot.foundation.concurrent.dispatchQueue.SynchronousSerialQueue class CancellableManagerProvider : Cancellable { - private val cancellableManager = CancellableManager() + private val serialQueue = SynchronousSerialQueue() private val internalCancellableManagerRef = AtomicReference(CancellableManager()) + private val isCancelled = AtomicReference(false) fun cancelPreviousAndCreate(): CancellableManager { - internalCancellableManagerRef.value.cancel() - return CancellableManager().also { - internalCancellableManagerRef.setOrThrow(internalCancellableManagerRef.value, it) - cancellableManager.add(it) + return CancellableManager().also { cancellableManager -> + internalCancellableManagerRef.getAndSet(cancellableManager).cancel() + serialQueue.dispatch { + if (isCancelled.value) { + cancellableManager.cancel() + } + } } } override fun cancel() { - cancellableManager.cancel() + serialQueue.dispatch { + if (isCancelled.compareAndSet(isCancelled.value, true)) { + internalCancellableManagerRef.value.cancel() + } + } } } diff --git a/trikot-streams/streams/src/commonTest/kotlin/com/mirego/trikot/streams/cancellable/CancellableManagerProviderTests.kt b/trikot-streams/streams/src/commonTest/kotlin/com/mirego/trikot/streams/cancellable/CancellableManagerProviderTests.kt index 21a9a21ee..090d0020a 100644 --- a/trikot-streams/streams/src/commonTest/kotlin/com/mirego/trikot/streams/cancellable/CancellableManagerProviderTests.kt +++ b/trikot-streams/streams/src/commonTest/kotlin/com/mirego/trikot/streams/cancellable/CancellableManagerProviderTests.kt @@ -24,4 +24,15 @@ class CancellableManagerProviderTests { assertTrue { cancelled } } + + @Test + fun requestingACancellableManagerFromACancelledProviderReturnsACancelledCancellableManager() { + val cancellableManagerProvider = CancellableManagerProvider() + cancellableManagerProvider.cancel() + val cancellableManager = cancellableManagerProvider.cancelPreviousAndCreate() + var cancelled = false + cancellableManager.add { cancelled = true } + + assertTrue { cancelled } + } }