Skip to content

chore: clearing selector and fixing fdv2 race conditions #347

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.

Sign up for GitHub

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

Merged
tanderson-ld merged 4 commits into from
Apr 21, 2026
Merged

chore: clearing selector and fixing fdv2 race conditions #347

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48b7edb
chore: clearing selector and fixing fdv2 race conditions
tanderson-ld Apr 15, 2026
4e44292
adding tests
tanderson-ld Apr 15, 2026
6fa3f6b
Merge remote-tracking branch 'origin' into ta/SDK-2072/fdv2-identify-…
tanderson-ld Apr 20, 2026
e6caba2
addressing review comments, fixing skip cache issue
tanderson-ld Apr 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 15 additions & 17 deletions ...rkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ClientContextImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import com.launchdarkly.sdk.android.subsystems.ClientContext;
import com.launchdarkly.sdk.android.subsystems.DataSourceUpdateSink;
import com.launchdarkly.sdk.android.subsystems.HttpConfiguration;
import com.launchdarkly.sdk.android.subsystems.TransactionalDataStore;
import com.launchdarkly.sdk.internal.events.DiagnosticStore;

import androidx.annotation.Nullable;
Expand Down Expand Up @@ -37,9 +36,9 @@ final class ClientContextImpl extends ClientContext {
private final TaskExecutor taskExecutor;
private final PersistentDataStoreWrapper.PerEnvironmentData perEnvironmentData;
@Nullable
private final TransactionalDataStore transactionalDataStore;
private final SelectorSource selectorSource;

/** Used by FDv1 code paths that do not need a {@link TransactionalDataStore}. */
/** Used by FDv1 code paths that do not need a {@link SelectorSource}. */
Copy link
Copy Markdown
Contributor Author

@tanderson-ld tanderson-ld Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers: The transactional data store was more than strictly necessary, so tidying that up.

ClientContextImpl(
ClientContext base,
DiagnosticStore diagnosticStore,
Expand All @@ -52,9 +51,8 @@ final class ClientContextImpl extends ClientContext {
}

/**
* Used by FDv2 code paths. The {@code transactionalDataStore} is needed by
* {@link FDv2DataSourceBuilder} to create {@link SelectorSourceFacade} instances
* that provide selector state to initializers and synchronizers.
* Used by FDv2 code paths. The {@code selectorSource} provides selector state to
* initializers and synchronizers via the {@link ContextDataManager.ContextDataManagerView}.
*/
ClientContextImpl(
ClientContext base,
Expand All @@ -63,15 +61,15 @@ final class ClientContextImpl extends ClientContext {
PlatformState platformState,
TaskExecutor taskExecutor,
PersistentDataStoreWrapper.PerEnvironmentData perEnvironmentData,
@Nullable TransactionalDataStore transactionalDataStore
@Nullable SelectorSource selectorSource
) {
super(base);
this.diagnosticStore = diagnosticStore;
this.fetcher = fetcher;
this.platformState = platformState;
this.taskExecutor = taskExecutor;
this.perEnvironmentData = perEnvironmentData;
this.transactionalDataStore = transactionalDataStore;
this.selectorSource = selectorSource;
}

static ClientContextImpl fromConfig(
Expand Down Expand Up @@ -119,7 +117,7 @@ public static ClientContextImpl get(ClientContext context) {
return new ClientContextImpl(context, null, null, null, null, null);
}

/** Creates a context for FDv1 data sources that do not need a {@link TransactionalDataStore}. */
/** Creates a context for FDv1 data sources that do not need a {@link SelectorSource}. */
public static ClientContextImpl forDataSource(
ClientContext baseClientContext,
DataSourceUpdateSink dataSourceUpdateSink,
Expand All @@ -132,17 +130,17 @@ public static ClientContextImpl forDataSource(
}

/**
* Creates a context for data sources, optionally including a {@link TransactionalDataStore}.
* FDv2 data sources require the store so that {@link FDv2DataSourceBuilder} can provide
* selector state to initializers and synchronizers via {@link SelectorSourceFacade}.
* Creates a context for data sources, optionally including a {@link SelectorSource}.
* FDv2 data sources require the selector source so that {@link FDv2DataSourceBuilder} can
* provide selector state to initializers and synchronizers.
*/
public static ClientContextImpl forDataSource(
ClientContext baseClientContext,
DataSourceUpdateSink dataSourceUpdateSink,
LDContext newEvaluationContext,
boolean newInBackground,
Boolean previouslyInBackground,
@Nullable TransactionalDataStore transactionalDataStore
@Nullable SelectorSource selectorSource
) {
ClientContextImpl baseContextImpl = ClientContextImpl.get(baseClientContext);
return new ClientContextImpl(
Expand All @@ -166,7 +164,7 @@ public static ClientContextImpl forDataSource(
baseContextImpl.getPlatformState(),
baseContextImpl.getTaskExecutor(),
baseContextImpl.getPerEnvironmentData(),
transactionalDataStore
selectorSource
);
}

Expand All @@ -183,7 +181,7 @@ public ClientContextImpl setEvaluationContext(LDContext context) {
this.platformState,
this.taskExecutor,
this.perEnvironmentData,
this.transactionalDataStore
this.selectorSource
);
}

Expand All @@ -208,8 +206,8 @@ public PersistentDataStoreWrapper.PerEnvironmentData getPerEnvironmentData() {
}

@Nullable
public TransactionalDataStore getTransactionalDataStore() {
return transactionalDataStore;
public SelectorSource getSelectorSource() {
return selectorSource;
}

private static <T> T throwExceptionIfNull(T o) {
Expand Down
117 changes: 62 additions & 55 deletions ...ly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ConnectivityManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.launchdarkly.sdk.android;

import androidx.annotation.NonNull;
import androidx.annotation.Nullable;

import com.launchdarkly.logging.LDLogger;
import com.launchdarkly.logging.LogValues;
Expand All @@ -15,19 +16,19 @@
import com.launchdarkly.sdk.android.subsystems.DataSourceUpdateSink;
import com.launchdarkly.sdk.android.subsystems.DataSourceUpdateSinkV2;
import com.launchdarkly.sdk.android.subsystems.EventProcessor;
import com.launchdarkly.sdk.android.subsystems.TransactionalDataStore;

import java.io.Closeable;
import java.io.IOException;
import java.lang.ref.WeakReference;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;

class ConnectivityManager {
class ConnectivityManager implements ContextDataManager.ContextSwitchListener {
// Implementation notes:
//
// 1. This class has no direct interactions with Android APIs. All logic related to detecting
Expand Down Expand Up @@ -56,11 +57,9 @@ class ConnectivityManager {
private final ClientContext baseClientContext;
private final PlatformState platformState;
private final ComponentConfigurer<DataSource> dataSourceFactory;
private final DataSourceUpdateSink dataSourceUpdateSink;
private final ConnectionInformationState connectionInformation;
private final PersistentDataStoreWrapper.PerEnvironmentData environmentStore;
private final EventProcessor eventProcessor;
private final TransactionalDataStore transactionalDataStore;
Copy link
Copy Markdown
Contributor Author

@tanderson-ld tanderson-ld Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers: Writing data is now done through the ContextDataManager.ContextDataManagerView

private final PlatformState.ForegroundChangeListener foregroundListener;
private final PlatformState.ConnectivityChangeListener connectivityChangeListener;
private final TaskExecutor taskExecutor;
Expand All @@ -71,7 +70,6 @@ class ConnectivityManager {
private final AtomicBoolean started = new AtomicBoolean();
private final AtomicBoolean closed = new AtomicBoolean();
private final AtomicReference<DataSource> currentDataSource = new AtomicReference<>();
private final AtomicReference<LDContext> currentContext = new AtomicReference<>();
private final AtomicReference<ModeState> previousModeState = new AtomicReference<>();
private final LDLogger logger;
private volatile boolean initialized = false;
Expand All @@ -80,33 +78,35 @@ class ConnectivityManager {
private volatile ConnectionMode currentFDv2Mode;
private final AutomaticModeSwitchingConfig autoModeSwitchingConfig;

private final AtomicReference<LDContext> currentContext = new AtomicReference<>();
private volatile ContextDataManager.ContextDataManagerView currentView;
@Nullable private volatile Callback<Void> pendingStartUpCallback;

// The DataSourceUpdateSinkImpl receives flag updates and status updates from the DataSource.
// This has two purposes: 1. to decouple the data source implementation from the details of how
// data is stored; 2. to implement additional logic that does not depend on what kind of data
// source we're using, like "if there was an error, update the ConnectionInformation."
// Data operations (init, upsert, apply) are routed through a ContextDataManager.ContextDataManagerView
// which gates them with a validity flag — invalidated views silently discard writes.
// Status operations (setStatus, shutDown) are routed to ConnectivityManager directly.
// A new instance is created for each data source, using the view that was current at the time.
private class DataSourceUpdateSinkImpl implements DataSourceUpdateSink, DataSourceUpdateSinkV2 {
private final ContextDataManager contextDataManager;
private final ContextDataManager.ContextDataManagerView view;

DataSourceUpdateSinkImpl(ContextDataManager contextDataManager) {
this.contextDataManager = contextDataManager;
DataSourceUpdateSinkImpl(ContextDataManager.ContextDataManagerView view) {
this.view = view;
}

@Override
public void init(LDContext context, Map<String, DataModel.Flag> items) {
contextDataManager.initData(context, EnvironmentData.usingExistingFlagsMap(items));
// Currently, contextDataManager is responsible for firing any necessary flag change events.
view.init(context, items);
}

@Override
public void upsert(LDContext context, DataModel.Flag item) {
contextDataManager.upsert(context, item);
// Currently, contextDataManager is responsible for firing any necessary flag change events.
view.upsert(context, item);
}

@Override
public void apply(@NonNull LDContext context, @NonNull ChangeSet<Map<String, DataModel.Flag>> changeSet) {
contextDataManager.apply(context, changeSet);
// Currently, contextDataManager is responsible for firing any necessary flag change events.
view.apply(context, changeSet);
}

@Override
Expand Down Expand Up @@ -144,20 +144,16 @@ public void shutDown() {
ConnectivityManager(@NonNull final ClientContext clientContext,
@NonNull final ComponentConfigurer<DataSource> dataSourceFactory,
@NonNull final EventProcessor eventProcessor,
@NonNull final ContextDataManager contextDataManager,
@NonNull final PersistentDataStoreWrapper.PerEnvironmentData environmentStore
) {
this.baseClientContext = clientContext;
this.dataSourceFactory = dataSourceFactory;
this.dataSourceUpdateSink = new DataSourceUpdateSinkImpl(contextDataManager);
this.platformState = ClientContextImpl.get(clientContext).getPlatformState();
this.eventProcessor = eventProcessor;
this.environmentStore = environmentStore;
this.transactionalDataStore = contextDataManager;
this.taskExecutor = ClientContextImpl.get(clientContext).getTaskExecutor();
this.logger = clientContext.getBaseLogger();

currentContext.set(clientContext.getEvaluationContext());
Copy link
Copy Markdown
Contributor Author

@tanderson-ld tanderson-ld Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers: The context is now given to the ConnectivityManager via onContextChanged which fires during startup to set the initial context.

forcedOffline.set(clientContext.isSetOffline());

LDConfig ldConfig = clientContext.getConfig();
Expand Down Expand Up @@ -189,32 +185,29 @@ public void shutDown() {
platformState.addForegroundChangeListener(foregroundListener);
}

/**
* Switches the {@link ConnectivityManager} to begin fetching/receiving information
* relevant to the context provided. This is likely to result in the teardown of existing
* connections, but the timing of that is not guaranteed.
*
* @param context to swtich to
* @param onCompletion callback that indicates when the switching is done
*/
Copy link
Copy Markdown
Contributor Author

@tanderson-ld tanderson-ld Apr 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers: switchToContext no longer exists, instead the connectivity manager listens to synchronous context changes from the ContextDataManager. This allows the context data manager to have confidence views have been invalidated and disseminated in an atomic way.

Copy link
Copy Markdown
Contributor

@aaron-zeisler aaron-zeisler Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! This is a cool change that I wouldn't have thought of 👍

void switchToContext(@NonNull LDContext context, @NonNull Callback<Void> onCompletion) {
DataSource dataSource = currentDataSource.get();
LDContext oldContext = currentContext.getAndSet(context);
@Override
public synchronized void onContextChanged(
@NonNull LDContext context,
@NonNull ContextDataManager.ContextDataManagerView view,
@NonNull Callback<Void> onCompletion
) {
this.currentContext.set(context);
this.currentView = view;

if (oldContext == context || oldContext.equals(context)) {
Copy link
Copy Markdown
Contributor Author

@tanderson-ld tanderson-ld Apr 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers: A nice consequence of these changes is that the ConnectivityManager gets dumber. It respects whatever context the ContextDataManager sends via onContextChanged and does not do any checks against it. "Oh, you want a data source for that context, coming right up!"

onCompletion.onSuccess(null);
Callback<Void> effectiveCallback;
if (pendingStartUpCallback != null) {
effectiveCallback = LDUtil.compositeCallback(Arrays.asList(pendingStartUpCallback, onCompletion));
pendingStartUpCallback = null;
} else {
ModeState state = snapshotModeState();
if (dataSource == null || dataSource.needsRefresh(!state.isForeground(), context)) {
updateEventProcessor(forcedOffline.get(), state.isNetworkAvailable(), state.isForeground());
updateDataSource(true, state, onCompletion);
} else {
onCompletion.onSuccess(null);
}
effectiveCallback = onCompletion;
}

ModeState state = snapshotModeState();
updateEventProcessor(forcedOffline.get(), state.isNetworkAvailable(), state.isForeground());
updateDataSource(true, state, effectiveCallback);
}

private synchronized boolean updateDataSource(
private boolean updateDataSource(
Copy link
Copy Markdown
Contributor Author

@tanderson-ld tanderson-ld Apr 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers: This synchronization was unnecessary. Sync on private methods is usually a smell.

boolean mustReinitializeDataSource,
@NonNull ModeState newState,
@NonNull Callback<Void> onCompletion
Expand Down Expand Up @@ -257,7 +250,7 @@ private synchronized boolean updateDataSource(
// Only consult needsRefresh() when the platform state has actually changed since the
// last data source was built. Duplicate notifications (e.g. a connectivity event that
// doesn't change the network state) are filtered out, preventing unnecessary rebuilds.
// Context changes are handled by switchToContext(), which passes
// Context changes are handled via onContextChanged(), which passes
// mustReinitializeDataSource=true directly.
if (!mustReinitializeDataSource && existingDataSource != null) {
boolean inBackground = !newState.isForeground();
Expand Down Expand Up @@ -287,11 +280,11 @@ private synchronized boolean updateDataSource(
} else if (forceOffline) {
logger.debug("Initialized in offline mode");
initialized = true;
dataSourceUpdateSink.setStatus(ConnectionInformation.ConnectionMode.SET_OFFLINE, null);
updateConnectionInfoForSuccess(ConnectionInformation.ConnectionMode.SET_OFFLINE);
} else if (!newState.isNetworkAvailable()) {
dataSourceUpdateSink.setStatus(ConnectionInformation.ConnectionMode.OFFLINE, null);
updateConnectionInfoForSuccess(ConnectionInformation.ConnectionMode.OFFLINE);
} else if (!newState.isForeground() && newState.isBackgroundUpdatingDisabled()) {
dataSourceUpdateSink.setStatus(ConnectionInformation.ConnectionMode.BACKGROUND_DISABLED, null);
updateConnectionInfoForSuccess(ConnectionInformation.ConnectionMode.BACKGROUND_DISABLED);
Copy link
Copy Markdown
Contributor Author

@tanderson-ld tanderson-ld Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers: Instead of going through sink impl and then back to a private method of this class, just go straight to the private method in this class.

} else {
shouldStopExistingDataSource = mustReinitializeDataSource;
shouldStartDataSourceIfStopped = true;
Expand All @@ -309,14 +302,17 @@ private synchronized boolean updateDataSource(
return false;
}

ContextDataManager.ContextDataManagerView view = currentView;
DataSourceUpdateSink dataSourceUpdateSink = new DataSourceUpdateSinkImpl(view);

logger.debug("Creating data source (background={})", !newState.isForeground());
ClientContext clientContext = ClientContextImpl.forDataSource(
baseClientContext,
dataSourceUpdateSink,
context,
!newState.isForeground(),
previousModeState.get() != null ? !previousModeState.get().isForeground() : null,
transactionalDataStore
view // view will serve as the selector source
);

if (useFDv2ModeResolution) {
Expand Down Expand Up @@ -510,22 +506,33 @@ private void updateListenersOnFailure(final LDFailure ldFailure) {
/**
* Attempts to start the data source if possible.
* <p>
* If we are configured to be offline or the network is unavailable, it immediately calls the
* completion listener and returns. Otherwise, it continues initialization asynchronously and
* the listener will be called when the data source successfully starts up or permanently fails.
* If we are configured to be offline or the network is unavailable, the callback
* is completed immediately with success. Otherwise, it continues initialization
* asynchronously and the callback will be called when the data source successfully
* starts up or permanently fails.
*
* @param contextDataManager the context data manager to listen to
* @param onCompletion callback that indicates when startup is done
* @return true if we are online, or false if we are offline (this determines whether we should
* try to send an identify event on startup)
*/
synchronized boolean startUp(@NonNull Callback<Void> onCompletion) {
if (closed.get() || started.getAndSet(true)) {
synchronized boolean startUp(
@NonNull ContextDataManager contextDataManager,
@NonNull Callback<Void> onCompletion
) {
if (closed.get() || started.get()) {
return false;
}
initialized = false;

ModeState state = snapshotModeState();
updateEventProcessor(forcedOffline.get(), state.isNetworkAvailable(), state.isForeground());
return updateDataSource(true, state, onCompletion);
Copy link
Copy Markdown
Contributor Author

@tanderson-ld tanderson-ld Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers: This logic moved to onContextChanged.

pendingStartUpCallback = onCompletion;
started.set(true);

// CDM immediately calls onContextChanged(...) after registration and that will
// handle creating the first data source synchronously.
contextDataManager.setContextSwitchListener(this);

return currentDataSource.get() != null;
}

/**
Expand Down
Loading
Loading