Conversation
…he initializer Introduce a CachedFlagStore interface in the subsystems package that provides read access to cached flag data by evaluation context. Add this as a nullable field to DataSourceBuildInputs and wire it through from FDv2DataSourceBuilder using PerEnvironmentData. This plumbing enables the upcoming FDv2 cache initializer to load persisted flags without depending on package-private types. Made-with: Cursor
…uilderImpl Add FDv2CacheInitializer that loads persisted flag data from the local cache as the first step in the initializer chain. Per CONNMODE 4.1.2, the result uses Selector.EMPTY and persist=false so the orchestrator continues to the polling initializer for a verified selector. Cache miss and no-store cases return interrupted status to move on without delay. Add CacheInitializerBuilderImpl in DataSystemComponents and comprehensive tests covering cache hit, miss, no store, exceptions, and shutdown behavior. Made-with: Cursor
Prepend the cache initializer to all connection modes per CONNMODE 4.1.1. Every mode now starts with a cache read before any network initializer, providing immediate flag values from local storage while the polling initializer fetches fresh data with a verified selector. Update initializer count assertions in DataSystemBuilderTest and FDv2DataSourceBuilderTest to reflect the new cache initializer. Made-with: Cursor
aaron-zeisler
force-pushed
the
aaronz/SDK-2070/cache-initializer
branch
from
363887c to
563ec22
Compare
...ndroid-client-sdk/src/main/java/com/launchdarkly/sdk/android/subsystems/CachedFlagStore.java
Outdated
Show resolved
Hide resolved
| * A cache miss is reported as an {@link FDv2SourceResult.Status#interrupted} status, | ||
| * causing the orchestrator to move to the next initializer without delay. | ||
| */ | ||
| final class FDv2CacheInitializer implements Initializer { |
There was a problem hiding this comment.
Here's a summary of how the spec requirements and the js-core implementation were used when developing this class:
| Decision | Source | Reasoning |
|---|---|---|
Selector.EMPTY on cache result |
CONNMODE 4.1.2 | Cache is unverified; empty selector tells the orchestrator to continue to the polling initializer for a real selector |
persist=false on ChangeSet |
CONNMODE 4.1.2 | Don't re-write data we just read from cache |
Cache miss = interrupted |
js-core pattern | Fast failure so the orchestrator immediately moves on; interrupted is the correct signal (not terminalError, which would stop the chain) |
fdv1Fallback=false always |
Logic | Cache is local storage, no server headers are involved |
Nullable cachedFlagStore |
Testing pragmatism | Test contexts don't have a persistent store; graceful degradation avoids test setup burden |
In FDv2, the FDv2CacheInitializer handles cache loading as the first step in the initializer chain, making the cache load in ContextDataManager.switchToContext() redundant. Add a skipCacheLoad parameter to ContextDataManager and a setCurrentContext() method so that the FDv2 path sets the context without reading from cache, while the FDv1 path continues to load cached flags immediately. Made-with: Cursor
launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/LDClient.java
Outdated
Show resolved
Hide resolved
|
@tanderson-ld The 5th (and currently last) commit updates the code in LDClient and ContextDataManager. In FDv1, flags are loaded from the cache in ContextDataManager's constructor and also in LDClient's "identify" flow. But in FDv2, loading from cache in those places is redundant now that we have the cache initializer. I could defer this commit to a separate pull request if you feel that's cleaner. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
...rkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/DataSystemComponents.java
Show resolved
Hide resolved
…entData Made-with: Cursor
…terrupted Cache miss and missing persistent store now return a "transfer of none" changeset (ChangeSetType.None with Selector.EMPTY) instead of an interrupted status. This fixes an OFFLINE mode regression where a cache miss left the SDK in a failed initialization state because no synchronizers follow to recover. Made-with: Cursor
...rkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/FDv2CacheInitializer.java
Outdated
Show resolved
Hide resolved
...rkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/FDv2CacheInitializer.java
Outdated
Show resolved
Hide resolved
...darkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ContextDataManager.java
Outdated
Show resolved
Hide resolved
| resultFuture.set(FDv2SourceResult.status( | ||
| FDv2SourceResult.Status.interrupted(e), false)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Something that might be worth testing.
Dispatching the execution onto the executor does have some overhead / delay associated with it as the other thread is scheduled to pick up the work. LDFutures.anyOf also has dispatching internally. If that overhead / delay is significant compared to the average time to read flags from the persistence, then this initializer may be better to just do the work right on the same thread as run() is called. This would undermine the "close-ability" of this initializer, but if the interaction with persistence is fast enough, do you really need "close-ability?
We may need this code path to be as fast as possible because a client initing with a timeout of 0 expects to get cached values as quickly as possible. We don't want to slow this use case down.
Unfortunately testing this on an emulator is not going to give good results so this would have to be done on a physical device.
This same concern does not apply to the polling or streaming initializers / synchronizers because the execution overhead is insignificant compared to the network latency (at least that is what we have been assuming).
There was a problem hiding this comment.
Thinking more, we may need to do more extensive testing around this "hot path to cached flags" because the FDv2 data source is already using a fixed thread pool executor which also has some overhead to start the thread that will call run().
There was a problem hiding this comment.
From my testing on my device, it looks like there is ~300us of delay. From other benchmarking of the Android SDKs startup performance on my phone, the total time to get from init starting to fdv2Source_start_start is ~48ms, so this task delay is 0.5% of startup time.
So not a big impact from a speed standpoint.
Conclusion: try to run on thread that called run() if trivial to keep thread count down, but if there is any concern it is an issue, then don't change.
There was a problem hiding this comment.
This change was really straightforward. Updated in that latest push.
|
We need to meet this requirement: For SDKs that are inherently waiting for cache today even with a timeout of 0 seconds, we need to ensure that in FDv2 that same SDK major version continues to wait for the cache initializer. |
...rkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/FDv2CacheInitializer.java
Show resolved
Hide resolved
…e exceptions as None Two fixes for the cache initializer: 1. Orchestrator: only complete initialization from the post-initializer loop when no synchronizers are available. When synchronizers exist, they are the authority on init completion. Fixes premature init in POLLING/STREAMING modes where a cache miss None changeset was completing start before the synchronizer fetched server data. 2. Cache initializer: return ChangeSetType.None on exceptions during cache read instead of interrupted status. A corrupt/unreadable cache is semantically equivalent to an empty cache, not a hard error. Made-with: Cursor
- Trim FDv2CacheInitializer Javadoc to only describe this class's responsibility; remove references to orchestrator, other initializers, HTTP status codes, and external spec sections. - Merge setCurrentContext() into switchToContext(context, skipCacheLoad) to eliminate the separate method and simplify call sites in the constructor and LDClient.identifyInternal(). Made-with: Cursor
….com:launchdarkly/android-client-sdk into aaronz/SDK-2070/cache-initializer
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b8a50de. Configure here.
| // consider initialization successful. When synchronizers are available, defer init | ||
| // completion to the synchronizer loop — the synchronizer is the authority on whether | ||
| // the SDK has a verified, up-to-date payload. | ||
| if (anyDataReceived && !sourceManager.hasAvailableSynchronizers()) { |
There was a problem hiding this comment.
ChangeSetType.None incorrectly marks data as received
Medium Severity
The new cache initializer returns ChangeSetType.None on cache miss, but runInitializers at line 324 sets anyDataReceived = true for any non-null changeset including None. Combined with the new condition at line 369, this causes modes without synchronizers (e.g., ONE_SHOT) to report successful initialization even when no actual flag data exists. For example: ONE_SHOT with cache miss + polling failure now calls tryCompleteStart(true, null) because the None changeset falsely set anyDataReceived, whereas previously initialization would correctly fail.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b8a50de. Configure here.
…or overhead Cache read now runs inline on the caller's thread instead of dispatching to an executor, removing ~300us of thread scheduling overhead per Todd's benchmarking. close() becomes a no-op since there is nothing to cancel. Made-with: Cursor
| // consider initialization successful. When synchronizers are available, defer init | ||
| // completion to the synchronizer loop — the synchronizer is the authority on whether | ||
| // the SDK has a verified, up-to-date payload. | ||
| if (anyDataReceived && !sourceManager.hasAvailableSynchronizers()) { |
There was a problem hiding this comment.
I'm against this change without understanding the reason more. runInitializers is designed to not know about synchronizers. What is the reason?
There was a problem hiding this comment.
OK, let me look into this part again. Now that I've taken a step back, I agree it's weird to have this synchronizer-specific bit inside runInitializers().
| // so we inject it here by replacing the placeholder with a wired copy. | ||
| final DataSourceBuilder<Initializer> effective; | ||
| if (builder instanceof DataSystemComponents.CacheInitializerBuilderImpl) { | ||
| effective = new DataSystemComponents.CacheInitializerBuilderImpl(envData); |
There was a problem hiding this comment.
If ReadOnlyPerEnvironmentData was available in inputs, does this go away?
We had talked about a con with putting it in inputs which was: If we ever have customers implement initializers and synchronizers, then their code would get access to the readable env data which is undesirable.
We have a sort of "pattern" with ClientContext and ClientContextImpl that lets us pass around internal only information and "unwrap" it where necessary in internal components and we tell customers to not make such a call as it isn't protected by any contractual guarantees. Could this sort of approach be used here? We would have DataSourceBuildInputs and an extending class like DataSourceBuildInputsInternal or something like that. ReadOnlyPerEnvironmentData would only be available on the Internal one.
We can discuss today as well.
There was a problem hiding this comment.
Yeah, if ReadOnlyPerEnvironemntData was available in the inputs, then this special handling for the cache initializer goes away. And that's correct: the con is that ReadOnlyPerEnvironmentData would need to be a public interface. And now that we've updated the interface's function to return EnvironmentData, I think EnvironmentData would need to be public as well? (The previous commit got around that last bit by returning a Map<> instead of an actual EnvironmentData object.)
What's the exact mechanism for the pattern you mentioned? Can you point out one instance of it being used? 🙏
There was a problem hiding this comment.
Any lines like ClientContextImpl impl = ClientContextImpl.get(clientContext);
So you could make a DataSourceBuildInputsInternal that would extend DataSourceBuildInputs. DataSourceBuildInputsInternal would have the ReadOnlyPerEnvironmentData member.
In the initializer that we write, we would receive the DataSourceBuildInputs and do something like
DataSourceBuildInputsInternal inputsInternal = DataSourceBuildInputsInternal.get(inputs)
inputsInternal.getPerEnvironmentDataIfAvailable()
This unwraps the inputs to get at the internal only properties. A customer can do this in their own code, but we are free to break that contract as we make no guarantees. I'm hand waving package visibility here, but I think DataSourceBuildInputs and DataSourceBuildInputsInternal having the same visibility isn't a problem. Copious documentation on the DataSourceBuildInputsInternal class can provide warning to developers that it is for internal use only.
There was a problem hiding this comment.
You had mentioned calling the cache initializer synchronously in start here right before sharedExecutor.execute. Is that a pending change?
There was a problem hiding this comment.
That change is in this other PR for now: #346. I put it in the separate PR so it's easier to review on its own.
2 participants
Goal
Implement the FDv2 cache initializer so that cached flag data is loaded from local storage as the first step in every connection mode's initializer chain, providing immediate flag values while the network initializer fetches fresh data.
Approach
The FDv1 production code already loads cached flags during
ContextDataManager.switchToContext(). In FDv2, this cache loading is modeled as a dedicatedInitializer— the first in the chain — per CONNMODE spec 4.1.1. The implementation follows the same patterns asFDv2PollingInitializer(executor-based async, shutdown future race viaLDFutures.anyOf).The cache initializer is wired through
DataSourceBuildInputsusingPersistentDataStoreWrapper.ReadOnlyPerEnvironmentData, adapted inFDv2DataSourceBuilder.makeInputs()from the existingPerEnvironmentData.Key Behaviors
CHANGE_SETwithChangeSetType.Full,Selector.EMPTY, andpersist=false, so the orchestrator applies the data immediately but continues to the next initializer for a verified selector from the serverChangeSetType.Nonechangeset — analogous to "transfer of none" / 304 Not Modified (CSFDV2 9.1.2). This signals "I checked the source and there is nothing new" rather than an errortryCompleteStartis only called when!sourceManager.hasAvailableSynchronizers(). This ensures OFFLINE mode (no synchronizers) initializes successfully from cache alone, while POLLING/STREAMING modes wait for the synchronizer to fetch verified server datafdv1Fallbackis alwaysfalsesince the cache is local (no server headers involved)ContextDataManagerskips redundant cache loading via a newskipCacheLoadconstructor parameter andsetCurrentContext()method, keeping FDv1 behavior unchangedNot In Scope
Requirements
Related issues
Provide links to any issues in this repository or elsewhere relating to this pull request.
Describe the solution you've provided
Provide a clear and concise description of what you expect to happen.
Describe alternatives you've considered
Provide a clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context about the pull request here.
Note
Medium Risk
Changes FDv2 startup/identify initialization flow and cache-loading behavior across all connection modes, which could affect when the SDK reports initialized and what flag values are available early in app startup. Logic is well-covered by tests but touches core data source orchestration and persistence interactions.
Overview
Adds an FDv2 cache initializer that reads persisted flag data from local storage and returns it as an FDv2
ChangeSet(cache hit =Fullwithpersist=falseand empty selector; all non-hit cases returnNonewithout error).Wires this cache initializer into the default FDv2 mode table so it runs first in every connection mode, and updates
FDv2DataSourceto only mark initialization complete from initializer data when no synchronizers will run.Adjusts context-switch caching behavior to avoid redundant loads on the FDv2 path by adding a
skipCacheLoadflag toContextDataManager.switchToContext(...), and plumbs optional persistent-store access viaClientContextImpl.getPerEnvironmentDataIfAvailable()and a newPersistentDataStoreWrapper.ReadOnlyPerEnvironmentDatainterface. Updates/extends unit tests accordingly.Reviewed by Cursor Bugbot for commit 3f0066b. Bugbot is set up for automated code reviews on this repo. Configure here.