Skip to content

Commit

Permalink
Handle configuration fetching vs. cache race condition (#49)
Browse files Browse the repository at this point in the history 
* failing test reproducing problem

* ignore cache if fetch completed first

* changes from self-review of PR

* feedback from PR

* bump version number
  • Loading branch information
@aarsilv
aarsilv authored May 21, 2024
1 parent d5b768c commit af501a7
Show file tree
Hide file tree
Showing 5 changed files with 103 additions and 60 deletions.
2 changes: 1 addition & 1 deletion eppo/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ plugins {
}

group = "cloud.eppo"
version = "1.0.9"
version = "1.0.10"

android {
compileSdk 33
Expand Down
101 changes: 61 additions & 40 deletions eppo/src/androidTest/java/cloud/eppo/android/EppoClientTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,14 @@
import java.lang.reflect.Type;
import java.lang.reflect.Field;
import java.util.List;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;

import cloud.eppo.android.dto.EppoValue;
import cloud.eppo.android.dto.FlagConfig;
import cloud.eppo.android.dto.RandomizationConfigResponse;
import cloud.eppo.android.dto.SubjectAttributes;
import cloud.eppo.android.dto.deserializers.EppoValueAdapter;

Expand Down Expand Up @@ -99,9 +102,6 @@ static class AssignmentValueTypeAdapter implements JsonDeserializer<AssignmentVa
this.defaultValue = defaultValue;
}

AssignmentValueTypeAdapter() {
}

@Override
public AssignmentValueType deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context)
throws JsonParseException {
Expand Down Expand Up @@ -159,8 +159,9 @@ public void onError(String errorMessage) {
// Wait for initialization to succeed or fail, up to 10 seconds, before continuing
try {
if (!lock.await(10000, TimeUnit.MILLISECONDS)) {
throw new InterruptedException("Request for RAC did not complete within timeout");
throw new InterruptedException("Client initialization not complete within timeout");
}
Log.d(TAG, "Test client initialized");
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
Expand All @@ -172,6 +173,9 @@ public void clearCaches() {
for (String apiKey : apiKeys) {
clearCacheFile(apiKey);
}
// Reset any development overrides
setHttpClientOverrideField(null);
setConfigurationStoreOverrideField(null);
}

private void clearCacheFile(String apiKey) {
Expand Down Expand Up @@ -371,27 +375,8 @@ public void testInvalidConfigJSON() {
return null; // doAnswer doesn't require a return value
}).when(mockHttpClient).get(anyString(), any(RequestCallback.class));

Field httpClientOverrideField = null;
try {
// Use reflection to set the httpClientOverride field
httpClientOverrideField = EppoClient.class.getDeclaredField("httpClientOverride");
httpClientOverrideField.setAccessible(true);
httpClientOverrideField.set(null, mockHttpClient);


initClient(TEST_HOST, true, true, false, DUMMY_API_KEY);
} catch (NoSuchFieldException | IllegalAccessException e) {
throw new RuntimeException(e);
} finally {
if (httpClientOverrideField != null) {
try {
httpClientOverrideField.set(null, null);
} catch (IllegalAccessException e) {
throw new RuntimeException(e);
}
httpClientOverrideField.setAccessible(false);
}
}
setHttpClientOverrideField(mockHttpClient);
initClient(TEST_HOST, true, true, false, DUMMY_API_KEY);

String result = EppoClient.getInstance().getStringAssignment("dummy subject", "dummy flag");
assertNull(result);
Expand Down Expand Up @@ -429,6 +414,7 @@ public void testDifferentCacheFilesPerKey() {

// Pre-seed a different flag configuration for the other API Key
ConfigCacheFile cacheFile2 = new ConfigCacheFile(ApplicationProvider.getApplicationContext(), safeCacheKey(DUMMY_OTHER_API_KEY));
// Set the experiment_with_boolean_variations flag to always return true
cacheFile2.setContents("{\n" +
" \"flags\": {\n" +
" \"8fc1fb33379d78c8a9edbf43afd6703a\": {\n" +
Expand Down Expand Up @@ -476,6 +462,41 @@ public void testDifferentCacheFilesPerKey() {
assertFalse(apiKey1Assignment);
}

@Test
public void testFetchCompletesBeforeCacheLoad() {
ConfigurationStore slowStore = new ConfigurationStore(ApplicationProvider.getApplicationContext(), safeCacheKey(DUMMY_API_KEY)) {
@Override
protected RandomizationConfigResponse readCacheFile() {
Log.d(TAG, "Simulating slow cache read start");
try {
Thread.sleep(2000);
} catch (InterruptedException ex) {
throw new RuntimeException(ex);
}
RandomizationConfigResponse response = new RandomizationConfigResponse();
ConcurrentHashMap<String, FlagConfig> mockFlags = new ConcurrentHashMap<>();
mockFlags.put("dummy", new FlagConfig()); // make the map non-empty
response.setFlags(mockFlags);

Log.d(TAG, "Simulating slow cache read end");
return response;
}
};

setConfigurationStoreOverrideField(slowStore);
initClient(TEST_HOST, true, false, false, DUMMY_API_KEY);

// Give time for async slow cache read to finish
try {
Thread.sleep(2500);
} catch (InterruptedException ex) {
throw new RuntimeException(ex);
}

String assignment = EppoClient.getInstance().getStringAssignment("6255e1a7d1a3025a26078b95", "randomization_algo");
assertEquals("green", assignment);
}

private void waitForPopulatedCache() {
long waitStart = System.currentTimeMillis();
long waitEnd = waitStart + 10 * 1000; // allow up to 10 seconds
Expand All @@ -497,22 +518,22 @@ private void waitForPopulatedCache() {
}
}

private void waitForNonNullAssignment() {
long waitStart = System.currentTimeMillis();
long waitEnd = waitStart + 15 * 1000; // allow up to 15 seconds
String assignment = null;
private void setHttpClientOverrideField(EppoHttpClient httpClient) {
setOverrideField("httpClientOverride", httpClient);
}

private void setConfigurationStoreOverrideField(ConfigurationStore configurationStore) {
setOverrideField("configurationStoreOverride", configurationStore);
}

private <T> void setOverrideField(String fieldName, T override) {
try {
while (assignment == null) {
if (System.currentTimeMillis() > waitEnd) {
throw new InterruptedException("Non-null assignment never received; assuming configuration not loaded");
}
// Uses third subject in test-case-0
assignment = EppoClient.getInstance().getStringAssignment("6255e1a7fc33a9c050ce9508", "randomization_algo");
if (assignment == null) {
Thread.sleep(100);
}
}
} catch (InterruptedException e) {
// Use reflection to set the httpClientOverride field
Field httpClientOverrideField = EppoClient.class.getDeclaredField(fieldName);
httpClientOverrideField.setAccessible(true);
httpClientOverrideField.set(null, override);
httpClientOverrideField.setAccessible(false);
} catch (NoSuchFieldException | IllegalAccessException e) {
throw new RuntimeException(e);
}
}
Expand Down
19 changes: 12 additions & 7 deletions eppo/src/main/java/cloud/eppo/android/ConfigurationRequestor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,45 +24,50 @@ public ConfigurationRequestor(ConfigurationStore configurationStore, EppoHttpCli
}

public void load(InitializationCallback callback) {
AtomicBoolean cachedUsed = new AtomicBoolean(false);
AtomicBoolean callbackCalled = new AtomicBoolean(false);
configurationStore.loadFromCache(new CacheLoadCallback() {
@Override
public void onCacheLoadSuccess() {
cachedUsed.set(true);
if (callback != null) {
if (callback != null && callbackCalled.compareAndSet(false, true)) {
Log.d(TAG, "Initialized from cache");
callback.onCompleted();
}
}

@Override
public void onCacheLoadFail() {
cachedUsed.set(false);
// no-op; fall-back to Fetch
}
});

Log.d(TAG, "Fetching configuration");
client.get("/api/randomized_assignment/v3/config", new RequestCallback() {
@Override
public void onSuccess(Reader response) {
try {
Log.d(TAG, "Processing fetch response");
configurationStore.setFlagsFromResponse(response);
Log.d(TAG, "Configuration fetch successful");
} catch (JsonSyntaxException | JsonIOException e) {
Log.e(TAG, "Error loading configuration response", e);
if (callback != null && !cachedUsed.get()) {
if (callback != null && callbackCalled.compareAndSet(false, true)) {
Log.d(TAG, "Initialization failure due to fetch response");
callback.onError("Unable to request configuration");
}
return;
}

if (callback != null && !cachedUsed.get()) {
if (callback != null && callbackCalled.compareAndSet(false, true)) {
Log.d(TAG, "Initialized from fetch");
callback.onCompleted();
}
}

@Override
public void onFailure(String errorMessage) {
Log.e(TAG, "Error fetching configuration: " + errorMessage);
if (callback != null && !cachedUsed.get()) {
if (callback != null && callbackCalled.compareAndSet(false, true)) {
Log.d(TAG, "Initialization failure due to fetch error");
callback.onError(errorMessage);
}
}
Expand Down
35 changes: 25 additions & 10 deletions eppo/src/main/java/cloud/eppo/android/ConfigurationStore.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@

import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.IOException;
import java.io.Reader;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.atomic.AtomicBoolean;

import cloud.eppo.android.dto.EppoValue;
import cloud.eppo.android.dto.FlagConfig;
Expand All @@ -33,6 +35,7 @@ public class ConfigurationStore {
.create();
private final ConfigCacheFile cacheFile;

private AtomicBoolean loadedFromFetchResponse = new AtomicBoolean(false);
private ConcurrentHashMap<String, FlagConfig> flags;

public ConfigurationStore(Application application, String cacheFileNameSuffix) {
Expand All @@ -49,19 +52,20 @@ public void loadFromCache(CacheLoadCallback callback) {
AsyncTask.execute(() -> {
Log.d(TAG, "Loading from cache");
try {
RandomizationConfigResponse configResponse;
synchronized (cacheFile) {
BufferedReader reader = cacheFile.getReader();
configResponse = gson.fromJson(reader, RandomizationConfigResponse.class);
reader.close();
}
RandomizationConfigResponse configResponse = readCacheFile();
if (configResponse == null || configResponse.getFlags() == null) {
throw new JsonSyntaxException("Configuration file missing flags");
throw new JsonSyntaxException("Cached configuration file missing flags");
}
flags = configResponse.getFlags();
if (flags.isEmpty()) {
if (configResponse.getFlags().isEmpty()) {
throw new IllegalStateException("Cached configuration file has empty flags");
}
if (loadedFromFetchResponse.get()) {
Log.w(TAG, "Configuration already updated via fetch; ignoring cache");
callback.onCacheLoadFail();
return;
}

flags = configResponse.getFlags();
Log.d(TAG, "Cache loaded successfully");
callback.onCacheLoadSuccess();
} catch (Exception e) {
Expand All @@ -72,14 +76,25 @@ public void loadFromCache(CacheLoadCallback callback) {
});
}

protected RandomizationConfigResponse readCacheFile() throws IOException {
RandomizationConfigResponse configResponse;
synchronized (cacheFile) {
BufferedReader reader = cacheFile.getReader();
configResponse = gson.fromJson(reader, RandomizationConfigResponse.class);
reader.close();
}
return configResponse;
}

public void setFlagsFromResponse(Reader response) {
RandomizationConfigResponse config = gson.fromJson(response, RandomizationConfigResponse.class);
if (config == null || config.getFlags() == null) {
Log.w(TAG, "Flags missing in configuration response");
flags = new ConcurrentHashMap<>();
} else {
loadedFromFetchResponse.set(true); // Record that flags were set from a response so we don't later clobber them with a slow cache read
flags = config.getFlags();
Log.d(TAG, "Loaded" + flags.size() + "flags from configuration response");
Log.d(TAG, "Loaded " + flags.size() + " flags from configuration response");
}

writeConfigToFile(config);
Expand Down
6 changes: 4 additions & 2 deletions eppo/src/main/java/cloud/eppo/android/EppoClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,16 @@ public class EppoClient {
private boolean isGracefulMode;
private static EppoClient instance;

// Useful for testing in situations where we want to mock the http client
// Useful for testing in situations where we want to mock the http client or configuration store
private static EppoHttpClient httpClientOverride = null;
private static ConfigurationStore configurationStoreOverride = null;


private EppoClient(Application application, String apiKey, String host, AssignmentLogger assignmentLogger,
boolean isGracefulMode) {
EppoHttpClient httpClient = httpClientOverride == null ? new EppoHttpClient(host, apiKey) : httpClientOverride;
String cacheFileNameSuffix = safeCacheKey(apiKey); // Cache at a per-API key level (useful for development)
ConfigurationStore configStore = new ConfigurationStore(application, cacheFileNameSuffix);
ConfigurationStore configStore = configurationStoreOverride == null ? new ConfigurationStore(application, cacheFileNameSuffix) : configurationStoreOverride;
requestor = new ConfigurationRequestor(configStore, httpClient);
this.isGracefulMode = isGracefulMode;
this.assignmentLogger = assignmentLogger;
Expand Down

0 comments on commit af501a7

Please sign in to comment.