diff --git a/lib/build.gradle b/lib/build.gradle index 445fdc88..295b2372 100644 --- a/lib/build.gradle +++ b/lib/build.gradle @@ -29,6 +29,15 @@ plugins { sourceCompatibility = JavaVersion.VERSION_1_8 targetCompatibility = JavaVersion.VERSION_1_8 +// Fail the build when a post-Java-8 API sneaks in, regardless of the JDK that +// compiles it: sourceCompatibility alone checks language level, not API usage. +// Guarded because JDK 8's javac has no --release flag (CI still builds on 8). +tasks.withType(JavaCompile).configureEach { + if (JavaVersion.current().isJava9Compatible()) { + options.release = 8 + } +} + group = 'com.github.growthbook' version = findProperty('version') ?: 'main-SNAPSHOT' diff --git a/lib/src/main/java/growthbook/sdk/java/GrowthBook.java b/lib/src/main/java/growthbook/sdk/java/GrowthBook.java index aa53a564..164c3a85 100644 --- a/lib/src/main/java/growthbook/sdk/java/GrowthBook.java +++ b/lib/src/main/java/growthbook/sdk/java/GrowthBook.java @@ -45,6 +45,13 @@ * GrowthBook SDK class. * Build a context with {@link GBContext#builder()} or the {@link GBContext} constructor * and pass it as an argument to the class constructor. + * + *

Threading: this class is single-threaded by design — one instance serves one + * user, typically created per request and discarded after it (matching the JavaScript + * SDK's per-user {@code GrowthBook} class). Instances share one mutable evaluation + * context internally and must not be used from multiple threads concurrently. For a + * long-lived instance shared across requests and threads, use + * {@link growthbook.sdk.java.multiusermode.GrowthBookClient} instead. */ @Slf4j public class GrowthBook implements IGrowthBook { diff --git a/lib/src/main/java/growthbook/sdk/java/multiusermode/GrowthBookClient.java b/lib/src/main/java/growthbook/sdk/java/multiusermode/GrowthBookClient.java index 71b0b7fb..033bb010 100644 --- a/lib/src/main/java/growthbook/sdk/java/multiusermode/GrowthBookClient.java +++ b/lib/src/main/java/growthbook/sdk/java/multiusermode/GrowthBookClient.java @@ -32,6 +32,7 @@ import growthbook.sdk.java.remoteeval.RemoteEvalResponse; import growthbook.sdk.java.remoteeval.RemoteEvalService; import growthbook.sdk.java.repository.FeatureRefreshStrategy; +import growthbook.sdk.java.repository.FeatureSnapshot; import growthbook.sdk.java.repository.GBFeaturesRepository; import growthbook.sdk.java.repository.RefreshMode; import growthbook.sdk.java.sandbox.CacheManagerFactory; @@ -43,12 +44,13 @@ import javax.annotation.Nullable; import java.time.Duration; -import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; @@ -57,7 +59,7 @@ public class GrowthBookClient { private final Options options; - private List callbacks; + private final List callbacks; private final FeatureEvaluator featureEvaluator; private final Map assigned; private final ExperimentEvaluator experimentEvaluatorEvaluator; @@ -79,8 +81,10 @@ public GrowthBookClient() { public GrowthBookClient(Options opts) { this.options = opts == null ? Options.builder().build() : opts; - this.assigned = new HashMap<>(); - this.callbacks = new ArrayList<>(); + // Shared across request threads: run()/subscribe() mutate these on a client + // that is documented as one-instance-for-all-requests. + this.assigned = new ConcurrentHashMap<>(); + this.callbacks = new CopyOnWriteArrayList<>(); this.featureEvaluator = new FeatureEvaluator(); this.experimentEvaluatorEvaluator = new ExperimentEvaluator(); this.diagnosticsProvider = new GrowthBookClientDiagnosticsProvider(this.options, clientStateView()); @@ -577,19 +581,29 @@ public void onError(Throwable throwable) { } private void fireSubscriptions(Experiment experiment, ExperimentResult result) { - String key = experiment.getKey(); - // If assigned variation has changed, fire subscriptions - AssignedExperiment prev = this.assigned.get(key); - if (prev == null - || !Objects.equals(prev.getInExperiment(), result.getInExperiment()) - || !Objects.equals(prev.getVariationId(), result.getVariationId())) { - AssignedExperiment current = new AssignedExperiment( - experiment.getKey(), - result.getInExperiment(), - result.getVariationId() - ); - this.assigned.put(key, current); + // ConcurrentHashMap rejects null keys (the previous HashMap tolerated them); + // a key-less experiment still dedupes, under one shared sentinel entry. + String key = experiment.getKey() != null ? experiment.getKey() : ""; + // If assigned variation has changed, fire subscriptions. The change check and + // the publish must be one atomic step or two concurrent run() calls can both + // observe the stale value and double-fire. Callbacks run outside compute(): + // user code must never execute inside a ConcurrentHashMap bin lock. + boolean[] changed = {false}; + this.assigned.compute(key, (k, prev) -> { + if (prev == null + || !Objects.equals(prev.getInExperiment(), result.getInExperiment()) + || !Objects.equals(prev.getVariationId(), result.getVariationId())) { + changed[0] = true; + return new AssignedExperiment( + experiment.getKey(), + result.getInExperiment(), + result.getVariationId() + ); + } + return prev; + }); + if (changed[0]) { for (ExperimentRunCallback cb : this.callbacks) { try { cb.onRun(experiment, result); @@ -630,9 +644,12 @@ private synchronized void replaceGlobalContextFrom(GBFeaturesRepository refreshe } private GlobalContext buildGlobalContext(GBFeaturesRepository sourceRepository) { + // Read the payload as ONE snapshot: two separate getter calls could pair + // new features with old saved groups if a refresh lands in between. + FeatureSnapshot featureSnapshot = sourceRepository.getFeatureSnapshot(); return GlobalContext.builder() - .features(sourceRepository.getParsedFeatures()) - .savedGroups(sourceRepository.getParsedSavedGroups()) + .features(featureSnapshot.getParsedFeatures()) + .savedGroups(featureSnapshot.getParsedSavedGroups()) .enabled(this.options.getEnabled()) .qaMode(this.options.getIsQaMode()) .forcedFeatureValues(this.options.getGlobalForcedFeatureValues()) diff --git a/lib/src/main/java/growthbook/sdk/java/multiusermode/configurations/Options.java b/lib/src/main/java/growthbook/sdk/java/multiusermode/configurations/Options.java index 97708c42..d343051b 100644 --- a/lib/src/main/java/growthbook/sdk/java/multiusermode/configurations/Options.java +++ b/lib/src/main/java/growthbook/sdk/java/multiusermode/configurations/Options.java @@ -327,7 +327,9 @@ public StickyBucketService getStickyBucketService() { } public void setInMemoryStickyBucketService() { - this.setStickyBucketService(new InMemoryStickyBucketServiceImpl(new HashMap<>())); + // Thread-safe backing map: this Options instance configures the multi-user + // GrowthBookClient, which evaluates (and therefore saves assignments) concurrently. + this.setStickyBucketService(new InMemoryStickyBucketServiceImpl()); } public void setGlobalAttributes(@Nullable String attributesJson) { diff --git a/lib/src/main/java/growthbook/sdk/java/repository/FeatureSnapshot.java b/lib/src/main/java/growthbook/sdk/java/repository/FeatureSnapshot.java new file mode 100644 index 00000000..ef63f1d6 --- /dev/null +++ b/lib/src/main/java/growthbook/sdk/java/repository/FeatureSnapshot.java @@ -0,0 +1,74 @@ +package growthbook.sdk.java.repository; + +import com.google.gson.JsonObject; +import growthbook.sdk.java.model.Feature; + +import java.util.Collections; +import java.util.Map; + +/** + * Immutable view of one successfully processed features payload: the raw JSON + * strings and their parsed forms, captured together. + * + *

The repository swaps a single {@code AtomicReference} on + * refresh, so a reader always observes features and saved groups from the SAME + * payload. Reading the parts through separate getters across a concurrent + * refresh could otherwise pair new features with old saved groups. + */ +public final class FeatureSnapshot { + public static final FeatureSnapshot EMPTY = new FeatureSnapshot( + GBFeaturesRepository.EMPTY_JSON_OBJECT_STRING, + GBFeaturesRepository.EMPTY_JSON_OBJECT_STRING, + Collections.emptyMap(), + new JsonObject() + ); + + private final String featuresJson; + private final String savedGroupsJson; + private final Map> parsedFeatures; + private final JsonObject parsedSavedGroups; + + FeatureSnapshot(String featuresJson, + String savedGroupsJson, + Map> parsedFeatures, + JsonObject parsedSavedGroups) { + this.featuresJson = featuresJson; + this.savedGroupsJson = savedGroupsJson; + this.parsedFeatures = parsedFeatures; + this.parsedSavedGroups = parsedSavedGroups; + } + + /** + * Builds a snapshot from pre-parsed parts. Intended for tests and callers + * constructing synthetic payloads; the repository builds its own snapshots + * from fetched responses. + * + * @param featuresJson raw features JSON + * @param savedGroupsJson raw saved groups JSON + * @param parsedFeatures parsed feature definitions + * @param parsedSavedGroups parsed saved groups + * @return an immutable snapshot of the supplied parts + */ + public static FeatureSnapshot of(String featuresJson, + String savedGroupsJson, + Map> parsedFeatures, + JsonObject parsedSavedGroups) { + return new FeatureSnapshot(featuresJson, savedGroupsJson, parsedFeatures, parsedSavedGroups); + } + + public String getFeaturesJson() { + return featuresJson; + } + + public String getSavedGroupsJson() { + return savedGroupsJson; + } + + public Map> getParsedFeatures() { + return parsedFeatures; + } + + public JsonObject getParsedSavedGroups() { + return parsedSavedGroups; + } +} diff --git a/lib/src/main/java/growthbook/sdk/java/repository/GBFeaturesRepository.java b/lib/src/main/java/growthbook/sdk/java/repository/GBFeaturesRepository.java index 64737b71..78aef77c 100644 --- a/lib/src/main/java/growthbook/sdk/java/repository/GBFeaturesRepository.java +++ b/lib/src/main/java/growthbook/sdk/java/repository/GBFeaturesRepository.java @@ -51,11 +51,10 @@ import java.net.HttpURLConnection; import java.time.Duration; import java.time.Instant; -import java.util.ArrayList; -import java.util.HashMap; import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ThreadFactory; @@ -83,6 +82,11 @@ public class GBFeaturesRepository implements IGBFeaturesRepository { thread.setDaemon(true); return thread; }; + private static final ThreadFactory POLL_THREAD_FACTORY = runnable -> { + Thread thread = new Thread(runnable, "growthbook-feature-poll"); + thread.setDaemon(true); + return thread; + }; /** * Thread-safe LRU cache with max 100 entries to prevent unbounded growth @@ -179,9 +183,11 @@ public class GBFeaturesRepository implements IGBFeaturesRepository { private OkHttpClient sseHttpClient; /** - * Optional callbacks for getting updates when features are refreshed + * Optional callbacks for getting updates when features are refreshed. + * CopyOnWriteArrayList: registration/clearing happens on caller threads while + * the poll/SSE/retry background threads iterate the list during dispatch. */ - private final ArrayList refreshCallbacks = new ArrayList<>(); + private final CopyOnWriteArrayList refreshCallbacks = new CopyOnWriteArrayList<>(); /** * Flag to know whether GBFeatureRepository is initialized @@ -200,14 +206,34 @@ public class GBFeaturesRepository implements IGBFeaturesRepository { @Nullable private EventSource sseEventSource = null; + /** + * The current features payload — raw JSON and parsed forms captured together + * from one successful refresh and swapped atomically, so readers can never + * observe features from one payload paired with saved groups from another. + */ + private final AtomicReference snapshot = new AtomicReference<>(FeatureSnapshot.EMPTY); + + /** + * The current features payload as one immutable snapshot. Prefer this over the + * individual getters when consuming more than one part of the payload. + * + * @return the snapshot from the most recent successful refresh + */ + public FeatureSnapshot getFeatureSnapshot() { + return this.snapshot.get(); + } + /** * Allows you to get the saved groups JSON from the provided {@link GBFeaturesRepository#getFeaturesEndpoint()}. * You must call {@link GBFeaturesRepository#initialize()} before calling this method * or your saved groups would not have loaded. + * + * @return saved groups JSON string */ - @Getter @Nullable - private volatile String savedGroupsJson = EMPTY_JSON_OBJECT_STRING; + public String getSavedGroupsJson() { + return this.snapshot.get().getSavedGroupsJson(); + } /** * Allows you to get the features JSON from the provided {@link GBFeaturesRepository#getFeaturesEndpoint()}. @@ -216,19 +242,23 @@ public class GBFeaturesRepository implements IGBFeaturesRepository { * * @return feature data JSON in a type of String. Handle refresh strategy */ - @Getter - private volatile String featuresJson = EMPTY_JSON_OBJECT_STRING; + public String getFeaturesJson() { + return this.snapshot.get().getFeaturesJson(); + } /** * Keys are unique identifiers for the features and the values are Feature objects. * Feature definitions - To be pulled from API / Cache + * + * @return parsed feature definitions */ - //@Getter - @Getter - private volatile Map> parsedFeatures = new HashMap<>(); + public Map> getParsedFeatures() { + return this.snapshot.get().getParsedFeatures(); + } - @Getter - private volatile JsonObject parsedSavedGroups = new JsonObject(); + public JsonObject getParsedSavedGroups() { + return this.snapshot.get().getParsedSavedGroups(); + } public void setCacheManager(GbCacheManager cacheManager) { if (!isCacheDisabled) { @@ -549,17 +579,15 @@ public Boolean getLastRefreshLoadedFromCache() { * @param callback This callback will be called when features are refreshed */ @Override - public synchronized void onFeaturesRefresh(FeatureRefreshCallback callback) { + public void onFeaturesRefresh(FeatureRefreshCallback callback) { if (callback == null) { return; } - if (!this.refreshCallbacks.contains(callback)) { - this.refreshCallbacks.add(callback); - } + this.refreshCallbacks.addIfAbsent(callback); } @Override - public synchronized void clearCallbacks() { + public void clearCallbacks() { this.refreshCallbacks.clear(); } @@ -583,8 +611,9 @@ private GbCacheManager createCacheManager() { private void schedulePolling() { if (pollScheduler != null || this.refreshStrategy == FeatureRefreshStrategy.SERVER_SENT_EVENTS) return; - // create single threaded executor - pollScheduler = Executors.newSingleThreadScheduledExecutor(); + // Named daemon thread: a non-daemon poller would keep the JVM alive + // when the application exits without calling shutdown(). + pollScheduler = Executors.newSingleThreadScheduledExecutor(POLL_THREAD_FACTORY); pollScheduler.scheduleWithFixedDelay(this::pollOnceSafe, this.swrTtlSeconds, this.swrTtlSeconds, TimeUnit.SECONDS); } @@ -693,7 +722,7 @@ public void onFeaturesResponse(String featuresJsonResponse) throws FeatureFetchE @Override public void onFeaturesUpdated() { - onRefreshSuccess(featuresJson); + onRefreshSuccess(getFeaturesJson()); recordRefreshSuccess(false); } } @@ -1012,18 +1041,21 @@ private void onResponseJson(String responseJsonString, boolean isFromCache) thro refreshedFeatures = featuresJsonElement.toString().trim(); } - this.featuresJson = refreshedFeatures; - this.savedGroupsJson = refreshedSavedGroups; - - Map> newParsed = TransformationUtil.transformFeatures(this.featuresJson); - JsonObject newSaved = TransformationUtil.transformSavedGroups(this.savedGroupsJson); - this.parsedFeatures = newParsed; - this.parsedSavedGroups = newSaved == null ? new JsonObject() : newSaved; + Map> newParsed = TransformationUtil.transformFeatures(refreshedFeatures); + JsonObject newSaved = TransformationUtil.transformSavedGroups(refreshedSavedGroups); + // One atomic swap: readers never see this payload's features paired + // with a previous payload's saved groups (or vice versa). + this.snapshot.set(new FeatureSnapshot( + refreshedFeatures, + refreshedSavedGroups, + newParsed, + newSaved == null ? new JsonObject() : newSaved + )); this.hasFeatureData.set(true); if (!isFromCache) { this.lastSuccessfulFetchAtMillis.set(System.currentTimeMillis()); - this.onRefreshSuccess(this.featuresJson); + this.onRefreshSuccess(refreshedFeatures); } // bump TTL only after successful processing this.refreshExpiresAt(); @@ -1055,7 +1087,7 @@ private void onRefreshFailed(Throwable throwable) { } public int getActiveFeatureCount() { - return this.parsedFeatures == null ? 0 : this.parsedFeatures.size(); + return this.snapshot.get().getParsedFeatures().size(); } /** @@ -1070,7 +1102,7 @@ private void onSuccess(Response response) throws FeatureFetchException { if (response.code() == HttpURLConnection.HTTP_NOT_MODIFIED) { log.info("Features not modified (304). Using existing data."); this.refreshExpiresAt(); - this.onRefreshSuccess(this.featuresJson); + this.onRefreshSuccess(getFeaturesJson()); recordRefreshSuccess(false); return; } diff --git a/lib/src/main/java/growthbook/sdk/java/stickyBucketing/InMemoryStickyBucketServiceImpl.java b/lib/src/main/java/growthbook/sdk/java/stickyBucketing/InMemoryStickyBucketServiceImpl.java index 17bb2de9..6a9dd508 100644 --- a/lib/src/main/java/growthbook/sdk/java/stickyBucketing/InMemoryStickyBucketServiceImpl.java +++ b/lib/src/main/java/growthbook/sdk/java/stickyBucketing/InMemoryStickyBucketServiceImpl.java @@ -4,6 +4,7 @@ import java.util.HashMap; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; /** * For simple bucket persistence using the in memory's storage(Map) (can be polyfilled for other environments) @@ -11,10 +12,20 @@ public class InMemoryStickyBucketServiceImpl implements StickyBucketService { private final Map localStorage; + /** + * Constructs a new {@code InMemoryStickyBucketServiceImpl} backed by a thread-safe map, + * suitable for use with {@code GrowthBookClient} where evaluations run concurrently. + */ + public InMemoryStickyBucketServiceImpl() { + this(new ConcurrentHashMap<>()); + } + /** * Constructs a new {@code InMemoryStickyBucketServiceImpl} with the specified local storage. * - * @param localStorage a map to store sticky assignments documents in memory. + * @param localStorage a map to store sticky assignments documents in memory. Pass a + * thread-safe map (e.g. {@link ConcurrentHashMap}) when the service is + * shared by a {@code GrowthBookClient} evaluating on multiple threads. */ public InMemoryStickyBucketServiceImpl(Map localStorage) { this.localStorage = localStorage; diff --git a/lib/src/test/java/growthbook/sdk/java/multiusermode/GrowthBookClientConcurrencyTest.java b/lib/src/test/java/growthbook/sdk/java/multiusermode/GrowthBookClientConcurrencyTest.java new file mode 100644 index 00000000..51e33af0 --- /dev/null +++ b/lib/src/test/java/growthbook/sdk/java/multiusermode/GrowthBookClientConcurrencyTest.java @@ -0,0 +1,148 @@ +package growthbook.sdk.java.multiusermode; + +import growthbook.sdk.java.callback.ExperimentRunCallback; +import growthbook.sdk.java.model.Experiment; +import growthbook.sdk.java.multiusermode.configurations.UserContext; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Regression tests for shared-state races on the multi-user client: run() and + * subscribe() are documented as safe on one shared instance, so the assigned + * map and callback list must tolerate concurrent use. + */ +class GrowthBookClientConcurrencyTest { + + private static final int THREADS = 8; + private static final int EXPERIMENTS_PER_THREAD = 50; + + @Test + @Timeout(30) + void concurrentRunAndSubscribeDoesNotCorruptSharedState() throws Exception { + GrowthBookClient client = new GrowthBookClient(); + + List failures = new CopyOnWriteArrayList<>(); + AtomicInteger subscriptionFires = new AtomicInteger(0); + client.subscribe(new ExperimentRunCallback() { + @Override + public void onRun(Experiment experiment, + growthbook.sdk.java.model.ExperimentResult result) { + subscriptionFires.incrementAndGet(); + } + }); + + CountDownLatch start = new CountDownLatch(1); + CountDownLatch done = new CountDownLatch(THREADS); + List threads = new ArrayList<>(); + for (int t = 0; t < THREADS; t++) { + final int threadId = t; + Thread thread = new Thread(() -> { + try { + start.await(); + for (int i = 0; i < EXPERIMENTS_PER_THREAD; i++) { + Experiment experiment = Experiment.builder() + .key("exp-" + threadId + "-" + i) + .variations(new ArrayList<>(Arrays.asList("control", "variant"))) + .build(); + UserContext user = UserContext.builder() + .attributesJson("{\"id\":\"user-" + threadId + "-" + i + "\"}") + .build(); + client.run(experiment, user); + // Concurrent registration while other threads dispatch: + // previously an unsynchronized ArrayList add. + client.subscribe(new ExperimentRunCallback() { + @Override + public void onRun(Experiment e, + growthbook.sdk.java.model.ExperimentResult r) { + // no-op + } + }); + } + } catch (Throwable e) { + failures.add(e); + } finally { + done.countDown(); + } + }); + thread.setDaemon(true); + threads.add(thread); + thread.start(); + } + + start.countDown(); + assertTrue(done.await(20, TimeUnit.SECONDS), "workers did not finish"); + for (Thread thread : threads) { + thread.join(TimeUnit.SECONDS.toMillis(5)); + } + + assertEquals(0, failures.size(), "concurrent run()/subscribe() threw: " + failures); + // Every distinct experiment key fired the pre-registered subscription + // exactly once (each key is assigned once, and the change-check + + // publish is atomic, so no double fire). + assertEquals(THREADS * EXPERIMENTS_PER_THREAD, subscriptionFires.get()); + } + + @Test + @Timeout(30) + void sameExperimentAcrossThreadsFiresSubscriptionOnce() throws Exception { + GrowthBookClient client = new GrowthBookClient(); + + Map firesByKey = new ConcurrentHashMap<>(); + client.subscribe(new ExperimentRunCallback() { + @Override + public void onRun(Experiment experiment, + growthbook.sdk.java.model.ExperimentResult result) { + firesByKey.computeIfAbsent(experiment.getKey(), k -> new AtomicInteger()).incrementAndGet(); + } + }); + + Experiment experiment = Experiment.builder() + .key("shared-exp") + .variations(new ArrayList<>(Arrays.asList("control", "variant"))) + .build(); + // Same user everywhere: the assigned variation never changes, so the + // atomic change-check must collapse all fires into exactly one. + UserContext user = UserContext.builder() + .attributesJson("{\"id\":\"same-user\"}") + .build(); + + CountDownLatch start = new CountDownLatch(1); + CountDownLatch done = new CountDownLatch(THREADS); + List failures = new CopyOnWriteArrayList<>(); + for (int t = 0; t < THREADS; t++) { + Thread thread = new Thread(() -> { + try { + start.await(); + for (int i = 0; i < EXPERIMENTS_PER_THREAD; i++) { + client.run(experiment, user); + } + } catch (Throwable e) { + failures.add(e); + } finally { + done.countDown(); + } + }); + thread.setDaemon(true); + thread.start(); + } + + start.countDown(); + assertTrue(done.await(20, TimeUnit.SECONDS), "workers did not finish"); + assertEquals(0, failures.size(), "concurrent run() threw: " + failures); + assertEquals(1, firesByKey.get("shared-exp").get(), + "unchanged assignment must fire the subscription exactly once"); + } +} diff --git a/lib/src/test/java/growthbook/sdk/java/multiusermode/GrowthBookClientTest.java b/lib/src/test/java/growthbook/sdk/java/multiusermode/GrowthBookClientTest.java index de057a43..7a6bf2d6 100644 --- a/lib/src/test/java/growthbook/sdk/java/multiusermode/GrowthBookClientTest.java +++ b/lib/src/test/java/growthbook/sdk/java/multiusermode/GrowthBookClientTest.java @@ -1,5 +1,6 @@ package growthbook.sdk.java.multiusermode; +import com.google.gson.JsonObject; import com.sun.net.httpserver.HttpServer; import growthbook.sdk.java.callback.ExperimentRunCallback; import growthbook.sdk.java.callback.FeatureRefreshCallback; @@ -16,6 +17,7 @@ import growthbook.sdk.java.multiusermode.configurations.UserContext; import growthbook.sdk.java.multiusermode.util.TransformationUtil; import growthbook.sdk.java.repository.FeatureRefreshStrategy; +import growthbook.sdk.java.repository.FeatureSnapshot; import growthbook.sdk.java.repository.GBFeaturesRepository; import growthbook.sdk.java.repository.RefreshMode; import growthbook.sdk.java.testhelpers.TestCasesJsonHelper; @@ -438,6 +440,7 @@ void refreshGlobalContext_repositoryUpdated_updatesGlobalContextFeatures() { Map> newFeatures = new HashMap<>(); when(mockRepository.getParsedFeatures()).thenReturn(newFeatures); + when(mockRepository.getFeatureSnapshot()).thenReturn(FeatureSnapshot.of("{}", "{}", newFeatures, new JsonObject())); try (MockedStatic mockedStatic = mockStatic(GBFeaturesRepository.class)) { mockedStatic.when(GBFeaturesRepository::builder).thenReturn(mockBuilder); @@ -476,6 +479,7 @@ void getFeatureValue_floatFeature_returnsCorrectValue() { Map> parsedFeatures = TransformationUtil.transformFeatures(demoFeaturesJson); when(mockRepository.getParsedFeatures()).thenReturn(parsedFeatures); + when(mockRepository.getFeatureSnapshot()).thenReturn(FeatureSnapshot.of("{}", "{}", parsedFeatures, new JsonObject())); try (MockedStatic mockedStatic = mockStatic(GBFeaturesRepository.class)) { mockedStatic.when(GBFeaturesRepository::builder).thenReturn(mockBuilder); @@ -516,6 +520,7 @@ void isOn_enabledFeature_returnsTrueAndIsOffReturnsFalse() { Map> parsedFeatures = TransformationUtil.transformFeatures(demoFeaturesJson); when(mockRepository.getParsedFeatures()).thenReturn(parsedFeatures); + when(mockRepository.getFeatureSnapshot()).thenReturn(FeatureSnapshot.of("{}", "{}", parsedFeatures, new JsonObject())); try (MockedStatic mockedStatic = mockStatic(GBFeaturesRepository.class)) { mockedStatic.when(GBFeaturesRepository::builder).thenReturn(mockBuilder); diff --git a/lib/src/test/java/growthbook/sdk/java/multiusermode/GrowthBookClientTestFixtures.java b/lib/src/test/java/growthbook/sdk/java/multiusermode/GrowthBookClientTestFixtures.java index 72820f40..59a48757 100644 --- a/lib/src/test/java/growthbook/sdk/java/multiusermode/GrowthBookClientTestFixtures.java +++ b/lib/src/test/java/growthbook/sdk/java/multiusermode/GrowthBookClientTestFixtures.java @@ -5,6 +5,7 @@ import growthbook.sdk.java.model.Feature; import growthbook.sdk.java.multiusermode.configurations.Options; import growthbook.sdk.java.repository.FeatureRefreshStrategy; +import growthbook.sdk.java.repository.FeatureSnapshot; import growthbook.sdk.java.repository.GBFeaturesRepository; import java.util.HashMap; @@ -34,6 +35,7 @@ static GBFeaturesRepository createMockRepository() { when(repository.getFeaturesJson()).thenReturn("{}"); when(repository.getSavedGroupsJson()).thenReturn("{}"); when(repository.getParsedFeatures()).thenReturn(features); + when(repository.getFeatureSnapshot()).thenReturn(FeatureSnapshot.of("{}", "{}", features, new JsonObject())); when(repository.getParsedSavedGroups()).thenReturn(new JsonObject()); when(repository.getRefreshStrategy()).thenReturn(FeatureRefreshStrategy.STALE_WHILE_REVALIDATE); when(repository.hasFeatureData()).thenReturn(true); diff --git a/lib/src/test/java/growthbook/sdk/java/repository/GBFeaturesRepositoryConcurrencyTest.java b/lib/src/test/java/growthbook/sdk/java/repository/GBFeaturesRepositoryConcurrencyTest.java new file mode 100644 index 00000000..66a28b55 --- /dev/null +++ b/lib/src/test/java/growthbook/sdk/java/repository/GBFeaturesRepositoryConcurrencyTest.java @@ -0,0 +1,182 @@ +package growthbook.sdk.java.repository; + +import growthbook.sdk.java.callback.FeatureRefreshCallback; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +import java.lang.reflect.Method; +import java.util.List; +import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Regression tests for repository-internal races: callback registration on + * caller threads vs. dispatch on the poll/SSE background threads, torn + * features/saved-groups reads across a concurrent refresh, and the polling + * scheduler pinning the JVM with a non-daemon thread. + */ +class GBFeaturesRepositoryConcurrencyTest { + + private static GBFeaturesRepository newRepository() { + return GBFeaturesRepository.builder() + .apiHost("https://cdn.growthbook.io") + .clientKey("sdk-concurrency-test") + .isCacheDisabled(true) + .build(); + } + + private static Method privateMethod(String name, Class... params) throws Exception { + Method method = GBFeaturesRepository.class.getDeclaredMethod(name, params); + method.setAccessible(true); + return method; + } + + @Test + @Timeout(30) + void registeringCallbacksWhileDispatchingDoesNotThrowCme() throws Exception { + GBFeaturesRepository repository = newRepository(); + Method onRefreshSuccess = privateMethod("onRefreshSuccess", String.class); + + List failures = new CopyOnWriteArrayList<>(); + CountDownLatch start = new CountDownLatch(1); + AtomicBoolean stop = new AtomicBoolean(false); + + // Dispatcher: iterates the callback list, as the poll/SSE threads do. + Thread dispatcher = new Thread(() -> { + try { + start.await(); + while (!stop.get()) { + onRefreshSuccess.invoke(repository, "{}"); + } + } catch (Throwable e) { + failures.add(e); + } + }); + dispatcher.setDaemon(true); + + // Registrar: adds callbacks, as initialize() does on the caller thread. + Thread registrar = new Thread(() -> { + try { + start.await(); + for (int i = 0; i < 5_000; i++) { + repository.onFeaturesRefresh(new FeatureRefreshCallback() { + @Override + public void onRefresh(String featuresJson) { + } + + @Override + public void onError(Throwable throwable) { + } + }); + } + repository.clearCallbacks(); + } catch (Throwable e) { + failures.add(e); + } finally { + stop.set(true); + } + }); + registrar.setDaemon(true); + + dispatcher.start(); + registrar.start(); + start.countDown(); + registrar.join(TimeUnit.SECONDS.toMillis(20)); + dispatcher.join(TimeUnit.SECONDS.toMillis(20)); + + assertEquals(0, failures.size(), + "registration concurrent with dispatch threw (was ConcurrentModificationException): " + failures); + } + + @Test + @Timeout(30) + void featureSnapshotIsNeverTornAcrossConcurrentRefresh() throws Exception { + GBFeaturesRepository repository = newRepository(); + Method onResponseJson = privateMethod("onResponseJson", String.class, boolean.class); + + String payloadA = "{\"features\":{\"marker\":{\"defaultValue\":\"A\"}},\"savedGroups\":{\"marker\":[\"A\"]}}"; + String payloadB = "{\"features\":{\"marker\":{\"defaultValue\":\"B\"}},\"savedGroups\":{\"marker\":[\"B\"]}}"; + + List failures = new CopyOnWriteArrayList<>(); + CountDownLatch start = new CountDownLatch(1); + AtomicBoolean stop = new AtomicBoolean(false); + + Thread writer = new Thread(() -> { + try { + start.await(); + for (int i = 0; i < 2_000; i++) { + onResponseJson.invoke(repository, (i % 2 == 0) ? payloadA : payloadB, true); + } + } catch (Throwable e) { + failures.add(e); + } finally { + stop.set(true); + } + }); + writer.setDaemon(true); + + Thread reader = new Thread(() -> { + try { + start.await(); + while (!stop.get()) { + FeatureSnapshot snapshot = repository.getFeatureSnapshot(); + Map features = snapshot.getParsedFeatures(); + if (features.isEmpty()) { + continue; // initial EMPTY snapshot + } + // Both halves must come from the SAME payload. + String featureMarker = snapshot.getFeaturesJson().contains("\"A\"") ? "A" : "B"; + String savedGroupMarker = snapshot.getParsedSavedGroups() + .getAsJsonArray("marker").get(0).getAsString(); + assertEquals(featureMarker, savedGroupMarker, + "torn snapshot: features from one payload, saved groups from another"); + } + } catch (Throwable e) { + failures.add(e); + } + }); + reader.setDaemon(true); + + writer.start(); + reader.start(); + start.countDown(); + writer.join(TimeUnit.SECONDS.toMillis(20)); + reader.join(TimeUnit.SECONDS.toMillis(20)); + + assertEquals(0, failures.size(), "snapshot consistency check failed: " + failures); + } + + @Test + @Timeout(30) + void pollingSchedulerUsesNamedDaemonThread() throws Exception { + GBFeaturesRepository repository = newRepository(); + try { + privateMethod("schedulePolling").invoke(repository); + + Thread pollThread = null; + // The scheduled executor creates its worker eagerly on first schedule; + // scan for it without sleeping. + for (int i = 0; i < 1_000 && pollThread == null; i++) { + for (Thread thread : Thread.getAllStackTraces().keySet()) { + if ("growthbook-feature-poll".equals(thread.getName())) { + pollThread = thread; + break; + } + } + } + + assertNotNull(pollThread, "polling worker thread not found by name"); + assertTrue(pollThread.isDaemon(), + "polling thread must be a daemon or it pins the JVM when the app exits without shutdown()"); + } finally { + repository.shutdown(); + } + } +} diff --git a/lib/src/test/java/growthbook/sdk/java/stickyBucketing/InMemoryStickyBucketServiceImplConcurrencyTest.java b/lib/src/test/java/growthbook/sdk/java/stickyBucketing/InMemoryStickyBucketServiceImplConcurrencyTest.java new file mode 100644 index 00000000..ec2f6a53 --- /dev/null +++ b/lib/src/test/java/growthbook/sdk/java/stickyBucketing/InMemoryStickyBucketServiceImplConcurrencyTest.java @@ -0,0 +1,87 @@ +package growthbook.sdk.java.stickyBucketing; + +import growthbook.sdk.java.model.StickyAssignmentsDocument; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * The no-arg constructor must be safe under the multi-user client's concurrent + * evaluations: parallel saves and reads against one instance. (A plain HashMap + * here is a structural data race, not just a lost update.) + */ +class InMemoryStickyBucketServiceImplConcurrencyTest { + + private static final int THREADS = 8; + private static final int DOCS_PER_THREAD = 500; + + @Test + @Timeout(30) + void concurrentSavesAndReadsAreSafeWithDefaultConstructor() throws Exception { + InMemoryStickyBucketServiceImpl service = new InMemoryStickyBucketServiceImpl(); + + List failures = new CopyOnWriteArrayList<>(); + CountDownLatch start = new CountDownLatch(1); + CountDownLatch done = new CountDownLatch(THREADS); + AtomicBoolean stop = new AtomicBoolean(false); + + for (int t = 0; t < THREADS; t++) { + final int threadId = t; + Thread writer = new Thread(() -> { + try { + start.await(); + for (int i = 0; i < DOCS_PER_THREAD; i++) { + Map assignments = new HashMap<>(); + assignments.put("exp__0", "v" + i); + service.saveAssignments(new StickyAssignmentsDocument( + "id", threadId + "-" + i, assignments)); + } + } catch (Throwable e) { + failures.add(e); + } finally { + done.countDown(); + } + }); + writer.setDaemon(true); + writer.start(); + } + + Thread reader = new Thread(() -> { + try { + start.await(); + while (!stop.get()) { + service.getAllAssignments(Collections.singletonMap("id", "0-0")); + } + } catch (Throwable e) { + failures.add(e); + } + }); + reader.setDaemon(true); + reader.start(); + + start.countDown(); + assertTrue(done.await(20, TimeUnit.SECONDS), "writers did not finish"); + stop.set(true); + reader.join(TimeUnit.SECONDS.toMillis(5)); + + assertEquals(0, failures.size(), "concurrent save/read threw: " + failures); + for (int t = 0; t < THREADS; t++) { + for (int i = 0; i < DOCS_PER_THREAD; i++) { + assertNotNull(service.getAssignments("id", t + "-" + i), + "lost document id||" + t + "-" + i); + } + } + } +}