Skip to content

Commit

Permalink
fix: improved LDClient.identify(...) behavior when offline (#261)
Browse files Browse the repository at this point in the history
**Requirements**

- [x] I have added test coverage for new or changed functionality

- [x] I have followed the repository's [pull request submission
guidelines](../blob/main/CONTRIBUTING.md#submitting-pull-requests)
- [x] I have validated my changes against all supported platform
versions

**Related issues**

SC-237349

**Describe the solution you've provided**

Updated code so that ContextDataManager switches context independently
of ConnectivityManager and ConnectivityManager is not in the path of
context switching.
  • Loading branch information
tanderson-ld authored Mar 26, 2024
1 parent aad01e7 commit 24ce942
Show file tree
Hide file tree
Showing 15 changed files with 273 additions and 207 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -144,4 +144,38 @@ public void clientUsesStoredFlagsIfInitializationTimesOutInPollingMode() throws
assertEquals(flagValue, client.stringVariation(flagKey, "default"));
}
}

@Test
public void identifyWhenPollingFailsAndCacheAlreadyExists() throws Exception {
// set up data store with flag data for ContextA and ContextB
// insert ContextA's flags
LDContext contextA = LDContext.create("ContextA");
String flagKeyA = "flag-keyA", flagValueA = "stored-valueA";
Flag flagA = new FlagBuilder(flagKeyA).version(1).value(LDValue.of(flagValueA)).build();
TestUtil.writeFlagUpdateToStore(store, MOBILE_KEY, contextA, flagA);
// insert contextB's flags
LDContext contextB = LDContext.create("ContextB");
String flagKeyB = "flag-keyB", flagValueB = "stored-valueB";
Flag flagB = new FlagBuilder(flagKeyB).version(1).value(LDValue.of(flagValueB)).build();
TestUtil.writeFlagUpdateToStore(store, MOBILE_KEY, contextB, flagB);

// response to initialization for ContextA
mockPollingServer.enqueue(new MockResponse().setResponseCode(401));

// response to contextB's identify
mockPollingServer.enqueue(new MockResponse().setResponseCode(401));

LDConfig config = baseConfig()
.dataSource(Components.pollingDataSource())
.serviceEndpoints(Components.serviceEndpoints().polling(mockPollingServerUri))
.build();

LDClient client = LDClient.init(application, config, contextA, 1);
assertFalse("client should not have been initialized", client.isInitialized());
assertFalse("client was offline", client.isOffline());
assertEquals(flagValueA, client.stringVariation(flagKeyA, "defaultA"));

client.identify(contextB).get();
assertEquals(flagValueB, client.stringVariation(flagKeyB, "defaultB"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,12 @@ public File getCacheDir() {
public void close() {
connectivityChangeListeners.clear();
foregroundChangeListeners.clear();
application.unregisterReceiver(connectivityReceiver);
try {
application.unregisterReceiver(connectivityReceiver);
} catch (IllegalArgumentException e) {
// getting here just means the receiver wasn't registered, which is our objective
}

application.unregisterActivityLifecycleCallbacks(lifecycleCallbacks);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,21 @@ public static ClientContextImpl forDataSource(
);
}

/**
* Sets the evaluation context and returns a new instance of {@link ClientContextImpl}
* @param context to now use as the evaluation context
* @return a new instance
*/
public ClientContextImpl setEvaluationContext(LDContext context) {
return new ClientContextImpl(
super.setEvaluationContext(context),
this.diagnosticStore,
this.fetcher,
this.platformState,
this.taskExecutor
);
}

public DiagnosticStore getDiagnosticStore() {
return diagnosticStore;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ class ConnectivityManager {
private final DataSourceUpdateSink dataSourceUpdateSink;
private final ConnectionInformationState connectionInformation;
private final PersistentDataStoreWrapper.PerEnvironmentData environmentStore;
private final ContextDataManager contextDataManager;
private final EventProcessor eventProcessor;
private final PlatformState.ForegroundChangeListener foregroundListener;
private final PlatformState.ConnectivityChangeListener connectivityChangeListener;
Expand All @@ -66,7 +65,7 @@ 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> currentEvaluationContext = new AtomicReference<>();
private final AtomicReference<LDContext> currentContext = new AtomicReference<>();
private final AtomicReference<Boolean> previouslyInBackground = new AtomicReference<>();
private final LDLogger logger;
private volatile boolean initialized = false;
Expand All @@ -76,19 +75,23 @@ class ConnectivityManager {
// 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."
private class DataSourceUpdateSinkImpl implements DataSourceUpdateSink {
private final ContextDataManager contextDataManager;
private final AtomicReference<ConnectionMode> connectionMode = new AtomicReference<>(null);
private final AtomicReference<LDFailure> lastFailure = new AtomicReference<>(null);

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

@Override
public void init(Map<String, DataModel.Flag> items) {
contextDataManager.initData(currentEvaluationContext.get(),
EnvironmentData.usingExistingFlagsMap(items));
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.
}

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

Expand Down Expand Up @@ -148,15 +151,14 @@ public void shutDown() {
) {
this.baseClientContext = clientContext;
this.dataSourceFactory = dataSourceFactory;
this.dataSourceUpdateSink = new DataSourceUpdateSinkImpl();
this.dataSourceUpdateSink = new DataSourceUpdateSinkImpl(contextDataManager);
this.platformState = ClientContextImpl.get(clientContext).getPlatformState();
this.eventProcessor = eventProcessor;
this.contextDataManager = contextDataManager;
this.environmentStore = environmentStore;
this.taskExecutor = ClientContextImpl.get(clientContext).getTaskExecutor();
this.logger = clientContext.getBaseLogger();

currentEvaluationContext.set(clientContext.getEvaluationContext());
currentContext.set(clientContext.getEvaluationContext());
forcedOffline.set(clientContext.isSetOffline());

LDConfig ldConfig = clientContext.getConfig();
Expand All @@ -172,20 +174,28 @@ public void shutDown() {
foregroundListener = foreground -> {
DataSource dataSource = currentDataSource.get();
if (dataSource == null || dataSource.needsRefresh(!foreground,
currentEvaluationContext.get())) {
currentContext.get())) {
updateDataSource(true, LDUtil.noOpCallback());
}
};
platformState.addForegroundChangeListener(foregroundListener);
}

void setEvaluationContext(@NonNull LDContext newContext, @NonNull Callback<Void> onCompletion) {
/**
* 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
*/
void switchToContext(@NonNull LDContext context, @NonNull Callback<Void> onCompletion) {
DataSource dataSource = currentDataSource.get();
LDContext oldContext = currentEvaluationContext.getAndSet(newContext);
if (oldContext == newContext || oldContext.equals(newContext)) {
LDContext oldContext = currentContext.getAndSet(context);
if (oldContext == context || oldContext.equals(context)) {
onCompletion.onSuccess(null);
} else {
if (dataSource == null || dataSource.needsRefresh(!platformState.isForeground(), newContext)) {
if (dataSource == null || dataSource.needsRefresh(!platformState.isForeground(), context)) {
updateDataSource(true, onCompletion);
} else {
onCompletion.onSuccess(null);
Expand All @@ -204,7 +214,7 @@ private synchronized boolean updateDataSource(
boolean forceOffline = forcedOffline.get();
boolean networkEnabled = platformState.isNetworkAvailable();
boolean inBackground = !platformState.isForeground();
LDContext evaluationContext = currentEvaluationContext.get();
LDContext context = currentContext.get();

eventProcessor.setOffline(forceOffline || !networkEnabled);
eventProcessor.setInBackground(inBackground);
Expand Down Expand Up @@ -241,7 +251,7 @@ private synchronized boolean updateDataSource(
ClientContext clientContext = ClientContextImpl.forDataSource(
baseClientContext,
dataSourceUpdateSink,
evaluationContext,
context,
inBackground,
previouslyInBackground.get()
);
Expand Down Expand Up @@ -357,13 +367,6 @@ synchronized boolean startUp(@NonNull Callback<Void> onCompletion) {
return false;
}
initialized = false;

// Calling initFromStoredData updates the current flag state *if* stored flags exist for
// this context. If they don't, it has no effect. Currently we do *not* return early from
// initialization just because stored flags exist; we're just making them available in case
// initialization times out or otherwise fails.
contextDataManager.initFromStoredData(currentEvaluationContext.get());

return updateDataSource(true, onCompletion);
}

Expand Down Expand Up @@ -400,12 +403,12 @@ synchronized ConnectionInformation getConnectionInformation() {

static void fetchAndSetData(
FeatureFetcher fetcher,
LDContext currentContext,
LDContext contextToFetch,
DataSourceUpdateSink dataSourceUpdateSink,
Callback<Boolean> resultCallback,
LDLogger logger
) {
fetcher.fetch(currentContext, new Callback<String>() {
fetcher.fetch(contextToFetch, new Callback<String>() {
@Override
public void onSuccess(String flagsJson) {
EnvironmentData data;
Expand All @@ -417,15 +420,15 @@ public void onSuccess(String flagsJson) {
e, LDFailure.FailureType.INVALID_RESPONSE_BODY));
return;
}
dataSourceUpdateSink.init(data.getAll());
dataSourceUpdateSink.init(contextToFetch, data.getAll());
resultCallback.onSuccess(true);
}

@Override
public void onError(Throwable e) {
logger.error("Error when attempting to get flag data: [{}] [{}]: {}",
LDUtil.base64Url(currentContext),
currentContext,
LDUtil.base64Url(contextToFetch),
contextToFetch,
LogValues.exceptionSummary(e));
resultCallback.onError(e);
}
Expand Down
Loading

0 comments on commit 24ce942

Please sign in to comment.