Skip to content
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

fix: improved LDClient.identify(...) behavior when offline #261

Merged
merged 3 commits into from
Mar 26, 2024
Merged

fix: improved LDClient.identify(...) behavior when offline #261

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9e8a61e
fix: improved LDClient.identify(...) behavior when offline
tanderson-ld Mar 22, 2024
4150e05
ci: adding testing related to identify
tanderson-ld Mar 25, 2024
2b93d4d
Review comments related to unit test and ClientContextImpl volatility.
tanderson-ld Mar 25, 2024
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
7 changes: 6 additions & 1 deletion ...y-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/AndroidPlatformState.java
View file
Open in desktop
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
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reviewers: AndroidPlatformState claims to be closeable, in which close is idempotent after the first call. This changes makes it idempotent.


application.unregisterActivityLifecycleCallbacks(lifecycleCallbacks);
}

Expand Down
15 changes: 15 additions & 0 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 @@ -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
);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reviewers: LDClient's ClientContextImpl member variable should be the authority on the current context. This allows us to change the context in that object so that can be true after an identify call.

Copy link
Member

Choose a reason for hiding this comment

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

I have some concerns with this. Copies of the client context are passed to various components, and those client contexts do not have a centralized update mechanism. So putting the evaluation context into the client context provides easy access to a stale or unset LDContext.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. I believe that is how it has been. Any components that use the context have an alternative mechanism for updating it when it changes.


public DiagnosticStore getDiagnosticStore() {
return diagnosticStore;
}
Expand Down
61 changes: 32 additions & 29 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
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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reviewers: The sink now holds the reference to the data manager instead of a private class holding a reference to an enclosing class's member.

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));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reviewers: The context was added to go along with the flag data. This helps other parts of the system make decisions on whether or not to accept the flag data. Ideally, this manager and the sources of this data could only exist when getting data from them makes sense, but a refactoring of the managers to be instead immutable instances and implement disposable is too risky at this time.

// 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());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reviewers: The ConnectivityManager should not be involved at all with the concept of using cached data.


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