Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add removal listener + build on local cache module #442

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MichaelOliveiraBx
Copy link

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.

@digitalbuddha
Copy link
Contributor

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?

@MichaelOliveiraBx
Copy link
Author

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

@MichaelOliveiraBx
Copy link
Author

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants