Store icon indicating copy to clipboard operation
Store copied to clipboard

Add removal listener + build on local cache module

Open MichaelOliveiraBx opened this issue 3 years ago • 3 comments

I need to know when an item is remove from the cache, so I add the possibility to put an listener in the MemoryPolicy wich be add to the CacheBuilder There are 2 point where I need your advice

  • I must retrieve the RemovalCause in the RemovalNotification but actually the project is base on cache3 host on GitHub, not in the local module of cache. We can't make PR on this project it deprecated and in read only. So I have remove this repo and use directly the local code of cache to be able to maintain
  • Actually add the listener will be leak because we must can remove it. But we can't actually with an builder. There is some option to handle it, we must the possibility to clear the listener in the Cache, and after either add an destroy function in the Store call by the user or use the coroutine scope and catch when an Cancellable are emit to remove the listener

I'm very interested in hearing your opinions on this.

MichaelOliveiraBx avatar Aug 30 '22 15:08 MichaelOliveiraBx

Thank you for the contribution! I like the coroutine scope idea let's so what others think

Sorry about artifacts. We are mid rewriting to kmp and didn't publish the new version yet. Makes sense to run against source for now.

Could you try the scope idea and also add tests to make sure no leak?

digitalbuddha avatar Aug 30 '22 16:08 digitalbuddha

Yes ok, I will try that and back to you. Thanks

MichaelOliveiraBx avatar Aug 30 '22 16:08 MichaelOliveiraBx

Hi @digitalbuddha, I push a new commit to try to handle the listener lifecycle with coroutine scope. But I come across other problems, the library throw exception when we access to it when the scope is cancel. See the test, it return

Job was cancelled
kotlinx.coroutines.JobCancellationException: Job was cancelled; job=JobImpl{Cancelled}@5e2be7d5
	at app//kotlinx.coroutines.JobSupport.cancel(JobSupport.kt:1578)
	at app//kotlinx.coroutines.CoroutineScopeKt.cancel(CoroutineScope.kt:287)
	at app//kotlinx.coroutines.CoroutineScopeKt.cancel$default(CoroutineScope.kt:285)
	at app//com.dropbox.android.external.store4.impl.StoreWithInMemoryCacheTest$store requests with removal listener when scope is cancel$1.invokeSuspend(StoreWithInMemoryCacheTest.kt:94)
	(Coroutine boundary)
	at com.dropbox.android.external.store4.impl.FetcherController$getFetcher$1.invokeSuspend(FetcherController.kt:93)
	at kotlinx.coroutines.flow.AbstractFlow.collect(Flow.kt:212)
	at kotlinx.coroutines.flow.FlowKt__EmittersKt$onStart$$inlined$unsafeFlow$1.collect(Emitters.kt:120)
	at com.dropbox.android.external.store4.impl.RealStore$stream$1$invokeSuspend$$inlined$transform$1.invokeSuspend(RealStore.kt:223)
	at kotlinx.coroutines.flow.AbstractFlow.collect(Flow.kt:212)
	at com.dropbox.android.external.store4.impl.RealStore$stream$1.invokeSuspend(RealStore.kt:122)
	at kotlinx.coroutines.flow.AbstractFlow.collect(Flow.kt:212)
	at kotlinx.coroutines.flow.FlowKt__ReduceKt.first(Reduce.kt:183)
	at com.dropbox.android.external.store4.StoreKt.get(Store.kt:70)

Just it is difficult to test that the listener has been remove There is some solution:

  • catch exception in some place in the library to print an error instead of throw (need some stuff)
  • let the user use correctly the store and only put an try catch in the test (maybe best solution to start)

MichaelOliveiraBx avatar Aug 31 '22 08:08 MichaelOliveiraBx