Better cache initialization #48
Conversation
| private void deleteCacheFiles() { | ||
| deleteFileIfExists(CACHE_FILE_NAME); | ||
| ConfigCacheFile cacheFile = new ConfigCacheFile(ApplicationProvider.getApplicationContext(), DUMMY_API_KEY); | ||
| cacheFile.delete(); |
There was a problem hiding this comment.
We can just leverage the delete functionality of the ConfigCacheFile class, which was doing the same thing.
| SharedPreferences sharedPreferences = ApplicationProvider.getApplicationContext().getSharedPreferences("eppo", Context.MODE_PRIVATE); | ||
| sharedPreferences.edit().clear().commit(); | ||
| } | ||
|
|
||
| private void initClient(String host, boolean throwOnCallackError, boolean shouldDeleteCacheFiles, boolean isGracefulMode) { | ||
| private void initClient(String host, boolean throwOnCallackError, boolean shouldDeleteCacheFiles, boolean isGracefulMode, String apiKey) { |
There was a problem hiding this comment.
Our test harness now accepts an API key as well
| @@ -263,13 +260,13 @@ private void runTestCases() { | |||
| @Test | |||
| public void testCachedAssignments() { | |||
| // First initialize successfully | |||
| initClient(TEST_HOST, false, true, false); // ensure cache is populated | |||
| initClient(TEST_HOST, true, true, false, DUMMY_API_KEY); // ensure cache is populated | |||
There was a problem hiding this comment.
This second argument is whether or not to rethrow errors, which we almost always want to be true unless we are testing graceful mode.
| waitForNonNullAssignment(); | ||
| String assignment = EppoClient.getInstance().getStringAssignment("6255e1a7fc33a9c050ce9508", "randomization_algo"); | ||
| assertEquals("control", assignment); | ||
| String assignment = EppoClient.getInstance().getStringAssignment("6255e1a7d1a3025a26078b95", "randomization_algo"); |
There was a problem hiding this comment.
no need to wait as the completed callback should not be called until fetch succeeds
| throw new RuntimeException(e); | ||
| } | ||
| ConfigCacheFile cacheFile = new ConfigCacheFile(ApplicationProvider.getApplicationContext(), safeCacheKey(DUMMY_API_KEY)); | ||
| cacheFile.setContents("{ invalid }"); |
There was a problem hiding this comment.
we want invalid JSON for this test as the empty object case is much more similar to the newly-added no flags test case
| } | ||
|
|
||
| @Test | ||
| public void testEmptyFlagsResponseRequiresFetch() { |
There was a problem hiding this comment.
Test that reproduced the valid-but-empty cache issue
| } | ||
|
|
||
| @Test | ||
| public void testDifferentCacheFilesPerKey() { |
There was a problem hiding this comment.
Test that reproduced the switching environments issue
| " \"variations\": [\n" + | ||
| " {\n" + | ||
| " \"name\": \"on\",\n" + | ||
| " \"value\": \"true\",\n" + | ||
| " \"typedValue\": true,\n" + | ||
| " \"shardRange\": {\n" + | ||
| " \"start\": 0,\n" + | ||
| " \"end\": 10000\n" + | ||
| " }\n" + | ||
| " }" + | ||
| " ]\n" + |
| /** | ||
| * Useful for passing in as a writer for gson serialization | ||
| */ | ||
| public BufferedWriter getWriter() throws IOException { |
There was a problem hiding this comment.
Switching to buffered writers and readers, which play nicer with larger files.
| } | ||
|
|
||
| public static SharedPreferences getSharedPrefs(Context context, String keySuffix) { | ||
| return context.getSharedPreferences("eppo-"+keySuffix, Context.MODE_PRIVATE); |
There was a problem hiding this comment.
isolate shared preferences by API key
| public static String cacheFileName(String keySuffix) { | ||
| return "eppo-sdk-config-v3-" + keySuffix + ".json"; |
There was a problem hiding this comment.
Isolate cache files by API key
| public static String safeCacheKey(String key) { | ||
| // Take the first eight characters to avoid the key being sensitive information | ||
| // Remove non-alphanumeric characters so it plays nice with filesystem | ||
| return key.substring(0,8).replaceAll("\\W", ""); |
There was a problem hiding this comment.
nit: we use keySuffix when referencing the value returned from this method, but this is returning the prefix
There was a problem hiding this comment.
Ah yeah this is confusing keySuffix refers to us adding a suffix onto the cache key (filename) but we use the prefix of the API key. Since we're removing shared preferences I think I can just rename this to the more descriptive cacheFileNameSuffix
Eppo Internal:
🎟️ Ticket: FF-2092 - Cache should not succeed unless loaded flags & SDK key specific
🗨️ Slack Convo: "...There should be no race between SDK initialization and assignment..."
Description
A user noticed an apparent race condition between initialization and assignment.
What is happening is that the client thinks it successfully populated from a cached configuration, but that configuration is empty. It fires off the completion callback before fetching an updated configuration. Then
getAssignment()is called with the empty configuration before the correct one is fetched.One way this could happen is if an empty configuration is fetched and cached for one environment, and then the API key is changed to hit a different environment that has flags.
Fix
To fix this, we require at least one flag to be present to consider loading from cache "successful".
Also, we make caching per-API key so that when switching between environments caches for the other environments are not used.
Additional Step: Removing use of Shared Preferences (for now)
The Android SDK uses two layers of caching: the filesystem and Shared Preferences. The former is done asynchronously, so the latter was used as a backstop for a very fast assignment request right after initialization. This is a valid but unusual use case, so we've opted to remove the use of Shared Preferences for now, and we can fold it in later if needed.