Implement caching Toggle.Store and Toggle@enabled() flow API #7019
+624
−4
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| /* | ||
| * Copyright (c) 2025 DuckDuckGo | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package com.duckduckgo.feature.toggles.api.internal | ||
|
|
||
| import com.duckduckgo.feature.toggles.api.Toggle | ||
| import com.duckduckgo.feature.toggles.api.Toggle.State | ||
| import com.google.common.cache.CacheBuilder | ||
| import com.google.common.cache.CacheLoader | ||
| import com.google.common.cache.LoadingCache | ||
| import org.jetbrains.annotations.TestOnly | ||
| import java.util.Optional | ||
| import kotlin.jvm.optionals.getOrNull | ||
|
|
||
| class CachedToggleStore constructor( | ||
| private val store: Toggle.Store, | ||
| ) : Toggle.Store { | ||
| @Volatile | ||
| private var listener: Listener? = null | ||
|
|
||
| private val cache: LoadingCache<String, Optional<State>> = | ||
| CacheBuilder | ||
| .newBuilder() | ||
| .maximumSize(50) | ||
| .build( | ||
| object : CacheLoader<String, Optional<State>>() { | ||
| override fun load(key: String): Optional<State> = Optional.ofNullable(store.get(key)) | ||
| }, | ||
| ) | ||
|
|
||
| override fun set( | ||
| key: String, | ||
| state: State, | ||
| ) { | ||
| cache.asMap().compute(key) { k, _ -> | ||
| store.set(k, state) | ||
| Optional.of(state) | ||
| } | ||
| // Notify AFTER compute() to avoid deadlocks or re-entrancy into the cache/store. | ||
| // If the store.set() above throws, this never runs (which is what we want). | ||
| // Swallow listener exceptions so they don't break writes. | ||
| listener?.runCatching { onToggleStored(state) } | ||
| } | ||
|
|
||
| /** | ||
| * Registers a [Listener] to observe changes in toggle states stored by this [CachedToggleStore]. | ||
| * | ||
| * Only a single listener is supported at a time. When a new listener is set, it replaces any | ||
| * previously registered listener. To avoid memory leaks, callers should always invoke the returned | ||
| * unsubscribe function when the listener is no longer needed (for example, when the collector | ||
| * of a [kotlinx.coroutines.flow.callbackFlow] is closed). | ||
| * | ||
| * Example usage: | ||
| * ``` | ||
| * val unsubscribe = cachedToggleStore.setListener(object : CachedToggleStore.Listener { | ||
| * override fun onToggleStored(newValue: Toggle.State, oldValue: Toggle.State?) { | ||
| * // React to state change | ||
| * } | ||
| * }) | ||
| * | ||
| * // Later, when no longer interested: | ||
| * unsubscribe() | ||
| * ``` | ||
| * | ||
| * @param listener the [Listener] instance that will receive callbacks for each `set` operation. | ||
| * @return a function that removes the listener when invoked. The returned function is safe to call | ||
| * multiple times and will only clear the listener if it is the same instance that was | ||
| * originally registered. | ||
| */ | ||
| fun setListener(listener: Listener?): () -> Unit { | ||
| this.listener = listener | ||
|
|
||
| return { if (this.listener === listener) this.listener = null } | ||
| } | ||
|
|
||
| /** | ||
| * DO NOT USE outside tests | ||
| */ | ||
| @TestOnly | ||
| fun invalidateAll() { | ||
| cache.invalidateAll() | ||
| } | ||
|
|
||
| override fun get(key: String): State? { | ||
| val value = cache.get(key).getOrNull() | ||
| if (value == null) { | ||
| // avoid negative caching | ||
| cache.invalidate(key) | ||
| } | ||
|
|
||
| return value | ||
| } | ||
|
|
||
| interface Listener { | ||
| fun onToggleStored(newValue: State) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a coroutine context isn't specified, we'll use the same one that the consumer has specified (which in practice, might be
Mainmost of he time. Shoud we docallbackFlow {...}.flowOn(Dispatchers.IO)? In practice, if the value is cached (and it should) it might not make much of a difference, but I think it still doesn't hurt to take some load off the main thread regardlessThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong opinions, that also has the issue that -- other than API documentation -- it's hard to make the caller aware of it, it can happen the caller is not and then it tries to refresh the UI in IO, which will crash.
That said, I can add it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the caller doesn't specify its own context when calling the coroutine and they use
appCoroutineScopeinstead oflifecycleScopethen yes, it will crash. That being said, I'd rather have the app crashing in that case (which will be very hard to miss during implementation and/or automated testing) than have the app silently run non-UI work in the main threadThe context we set in
flowOnwill only apply to that firsttrySendblock, though, as the second one is within a callback, so if we want to also set the context there, we'd need to launch a coroutine