Skip to content

Commit b9d2afe

Browse files
novalisdenahiz4kn4fein
novalisdenahi
and
z4kn4fein
authored
Refine locking in ConfigService (#54)
* Refine locking in ConfigService * Fix possible race condition. --------- Co-authored-by: Peter Csajtai <peter.csajtai@outlook.com>
1 parent b366878 commit b9d2afe

File tree

4 files changed

+79
-58
lines changed

4 files changed

+79
-58
lines changed

‎src/main/java/com/configcat/ConfigService.java‎

Lines changed: 43 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,14 @@
1010
import java.util.concurrent.ScheduledExecutorService;
1111
import java.util.concurrent.TimeUnit;
1212
import java.util.concurrent.atomic.AtomicBoolean;
13+
import java.util.concurrent.atomic.AtomicReference;
1314
import java.util.concurrent.locks.ReentrantLock;
1415

1516
public class ConfigService implements Closeable {
1617

1718
private static final String CACHE_BASE = "%s_" + Constants.CONFIG_JSON_NAME + "_" + Constants.SERIALIZATION_FORMAT_VERSION;
1819

19-
private Entry cachedEntry = Entry.EMPTY;
20-
private String cachedEntryString = "";
20+
private final AtomicReference<Entry> cachedEntry = new AtomicReference<>(Entry.EMPTY);
2121
private final ConfigCache cache;
2222
private final String cacheKey;
2323
private final ConfigFetcher configFetcher;
@@ -55,29 +55,29 @@ public ConfigService(String sdkKey,
5555

5656
this.initScheduler = Executors.newSingleThreadScheduledExecutor();
5757
this.initScheduler.schedule(() -> {
58-
lock.lock();
59-
try {
60-
if (initialized.compareAndSet(false, true)) {
61-
this.configCatHooks.invokeOnClientReady(determineCacheState());
58+
if (initialized.compareAndSet(false, true)) {
59+
lock.lock();
60+
try {
61+
this.configCatHooks.invokeOnClientReady(determineCacheState(cachedEntry.get()));
6262
String message = ConfigCatLogMessages.getAutoPollMaxInitWaitTimeReached(autoPollingMode.getMaxInitWaitTimeSeconds());
6363
this.logger.warn(4200, message);
64-
completeRunningTask(Result.error(message, cachedEntry));
64+
completeRunningTask(Result.error(message, cachedEntry.get()));
65+
} finally {
66+
lock.unlock();
6567
}
66-
} finally {
67-
lock.unlock();
6868
}
6969
}, autoPollingMode.getMaxInitWaitTimeSeconds(), TimeUnit.SECONDS);
7070

7171
} else {
7272
// Sync up with cache before reporting ready state
73-
cachedEntry = readCache();
73+
cachedEntry.set(readCache());
7474
setInitialized();
7575
}
7676
}
7777

7878
private void setInitialized() {
7979
if (initialized.compareAndSet(false, true)) {
80-
configCatHooks.invokeOnClientReady(determineCacheState());
80+
configCatHooks.invokeOnClientReady(determineCacheState(cachedEntry.get()));
8181
}
8282
}
8383

@@ -121,27 +121,27 @@ public CompletableFuture<SettingResult> getSettings() {
121121
}
122122

123123
private CompletableFuture<Result<Entry>> fetchIfOlder(long threshold, boolean preferCached) {
124+
// Sync up with the cache and use it when it's not expired.
125+
Entry fromCache = readCache();
126+
if (!fromCache.isEmpty() && !fromCache.getETag().equals(cachedEntry.get().getETag()) && fromCache.getFetchTime() > cachedEntry.get().getFetchTime()) {
127+
configCatHooks.invokeOnConfigChanged(fromCache.getConfig().getEntries());
128+
cachedEntry.set(fromCache);
129+
}
130+
// Cache isn't expired
131+
if (cachedEntry.get().getFetchTime() > threshold) {
132+
setInitialized();
133+
return CompletableFuture.completedFuture(Result.success(cachedEntry.get()));
134+
}
135+
// If we are in offline mode or the caller prefers cached values, do not initiate fetch.
136+
if (offline.get() || preferCached) {
137+
return CompletableFuture.completedFuture(Result.success(cachedEntry.get()));
138+
}
139+
124140
lock.lock();
125141
try {
126-
// Sync up with the cache and use it when it's not expired.
127-
Entry fromCache = readCache();
128-
if (!fromCache.isEmpty() && !fromCache.getETag().equals(cachedEntry.getETag())) {
129-
configCatHooks.invokeOnConfigChanged(fromCache.getConfig().getEntries());
130-
cachedEntry = fromCache;
131-
}
132-
// Cache isn't expired
133-
if (!cachedEntry.isExpired(threshold)) {
134-
setInitialized();
135-
return CompletableFuture.completedFuture(Result.success(cachedEntry));
136-
}
137-
// If we are in offline mode or the caller prefers cached values, do not initiate fetch.
138-
if (offline.get() || preferCached) {
139-
return CompletableFuture.completedFuture(Result.success(cachedEntry));
140-
}
141-
142142
if (runningTask == null) { // No fetch is running, initiate a new one.
143143
runningTask = new CompletableFuture<>();
144-
configFetcher.fetchAsync(cachedEntry.getETag())
144+
configFetcher.fetchAsync(cachedEntry.get().getETag())
145145
.thenAccept(this::processResponse);
146146
}
147147

@@ -194,22 +194,23 @@ public boolean isOffline() {
194194
}
195195

196196
private void processResponse(FetchResponse response) {
197+
Entry previousEntry = cachedEntry.get();
197198
lock.lock();
198199
try {
199200
if (response.isFetched()) {
200201
Entry entry = response.entry();
201-
cachedEntry = entry;
202+
cachedEntry.set(entry);
202203
writeCache(entry);
203204
configCatHooks.invokeOnConfigChanged(entry.getConfig().getEntries());
204205
completeRunningTask(Result.success(entry));
205206
} else {
206207
if (response.isFetchTimeUpdatable()) {
207-
cachedEntry = cachedEntry.withFetchTime(System.currentTimeMillis());
208-
writeCache(cachedEntry);
208+
cachedEntry.set(previousEntry.withFetchTime(System.currentTimeMillis()));
209+
writeCache(cachedEntry.get());
209210
}
210211
completeRunningTask(response.isFailed()
211-
? Result.error(response.error(), cachedEntry)
212-
: Result.success(cachedEntry));
212+
? Result.error(response.error(), cachedEntry.get())
213+
: Result.success(cachedEntry.get()));
213214
}
214215
setInitialized();
215216
} finally {
@@ -225,10 +226,9 @@ private void completeRunningTask(Result<Entry> result) {
225226
private Entry readCache() {
226227
try {
227228
String cachedConfigJson = cache.read(cacheKey);
228-
if (cachedConfigJson != null && cachedConfigJson.equals(cachedEntryString)) {
229+
if (cachedConfigJson != null && cachedConfigJson.equals(cachedEntry.get().getCacheString())) {
229230
return Entry.EMPTY;
230231
}
231-
cachedEntryString = cachedConfigJson;
232232
Entry deserialized = Entry.fromString(cachedConfigJson);
233233
return deserialized == null || deserialized.getConfig() == null ? Entry.EMPTY : deserialized;
234234
} catch (Exception e) {
@@ -239,26 +239,24 @@ private Entry readCache() {
239239

240240
private void writeCache(Entry entry) {
241241
try {
242-
String configToCache = entry.serialize();
243-
cachedEntryString = configToCache;
244-
cache.write(cacheKey, configToCache);
242+
cache.write(cacheKey, entry.getCacheString());
245243
} catch (Exception e) {
246244
logger.error(2201, ConfigCatLogMessages.CONFIG_SERVICE_CACHE_WRITE_ERROR, e);
247245
}
248246
}
249247

250-
private ClientCacheState determineCacheState(){
251-
if(cachedEntry.isEmpty()) {
248+
private ClientCacheState determineCacheState(Entry cachedEntry) {
249+
if (cachedEntry.isEmpty()) {
252250
return ClientCacheState.NO_FLAG_DATA;
253251
}
254-
if(pollingMode instanceof ManualPollingMode) {
252+
if (pollingMode instanceof ManualPollingMode) {
255253
return ClientCacheState.HAS_CACHED_FLAG_DATA_ONLY;
256-
} else if(pollingMode instanceof LazyLoadingMode) {
257-
if(cachedEntry.isExpired(System.currentTimeMillis() - (((LazyLoadingMode)pollingMode).getCacheRefreshIntervalInSeconds() * 1000L))) {
254+
} else if (pollingMode instanceof LazyLoadingMode) {
255+
if (cachedEntry.isExpired(System.currentTimeMillis() - (((LazyLoadingMode) pollingMode).getCacheRefreshIntervalInSeconds() * 1000L))) {
258256
return ClientCacheState.HAS_CACHED_FLAG_DATA_ONLY;
259257
}
260-
} else if(pollingMode instanceof AutoPollingMode) {
261-
if(cachedEntry.isExpired(System.currentTimeMillis() - (((AutoPollingMode)pollingMode).getAutoPollRateInSeconds() * 1000L))) {
258+
} else if (pollingMode instanceof AutoPollingMode) {
259+
if (cachedEntry.isExpired(System.currentTimeMillis() - (((AutoPollingMode) pollingMode).getAutoPollRateInSeconds() * 1000L))) {
262260
return ClientCacheState.HAS_CACHED_FLAG_DATA_ONLY;
263261
}
264262
}

‎src/main/java/com/configcat/Entry.java‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
public class Entry {
44
private final Config config;
55
private final String eTag;
6-
private final String configJson;
6+
private final String cacheString;
77
private final long fetchTime;
88

99
public Config getConfig() {
@@ -18,12 +18,22 @@ public long getFetchTime() {
1818
return fetchTime;
1919
}
2020

21-
public String getConfigJson() {
22-
return configJson;
21+
public String getCacheString() {
22+
return cacheString;
2323
}
2424

2525
public Entry withFetchTime(long fetchTime) {
26-
return new Entry(getConfig(), getETag(), getConfigJson(), fetchTime);
26+
String cacheString = getCacheString();
27+
int fetchTimeIndex = cacheString.indexOf("\n");
28+
if (fetchTimeIndex == -1) {
29+
return this;
30+
}
31+
int eTagIndex = cacheString.indexOf("\n", fetchTimeIndex + 1);
32+
if (eTagIndex == -1) {
33+
return this;
34+
}
35+
String configJson = cacheString.substring(eTagIndex + 1);
36+
return new Entry(getConfig(), getETag(), configJson, fetchTime);
2737
}
2838

2939
public boolean isExpired(long threshold) {
@@ -32,7 +42,7 @@ public boolean isExpired(long threshold) {
3242
public Entry(Config config, String eTag, String configJson, long fetchTime) {
3343
this.config = config;
3444
this.eTag = eTag;
35-
this.configJson = configJson;
45+
this.cacheString = serialize(fetchTime, eTag, configJson);
3646
this.fetchTime = fetchTime;
3747
}
3848

@@ -42,8 +52,8 @@ boolean isEmpty() {
4252

4353
public static final Entry EMPTY = new Entry(Config.EMPTY, "", "", Constants.DISTANT_PAST);
4454

45-
public String serialize() {
46-
return getFetchTime() + "\n" + getETag() + "\n" + getConfigJson();
55+
private static String serialize(long fetchTime, String etag, String configJson) {
56+
return fetchTime + "\n" + etag + "\n" + configJson;
4757
}
4858

4959
public static Entry fromString(String cacheValue) throws IllegalArgumentException {

‎src/test/java/com/configcat/EntrySerializationTest.java‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ void serialize() {
1717
long fetchTime = System.currentTimeMillis();
1818
Entry entry = new Entry(config, "fakeTag", json, fetchTime);
1919

20-
String serializedString = entry.serialize();
20+
String serializedString = entry.getCacheString();
2121

2222
assertEquals(String.format(SERIALIZED_DATA, fetchTime, "fakeTag", json), serializedString);
2323
}
@@ -28,7 +28,7 @@ void payloadSerializationPlatformIndependent() {
2828

2929
Config config = Utils.gson.fromJson(payloadTestConfigJson, Config.class);
3030
Entry entry = new Entry(config, "test-etag", payloadTestConfigJson, 1686756435844L);
31-
String serializedString = entry.serialize();
31+
String serializedString = entry.getCacheString();
3232

3333
assertEquals("1686756435844\ntest-etag\n" + payloadTestConfigJson, serializedString);
3434
}
@@ -42,11 +42,24 @@ void deserialize() {
4242

4343
assertNotNull(entry);
4444
assertEquals("fakeTag", entry.getETag());
45-
assertEquals(json, entry.getConfigJson());
45+
assertEquals(currentTimeMillis + "\nfakeTag\n" + json, entry.getCacheString());
4646
assertEquals(1, entry.getConfig().getEntries().size());
4747
assertEquals(currentTimeMillis, entry.getFetchTime());
4848
}
4949

50+
@Test
51+
void withFetchTime() {
52+
String json = String.format(TEST_JSON, "test", "1");
53+
long currentTimeMillis = System.currentTimeMillis();
54+
55+
Entry entry = Entry.fromString(String.format(SERIALIZED_DATA, currentTimeMillis, "fakeTag", json));
56+
57+
long updatedMillis = System.currentTimeMillis() + 1000;
58+
Entry updated = entry.withFetchTime(updatedMillis);
59+
60+
assertEquals(updatedMillis + "\nfakeTag\n" + json, updated.getCacheString());
61+
}
62+
5063
@Test
5164
void deserializeMissingValue() {
5265
Entry deserializeNull = Entry.fromString(null);

‎src/test/java/com/configcat/Helpers.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,19 +8,19 @@ final class Helpers {
88
static String cacheValueFromConfigJson(String json) {
99
Config config = Utils.gson.fromJson(json, Config.class);
1010
Entry entry = new Entry(config, "fakeTag", json, System.currentTimeMillis());
11-
return entry.serialize();
11+
return entry.getCacheString();
1212
}
1313

1414
static String cacheValueFromConfigJsonWithTime(String json, long time) {
1515
Config config = Utils.gson.fromJson(json, Config.class);
1616
Entry entry = new Entry(config, "fakeTag", json, time);
17-
return entry.serialize();
17+
return entry.getCacheString();
1818
}
1919

2020
static String cacheValueFromConfigJsonWithEtag(String json, String etag) {
2121
Config config = Utils.gson.fromJson(json, Config.class);
2222
Entry entry = new Entry(config, etag, json, System.currentTimeMillis());
23-
return entry.serialize();
23+
return entry.getCacheString();
2424
}
2525

2626
static void waitFor(Supplier<Boolean> predicate) throws InterruptedException {

0 commit comments

Comments
 (0)