From 8249490cbf12ce2da1d5b30ce7dde3201a9b1098 Mon Sep 17 00:00:00 2001 From: Tyler Potter Date: Thu, 28 May 2026 23:48:30 -0600 Subject: [PATCH] fix: address threading and resource management feedback on android-sdk-framework MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - volatile on pollingIntervalMs/pollingJitterMs in BaseAndroidClient (builder thread writes, polling thread reads — no synchronization guard existed) - volatile on EppoClient.instance singleton field - FileBackedByteStore: IO_EXECUTOR demoted from static final to instance-level ExecutorService; implements Closeable with shutdown() so tests can reclaim the IO thread - CachingConfigurationStore: remove optimistic update pattern in saveConfiguration(); disk write now happens first and in-memory cache is only updated on success, eliminating the window where getConfiguration() could return an unpersisted value --- .../android/framework/BaseAndroidClient.java | 4 +-- .../storage/CachingConfigurationStore.java | 24 ++++++---------- .../storage/FileBackedByteStore.java | 28 ++++++++++++++----- .../java/cloud/eppo/android/EppoClient.java | 2 +- 4 files changed, 33 insertions(+), 25 deletions(-) diff --git a/android-sdk-framework/src/main/java/cloud/eppo/android/framework/BaseAndroidClient.java b/android-sdk-framework/src/main/java/cloud/eppo/android/framework/BaseAndroidClient.java index 8cce9633..9cdf2da7 100644 --- a/android-sdk-framework/src/main/java/cloud/eppo/android/framework/BaseAndroidClient.java +++ b/android-sdk-framework/src/main/java/cloud/eppo/android/framework/BaseAndroidClient.java @@ -38,8 +38,8 @@ public class BaseAndroidClient extends BaseEppoClientThe disk write is performed first. The in-memory cache is updated only after the write + * succeeds, ensuring that the in-memory state never reflects a configuration that failed to + * persist. Concurrent saves are safe: the last write to complete successfully wins in memory. + * * @param config the configuration to save (must not be null) * @return a future that completes when the write finishes, or completes exceptionally if the * underlying {@link ByteStore} write fails (e.g. with an {@link java.io.IOException}) @@ -53,26 +57,16 @@ protected CachingConfigurationStore( if (config == null) { throw new IllegalArgumentException("config must not be null"); } - Configuration previousConfiguration = configuration.get(); byte[] bytes = codec.toBytes(config); - configuration.set(config); // optimistic update — in-memory reflects last submitted save return byteStore .write(bytes) .whenComplete( (v, ex) -> { - if (ex != null) { - // Revert the optimistic update, but only if no later save has superseded this - // one. compareAndSet atomically checks that the in-memory value is still the - // one we wrote; if a concurrent save has already advanced it, the revert is - // skipped. - // - // Edge case: if two concurrent saves both fail their IO writes, the second - // failure's revert may land on a value that was itself never persisted (the - // first save's value). This is acceptable — the in-memory state may diverge - // from disk, but the next successful save will reconcile them. Saves are - // serialized through a single-thread IO_EXECUTOR, so true concurrent IO - // failures are unlikely in practice. - configuration.compareAndSet(config, previousConfiguration); + if (ex == null) { + // Only update in-memory cache after a successful disk write. + // Use set() rather than compareAndSet() so the most-recently-persisted + // configuration always wins, regardless of submission order. + configuration.set(config); } }); } diff --git a/android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/FileBackedByteStore.java b/android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/FileBackedByteStore.java index 9cd27227..2b97636f 100644 --- a/android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/FileBackedByteStore.java +++ b/android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/FileBackedByteStore.java @@ -1,19 +1,24 @@ package cloud.eppo.android.framework.storage; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.Executor; +import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import org.jetbrains.annotations.NotNull; /** * {@link ByteStore} implementation that reads and writes a single file via {@link BaseCacheFile}. + * + *

Implements {@link java.io.Closeable}. Call {@link #close()} when the store is no longer needed + * to release the background IO thread. If not closed explicitly, the daemon thread will be + * reclaimed by the JVM/Android runtime on process exit. */ -public final class FileBackedByteStore implements ByteStore { +public final class FileBackedByteStore implements ByteStore, java.io.Closeable { // Dedicated single-thread executor avoids saturating ForkJoinPool.commonPool() with blocking I/O - // on low-core-count Android devices. Daemon thread so the executor does not block JVM/process - // exit in test environments (e.g. Robolectric). - private static final Executor IO_EXECUTOR = + // on low-core-count Android devices. Instance-level (not static) so it can be shut down via + // close(). The thread is a daemon so it does not block JVM/process exit when close() is omitted + // (e.g. in test environments such as Robolectric). + private final ExecutorService ioExecutor = Executors.newSingleThreadExecutor( r -> { Thread t = new Thread(r, "eppo-io"); @@ -30,6 +35,15 @@ public FileBackedByteStore(@NotNull BaseCacheFile cacheFile) { this.cacheFile = cacheFile; } + /** + * Shuts down the background IO executor. In-flight operations are allowed to complete; no new + * operations will be accepted after this call. + */ + @Override + public void close() { + ioExecutor.shutdown(); + } + @Override @NotNull public CompletableFuture read() { return CompletableFuture.supplyAsync( @@ -43,7 +57,7 @@ public FileBackedByteStore(@NotNull BaseCacheFile cacheFile) { throw new RuntimeException("Failed to read from cache file", e); } }, - IO_EXECUTOR); + ioExecutor); } @Override @@ -59,7 +73,7 @@ public FileBackedByteStore(@NotNull BaseCacheFile cacheFile) { throw new RuntimeException("Failed to write to cache file", e); } }, - IO_EXECUTOR); + ioExecutor); } private static byte[] readAllBytes(java.io.InputStream in) throws java.io.IOException { diff --git a/eppo/src/main/java/cloud/eppo/android/EppoClient.java b/eppo/src/main/java/cloud/eppo/android/EppoClient.java index 2b3506d2..8dd02355 100644 --- a/eppo/src/main/java/cloud/eppo/android/EppoClient.java +++ b/eppo/src/main/java/cloud/eppo/android/EppoClient.java @@ -34,7 +34,7 @@ public class EppoClient extends BaseEppoClient { private long pollingIntervalMs, pollingJitterMs; - @Nullable private static EppoClient instance; + @Nullable private static volatile EppoClient instance; private EppoClient( String apiKey,