chore: transactional ContextDataManager in prep for FDv2#322
chore: transactional ContextDataManager in prep for FDv2#322tanderson-ld wants to merge 2 commits intomainfrom
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
...darkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ContextDataManager.java
Outdated
Show resolved
Hide resolved
...darkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ContextDataManager.java
Show resolved
Hide resolved
| break; | ||
| case None: | ||
| default: | ||
| break; |
There was a problem hiding this comment.
None changeset silently drops its selector
Medium Severity
The ChangeSetType.None documentation explicitly states the changeset "may still carry a selector to store in memory," but the apply method's None case just breaks without updating currentSelector. A None changeset with a non-empty selector will have that selector silently discarded. Both Full and Partial branches update the selector, but None does not.
Additional Locations (1)
There was a problem hiding this comment.
I don't think this matters as the selector should be the same as the previous selector.
I guess theoretically the back-end could just keep sending us different selectors, but it shouldn't have a practical effect.
| logger.debug("Using stored flag data for this context"); | ||
| initDataInternal(context, storedData, false); | ||
| // when we switch context, we don't have a selector because we don't currently support persisting the selector. | ||
| applyFullData(context, Selector.EMPTY, storedData.getAll(), false); |
There was a problem hiding this comment.
Stale selector persists after context switch
Medium Severity
When switchToContext is called, currentSelector is never reset. The comment at line 106 states the intent is to have no selector after switching, but applyFullData is called with Selector.EMPTY, and the if (!selector.isEmpty()) guard prevents the assignment. Additionally, when storedData is null, the method returns early without any selector reset. This leaves a stale selector from the previous context, which could cause incorrect basis requests in FDv2.
Additional Locations (1)
| @NonNull private volatile ContextIndex index; | ||
|
|
||
| /** Selector from the last applied changeset that carried one; in-memory only, not persisted. */ | ||
| @NonNull private volatile Selector currentSelector = Selector.EMPTY; |
There was a problem hiding this comment.
I think this works out because we are single-threaded single-producer for that data. But the volatile sometimes makes me unseasy, because it feels like the writes should by synced. No action required, just chain of thought.
There was a problem hiding this comment.
The writes are synchronized. So I am happy.
| if (!context.equals(currentContext)) { | ||
| return; | ||
| } | ||
| if (!selector.isEmpty()) { |
There was a problem hiding this comment.
I think this should be non-conditional.
There was a problem hiding this comment.
If we are applying selectorless data, then data would need to resume based on a selectorless state. Otherwise we would apply a patch with unrelated provenance.
| * to store in memory for the next request (e.g. as basis), and for Full/Partial types a map of | ||
| * flag key to flag data. | ||
| */ | ||
| public final class ChangeSet { |
There was a problem hiding this comment.
Is this compatible with the server one? I am just wondering if maybe we can hoist the server one because it is generic over the model type.
3 participants
Requirements
Related issues
SDK-1817
Note
Medium Risk
Touches core flag update/persistence and listener-notification paths and adds a new update mechanism that could affect correctness under concurrency/versioning; mitigated by focused unit tests but still impacts critical runtime behavior.
Overview
Adds a transactional update API for flag data by introducing
ChangeSet/ChangeSetTypeplusTransactionalDataStore/TransactionalDataSourceUpdateSink, and extends the data source sink surface withDataSourceUpdateSinkV2.Refactors
ContextDataManagerto implement transactionalapply()for full replace and partial merge updates (including optional persistence) and to track the latest non-emptySelectorin memory;ConnectivityManagernow forwardsapply(ChangeSet)from the sink to the store.Updates shared test infrastructure and adds
ContextDataManagerApplyTestto cover full/partial/none changesets, persistence toggles, selector retention behavior, and context-mismatch no-ops; shared test module now depends onlaunchdarkly-java-sdk-internalforSelector.Written by Cursor Bugbot for commit 1e3da90. This will update automatically on new commits. Configure here.