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

tanderson-ld · 2024-03-22T20:33:14Z

Requirements

I have added test coverage for new or changed functionality
I have followed the repository's pull request submission guidelines
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.

shortcut-integration · 2024-03-22T20:33:18Z

This pull request has been linked to Shortcut Story #237349: Investigate Android airplane mode identify bug.

tanderson-ld · 2024-03-22T20:34:53Z

...rkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/AndroidPlatformState.java

+            application.unregisterReceiver(connectivityReceiver);
+        } catch (IllegalArgumentException e) {
+            // getting here just means the receiver wasn't registered, which is our objective
+        }


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

tanderson-ld · 2024-03-22T20:35:42Z

...hdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ClientContextImpl.java

+            this.platformState,
+            this.taskExecutor
+        );
+    }


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.

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.

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.

tanderson-ld · 2024-03-22T20:36:28Z

...arkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ConnectivityManager.java

@@ -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;


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.

tanderson-ld · 2024-03-22T20:38:20Z

...arkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ConnectivityManager.java

-            contextDataManager.initData(currentEvaluationContext.get(),
-                    EnvironmentData.usingExistingFlagsMap(items));
+        public void init(LDContext context, Map<String, DataModel.Flag> items) {
+            contextDataManager.initData(context, EnvironmentData.usingExistingFlagsMap(items));


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.

tanderson-ld · 2024-03-22T20:39:46Z

...arkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ConnectivityManager.java

-        // 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());


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

tanderson-ld · 2024-03-22T20:41:11Z

...darkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ContextDataManager.java

+                environmentStore.setContextData(contextId, newData);
+                logger.debug("Updated flag data for context {} in persistent store", contextId);
+            }
+            environmentStore.setIndex(newIndex);


For reviewers: environmentStore.setIndex(newIndex); should be in the lock block, so it was moved.

In the long-term I think we should attempt to change this to a queue of operations that apply to the store. Maybe transactional in some way. It would be preferable to not have to lock for the duration of IO.

tanderson-ld · 2024-03-22T20:42:12Z

...darkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ContextDataManager.java

@@ -162,21 +139,21 @@ private void initDataInternal(
            newIndex = index.updateTimestamp(contextId, System.currentTimeMillis())
                    .prune(maxCachedContexts, removedContextIds);
            index = newIndex;
-            flagsContextId = contextId;


For reviewers: Calculating a context id is fast and trivial. Why risk it being outdated? Removed.

tanderson-ld · 2024-03-22T20:43:29Z

...darkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ContextDataManager.java

-     * @param context the new context
-     * @return true if successful
-     */
-    public boolean initFromStoredData(@NonNull LDContext context) {


For reviewers: This functionality was moved to switchToContext. This also helps to privatize the concept of stored data so that other components don't have to be involved. Yay separation of concerns.

tanderson-ld · 2024-03-22T20:45:34Z

...darkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ContextDataManager.java

    }

    /**
-     * Returns the current context.
+     * Switches to providing flag data for the provided context.


For reviewers: You can no longer switch which context this DataManager is concerned with without also attempting to switch the flag data. Weird that was ever an option to begin with...

tanderson-ld · 2024-03-22T20:48:29Z

...darkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ContextDataManager.java

+            if (!context.equals(currentContext)) {
+                // if incoming new data is not for the current context, reject it.
+                return;
+            }


For reviewers: I considered refactoring this code to be willing to store the data for the non-current context if such a situation was to occur, but it seemed unlikely and the refactor would be with risk. There is also an interesting edge case / race condition if that non-current context was not already cached and it put the cache over its cacheLimit.

tanderson-ld · 2024-03-22T20:49:41Z

launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/LDClient.java

+        // 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.


Need to remove copy-pasta'd comment.

tanderson-ld · 2024-03-22T20:51:07Z

Need to add unit test and end to end integration test.

...d-client-sdk/src/test/java/com/launchdarkly/sdk/android/ContextDataManagerListenersTest.java

launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/LDClient.java

...droid-client-sdk/src/androidTest/java/com/launchdarkly/sdk/android/LDClientEndToEndTest.java

kinyoklion · 2024-03-25T22:48:49Z

...darkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ContextDataManager.java

-    }
+        EnvironmentData storedData = getStoredData(currentContext);
+        if (storedData == null) {
+            logger.debug("No stored flag data is available for this context");


Yay logging.

🤖 I have created a release *beep* *boop* --- ## [5.1.1](5.1.0...5.1.1) (2024-03-26) ### Bug Fixes * improved LDClient.identify(...) behavior when offline ([#261](#261)) ([24ce942](24ce942)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

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

9e8a61e

tanderson-ld commented Mar 22, 2024

View reviewed changes

tanderson-ld marked this pull request as ready for review March 22, 2024 20:51

tanderson-ld requested a review from a team March 22, 2024 20:51

kinyoklion reviewed Mar 25, 2024

View reviewed changes

...d-client-sdk/src/test/java/com/launchdarkly/sdk/android/ContextDataManagerListenersTest.java Outdated Show resolved Hide resolved

ci: adding testing related to identify

4150e05

kinyoklion reviewed Mar 25, 2024

View reviewed changes

launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/LDClient.java Outdated Show resolved Hide resolved

Review comments related to unit test and ClientContextImpl volatility.

2b93d4d

kinyoklion reviewed Mar 25, 2024

View reviewed changes

...droid-client-sdk/src/androidTest/java/com/launchdarkly/sdk/android/LDClientEndToEndTest.java Show resolved Hide resolved

kinyoklion reviewed Mar 25, 2024

View reviewed changes

kinyoklion approved these changes Mar 26, 2024

View reviewed changes

tanderson-ld merged commit 24ce942 into main Mar 26, 2024
3 checks passed

tanderson-ld deleted the ta/sc-237349/identify branch March 26, 2024 19:59

github-actions bot mentioned this pull request Mar 26, 2024

chore(main): release 5.1.1 #262

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tanderson-ld commented Mar 22, 2024 •

edited

Loading

shortcut-integration bot commented Mar 22, 2024

tanderson-ld Mar 22, 2024

tanderson-ld Mar 22, 2024

kinyoklion Mar 25, 2024

tanderson-ld Mar 25, 2024

tanderson-ld Mar 22, 2024

tanderson-ld Mar 22, 2024

tanderson-ld Mar 22, 2024

tanderson-ld Mar 22, 2024 •

edited

Loading

kinyoklion Mar 25, 2024

tanderson-ld Mar 22, 2024

tanderson-ld Mar 22, 2024

tanderson-ld Mar 22, 2024 •

edited

Loading

tanderson-ld Mar 22, 2024 •

edited

Loading

tanderson-ld Mar 22, 2024

tanderson-ld commented Mar 22, 2024

kinyoklion Mar 25, 2024

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

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

Conversation

tanderson-ld commented Mar 22, 2024 • edited Loading

shortcut-integration bot commented Mar 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanderson-ld Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanderson-ld Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

tanderson-ld Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanderson-ld commented Mar 22, 2024

Choose a reason for hiding this comment

tanderson-ld commented Mar 22, 2024 •

edited

Loading

tanderson-ld Mar 22, 2024 •

edited

Loading

tanderson-ld Mar 22, 2024 •

edited

Loading

tanderson-ld Mar 22, 2024 •

edited

Loading