From 0b95531a4d0ad13d72bed631071d6483d8401403 Mon Sep 17 00:00:00 2001 From: wadii Date: Tue, 22 Sep 2026 16:12:14 +0200 Subject: [PATCH 01/16] feat: add experimentation support Parse experiment metadata from remote evaluation and add an opt-in event processor so SDK users can resolve experiment flags and record exposures. - Flag and FeatureStateModel now carry variant, reason and experiment (metadata.experiment), populated by remote evaluation only. Local evaluation sets reason from FlagResult; variant and experiment stay null because the environment document has no variant keys. - New EventProcessor buffers events and POSTs {"events": [...]} to {eventsUri}v1/events, flushing on a 10s timer, at 1000 buffered events and on close(). Exposures are deduplicated per flush window; a failed batch is retried once on a connection error or 5xx, never on 4xx, then dropped. Nothing thrown inside it reaches caller code. - New client methods: getExperimentFlag, trackEvent, trackExposureEvent and flushEvents. close() now also closes the event processor. - Opt in with FlagsmithConfig.Builder.withEnableEvents(true). Configuring the buffer, interval or events URI without enabling events is rejected at build time, as is enabling events in offline mode. - Retry gains an opt-in statusForcelistOnly flag so a force-listed status respects the attempts budget instead of retrying forever. The default stays false, preserving existing behaviour. - RequestProcessor gains submit(), returning a CompletableFuture so the event processor can compose on batch completion. Nothing changes for users who do not opt in. --- .../java/com/flagsmith/FlagsmithClient.java | 200 +++++++++ .../com/flagsmith/config/FlagsmithConfig.java | 89 +++++ src/main/java/com/flagsmith/config/Retry.java | 32 +- .../com/flagsmith/mappers/EngineMappers.java | 1 + .../flagsmith/models/ExperimentMetadata.java | 19 + src/main/java/com/flagsmith/models/Flag.java | 35 ++ .../models/features/FeatureStateMetadata.java | 13 + .../models/features/FeatureStateModel.java | 5 +- .../com/flagsmith/threads/EventProcessor.java | 305 ++++++++++++++ .../flagsmith/threads/RequestProcessor.java | 19 + .../com/flagsmith/FlagsmithClientTest.java | 278 +++++++++++++ .../com/flagsmith/FlagsmithTestHelper.java | 84 ++++ .../flagsmith/flagengine/models/FlagTest.java | 67 +++- .../models/FeatureStateModelTest.java | 71 ++++ .../flagsmith/threads/EventProcessorTest.java | 378 ++++++++++++++++++ 15 files changed, 1585 insertions(+), 11 deletions(-) create mode 100644 src/main/java/com/flagsmith/models/ExperimentMetadata.java create mode 100644 src/main/java/com/flagsmith/models/features/FeatureStateMetadata.java create mode 100644 src/main/java/com/flagsmith/threads/EventProcessor.java create mode 100644 src/test/java/com/flagsmith/models/FeatureStateModelTest.java create mode 100644 src/test/java/com/flagsmith/threads/EventProcessorTest.java diff --git a/src/main/java/com/flagsmith/FlagsmithClient.java b/src/main/java/com/flagsmith/FlagsmithClient.java index dd2917de..4dd259fe 100644 --- a/src/main/java/com/flagsmith/FlagsmithClient.java +++ b/src/main/java/com/flagsmith/FlagsmithClient.java @@ -13,19 +13,24 @@ import com.flagsmith.interfaces.FlagsmithSdk; import com.flagsmith.mappers.EngineMappers; import com.flagsmith.models.BaseFlag; +import com.flagsmith.models.ExperimentMetadata; +import com.flagsmith.models.Flag; import com.flagsmith.models.Flags; import com.flagsmith.models.Segment; import com.flagsmith.models.SegmentMetadata; +import com.flagsmith.threads.EventProcessor; import com.flagsmith.threads.PollingManager; import com.flagsmith.utils.ModelUtils; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.concurrent.CompletableFuture; import java.util.function.Function; import java.util.stream.Collectors; import lombok.Data; import lombok.NonNull; +import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -197,6 +202,170 @@ public List getIdentitySegments(String identifier, Map }).filter(Objects::nonNull).collect(Collectors.toList()); } + /** + * Resolve a flag for an identity and record one {@code $flag_exposure} event when the identity + * is enrolled in a running experiment on that feature. Identity flags are fetched exactly as + * {@link #getIdentityFlags(String)} fetches them. + * + *

Experiment metadata is only carried by remote evaluation. With local evaluation or offline + * mode the flag is still returned but no exposure is recorded. + * + * @param featureName feature name + * @param identifier identifier string + * @return the flag for the given feature + * @throws FlagsmithRuntimeError when events are not enabled + */ + public BaseFlag getExperimentFlag(String featureName, String identifier) + throws FlagsmithClientError { + return getExperimentFlag(featureName, identifier, new HashMap<>()); + } + + /** + * Resolve a flag for an identity and record one {@code $flag_exposure} event when the identity + * is enrolled in a running experiment on that feature. Identity flags are fetched exactly as + * {@link #getIdentityFlags(String, Map)} fetches them. + * + *

Experiment metadata is only carried by remote evaluation. With local evaluation or offline + * mode the flag is still returned but no exposure is recorded. + * + * @param featureName feature name + * @param identifier identifier string + * @param traits a map of trait keys to trait values + * @return the flag for the given feature + * @throws FlagsmithRuntimeError when events are not enabled + */ + public BaseFlag getExperimentFlag( + String featureName, String identifier, Map traits) + throws FlagsmithClientError { + requireEventProcessor("get experiment flags"); + + Flags flags = getIdentityFlags(identifier, traits); + BaseFlag flag = flags.getFlag(featureName); + + if (!(flag instanceof Flag)) { + logger.info("Not recording an exposure for feature {}: served by the default flag handler.", + featureName); + return flag; + } + + if (!Boolean.TRUE.equals(flag.getEnabled())) { + logger.info("Not recording an exposure for feature {}: the flag is disabled.", featureName); + return flag; + } + + ExperimentMetadata experiment = ((Flag) flag).getExperiment(); + if (experiment == null || !Boolean.TRUE.equals(experiment.getInExperiment())) { + logger.info("Not recording an exposure for feature {}: the identity is not enrolled in a " + + "running experiment.", featureName); + return flag; + } + + Map metadata = new HashMap<>(); + metadata.put("experiment_id", experiment.getId()); + trackExposureEvent(featureName, identifier, ((Flag) flag).getVariant(), traits, metadata); + + return flag; + } + + /** + * Record a custom event. + * + * @param event event name + * @throws FlagsmithRuntimeError when events are not enabled + * @throws IllegalArgumentException when the event name starts with "$" + */ + public void trackEvent(String event) { + trackEvent(event, null, null, null, null); + } + + /** + * Record a custom event for an identity. + * + * @param event event name + * @param identifier identifier string + * @throws FlagsmithRuntimeError when events are not enabled + * @throws IllegalArgumentException when the event name starts with "$" + */ + public void trackEvent(String event, String identifier) { + trackEvent(event, identifier, null, null, null); + } + + /** + * Record a custom event for an identity, with a value, traits and metadata. + * + * @param event event name + * @param identifier identifier string + * @param value event value, stringified before sending + * @param traits a map of trait keys to trait values + * @param metadata a map of metadata to attach to the event + * @throws FlagsmithRuntimeError when events are not enabled + * @throws IllegalArgumentException when the event name starts with "$" + */ + public void trackEvent(String event, String identifier, Object value, + Map traits, Map metadata) { + EventProcessor processor = requireEventProcessor("track events"); + + if (event != null && event.startsWith("$")) { + throw new IllegalArgumentException("Event names starting with \"$\" are reserved; use " + + "trackExposureEvent to record \"" + EventProcessor.FLAG_EXPOSURE_EVENT + "\"."); + } + + processor.trackEvent(event, identifier, value, traits, metadata); + } + + /** + * Record a {@code $flag_exposure} event. Skipped, with a log line, when the identifier is + * blank. + * + * @param featureName feature the identity was exposed to + * @param identifier identifier string + * @param value variant the identity was bucketed into + * @throws FlagsmithRuntimeError when events are not enabled + */ + public void trackExposureEvent(String featureName, String identifier, Object value) { + trackExposureEvent(featureName, identifier, value, null, null); + } + + /** + * Record a {@code $flag_exposure} event, with traits and metadata. Skipped, with a log line, + * when the identifier is blank. + * + * @param featureName feature the identity was exposed to + * @param identifier identifier string + * @param value variant the identity was bucketed into + * @param traits a map of trait keys to trait values + * @param metadata a map of metadata to attach to the event + * @throws FlagsmithRuntimeError when events are not enabled + */ + public void trackExposureEvent(String featureName, String identifier, Object value, + Map traits, Map metadata) { + EventProcessor processor = requireEventProcessor("track exposure events"); + + if (StringUtils.isBlank(identifier)) { + logger.info("Not sending {} for feature {}: an exposure requires an identifier.", + EventProcessor.FLAG_EXPOSURE_EVENT, featureName); + return; + } + + processor.trackExposureEvent(featureName, identifier, value, traits, metadata); + } + + /** + * Send buffered events now. + * + * @return a future completing once every in-flight batch is done, already completed when events + * are not enabled + */ + public CompletableFuture flushEvents() { + EventProcessor processor = getEventProcessor(); + + if (processor == null) { + return CompletableFuture.completedFuture(null); + } + + return processor.flush(); + } + /** * Should be called when terminating the client to clean up any resources that * need cleaning up. @@ -205,9 +374,31 @@ public void close() { if (pollingManager != null) { pollingManager.stopPolling(); } + + EventProcessor eventProcessor = getEventProcessor(); + if (eventProcessor != null) { + eventProcessor.close(); + } + flagsmithSdk.close(); } + private EventProcessor getEventProcessor() { + FlagsmithConfig config = getConfig(); + return config != null ? config.getEventProcessor() : null; + } + + private EventProcessor requireEventProcessor(String action) { + EventProcessor processor = getEventProcessor(); + + if (processor == null) { + throw new FlagsmithRuntimeError( + "Events must be enabled to " + action + ". Use withEnableEvents(true)."); + } + + return processor; + } + private Flags getEnvironmentFlagsFromEvaluationContext() throws FlagsmithClientError { if (evaluationContext == null) { if (getConfig().getFlagsmithFlagDefaults() == null) { @@ -525,6 +716,15 @@ public FlagsmithClient build() { configuration.getAnalyticsProcessor().setLogger(client.logger); } + if (configuration.getEventProcessor() != null) { + if (configuration.getOfflineMode()) { + throw new FlagsmithRuntimeError("Events cannot be enabled in offline mode."); + } + configuration.getEventProcessor().setApi(client.flagsmithSdk); + configuration.getEventProcessor().setLogger(client.logger); + configuration.getEventProcessor().start(); + } + if (configuration.getEnableLocalEvaluation()) { if (configuration.getOfflineHandler() != null) { throw new FlagsmithRuntimeError( diff --git a/src/main/java/com/flagsmith/config/FlagsmithConfig.java b/src/main/java/com/flagsmith/config/FlagsmithConfig.java index 33d8cd1f..b711c916 100644 --- a/src/main/java/com/flagsmith/config/FlagsmithConfig.java +++ b/src/main/java/com/flagsmith/config/FlagsmithConfig.java @@ -3,6 +3,7 @@ import com.flagsmith.FlagsmithFlagDefaults; import com.flagsmith.interfaces.IOfflineHandler; import com.flagsmith.threads.AnalyticsProcessor; +import com.flagsmith.threads.EventProcessor; import java.net.Proxy; import java.util.ArrayList; import java.util.List; @@ -30,17 +31,23 @@ public final class FlagsmithConfig { private static final int DEFAULT_ENVIRONMENT_REFRESH_SECONDS = 60; private static final HttpUrl DEFAULT_BASE_URI = HttpUrl .get("https://edge.api.flagsmith.com/api/v1/"); + private static final HttpUrl DEFAULT_EVENTS_URI = HttpUrl + .get("https://events.api.flagsmith.com/"); + private static final int DEFAULT_EVENTS_MAX_BUFFER_ITEMS = 1000; + private static final int DEFAULT_EVENTS_FLUSH_INTERVAL_MILLIS = 10000; private final HttpUrl flagsUri; private final HttpUrl identitiesUri; private final HttpUrl traitsUri; private final HttpUrl environmentUri; private final OkHttpClient httpClient; private final HttpUrl baseUri; + private final HttpUrl eventsUri; private final Retry retries; private Boolean enableLocalEvaluation; private Integer environmentRefreshIntervalSeconds; private AnalyticsProcessor analyticsProcessor; + private EventProcessor eventProcessor; private FlagsmithFlagDefaults flagsmithFlagDefaults = null; private Boolean raiseUpdateEnvironmentErrorsOnStartup = true; private Boolean offlineMode = false; @@ -88,6 +95,18 @@ protected FlagsmithConfig(Builder builder) { } } + this.eventsUri = builder.eventsUri; + + if (builder.enableEvents) { + eventProcessor = builder.eventProcessor != null + ? builder.eventProcessor + : new EventProcessor(httpClient, eventsUri, builder.eventsMaxBufferItems, + builder.eventsFlushIntervalMillis, DEFAULT_READ_TIMEOUT_MILLIS); + } else if (builder.eventsConfigured) { + throw new IllegalArgumentException( + "Events must be enabled with withEnableEvents(true) to configure the event processor."); + } + this.offlineMode = builder.offlineMode; this.offlineHandler = builder.offlineHandler; } @@ -120,6 +139,13 @@ public static class Builder { private Integer environmentRefreshIntervalSeconds = DEFAULT_ENVIRONMENT_REFRESH_SECONDS; private Boolean enableAnalytics = Boolean.FALSE; + private HttpUrl eventsUri = DEFAULT_EVENTS_URI; + private EventProcessor eventProcessor; + private Boolean enableEvents = Boolean.FALSE; + private Boolean eventsConfigured = Boolean.FALSE; + private int eventsMaxBufferItems = DEFAULT_EVENTS_MAX_BUFFER_ITEMS; + private int eventsFlushIntervalMillis = DEFAULT_EVENTS_FLUSH_INTERVAL_MILLIS; + private Boolean offlineMode = Boolean.FALSE; private IOfflineHandler offlineHandler; @@ -272,6 +298,69 @@ public Builder withEnableAnalytics(Boolean enable) { return this; } + /** + * Set the base URL of the Flagsmith events API, overriding the default one. + * + * @param eventsUri the new base URI for the events API + * @return the Builder + */ + public Builder eventsUri(String eventsUri) { + if (eventsUri != null) { + this.eventsUri = HttpUrl.get(eventsUri.endsWith("/") ? eventsUri : eventsUri + "/"); + this.eventsConfigured = Boolean.TRUE; + } + return this; + } + + /** + * Enable the event processor, which records experiment exposures and custom events. + * + * @param enable boolean to enable + * @return the Builder + */ + public Builder withEnableEvents(Boolean enable) { + this.enableEvents = enable; + return this; + } + + /** + * Set the event processor. + * + * @param processor event processor object + * @return the Builder + */ + public Builder withEventProcessor(EventProcessor processor) { + this.eventProcessor = processor; + this.enableEvents = Boolean.TRUE; + return this; + } + + /** + * Set the number of buffered events that triggers an immediate flush. Requires events to be + * enabled. + * + * @param items the maximum number of buffered events + * @return the Builder + */ + public Builder withEventsMaxBufferItems(int items) { + this.eventsMaxBufferItems = items; + this.eventsConfigured = Boolean.TRUE; + return this; + } + + /** + * Set the interval between timed event flushes, in milliseconds. Zero disables the timer. + * Requires events to be enabled. + * + * @param millis the flush interval in milliseconds + * @return the Builder + */ + public Builder withEventsFlushIntervalMillis(int millis) { + this.eventsFlushIntervalMillis = millis; + this.eventsConfigured = Boolean.TRUE; + return this; + } + /** * Enable offline mode. * diff --git a/src/main/java/com/flagsmith/config/Retry.java b/src/main/java/com/flagsmith/config/Retry.java index aedec811..fa70151f 100644 --- a/src/main/java/com/flagsmith/config/Retry.java +++ b/src/main/java/com/flagsmith/config/Retry.java @@ -25,17 +25,47 @@ public class Retry { add(429); add(503); }}; + /** + * When true, a response is only retried if its status code is in {@link #statusForcelist}, and + * never beyond the {@link #total} attempts budget. A connection failure (a null status code) is + * still retried while attempts remain. Defaults to false, which keeps the historical behaviour + * of retrying any status while attempts remain, and retrying a force-listed status regardless of + * the budget. + */ + private Boolean statusForcelistOnly = Boolean.FALSE; public Retry(Integer total) { this.total = total; } + /** + * Instantiate without a status forcelist policy, which keeps the historical behaviour. + * + * @param total number of attempts before giving up + * @param attempts attempts made so far + * @param backoffFactor factor applied to the backoff between attempts + * @param backoffMax upper bound on the backoff, in seconds + * @param statusForcelist status codes that are always retried + */ + public Retry(Integer total, Integer attempts, Float backoffFactor, Float backoffMax, + Set statusForcelist) { + this(total, attempts, backoffFactor, backoffMax, statusForcelist, Boolean.FALSE); + } + /** * Should Retry or not?. * - * @param statusCode status code of last call + * @param statusCode status code of last call, or null if the call did not get a response */ public Boolean isRetry(Integer statusCode) { + if (Boolean.TRUE.equals(statusForcelistOnly)) { + if (total <= attempts) { + return Boolean.FALSE; + } + return statusCode == null + || (statusForcelist != null && statusForcelist.contains(statusCode)); + } + if (statusForcelist != null && !statusForcelist.isEmpty() && statusForcelist.contains(statusCode)) { return Boolean.TRUE; diff --git a/src/main/java/com/flagsmith/mappers/EngineMappers.java b/src/main/java/com/flagsmith/mappers/EngineMappers.java index 3beee22e..c9a23aaf 100644 --- a/src/main/java/com/flagsmith/mappers/EngineMappers.java +++ b/src/main/java/com/flagsmith/mappers/EngineMappers.java @@ -62,6 +62,7 @@ public static Flag mapFlagResultToFlag( flag.setFeatureName(flagResult.getName()); flag.setValue(flagResult.getValue()); flag.setEnabled(flagResult.getEnabled()); + flag.setReason(flagResult.getReason()); return flag; } diff --git a/src/main/java/com/flagsmith/models/ExperimentMetadata.java b/src/main/java/com/flagsmith/models/ExperimentMetadata.java new file mode 100644 index 00000000..12d17303 --- /dev/null +++ b/src/main/java/com/flagsmith/models/ExperimentMetadata.java @@ -0,0 +1,19 @@ +package com.flagsmith.models; + +import com.fasterxml.jackson.annotation.JsonProperty; +import lombok.Data; + +/** + * Details of the experiment running on a feature, as returned by remote evaluation. + */ +@Data +public class ExperimentMetadata { + private Integer id; + private String name; + /** + * Whether this identity is enrolled in the experiment. The variant alone cannot tell: an + * identity outside the rollout is still bucketed into a variant. + */ + @JsonProperty("in_experiment") + private Boolean inExperiment = Boolean.FALSE; +} diff --git a/src/main/java/com/flagsmith/models/Flag.java b/src/main/java/com/flagsmith/models/Flag.java index 990976b9..625f4c9d 100644 --- a/src/main/java/com/flagsmith/models/Flag.java +++ b/src/main/java/com/flagsmith/models/Flag.java @@ -1,6 +1,8 @@ package com.flagsmith.models; import com.fasterxml.jackson.databind.JsonNode; +import com.flagsmith.MapperFactory; +import com.flagsmith.models.features.FeatureStateMetadata; import com.flagsmith.models.features.FeatureStateModel; import lombok.Data; @@ -8,6 +10,18 @@ public class Flag extends BaseFlag { private Integer featureId = 0; private Boolean isDefault; + /** + * Variant key the identity was bucketed into. Set by remote evaluation only; null otherwise. + */ + private String variant; + /** + * Evaluation reason, e.g. "DEFAULT", "SPLIT; weight=70.0", "TARGETING_MATCH; segment=...". + */ + private String reason; + /** + * Experiment running on this feature. Set by remote identity evaluation only; null otherwise. + */ + private ExperimentMetadata experiment; /** * return flag from feature state model and identity id. @@ -21,6 +35,10 @@ public static Flag fromFeatureStateModel(FeatureStateModel featureState) { flag.setValue(featureState.getValue()); flag.setFeatureName(featureState.getFeature().getName()); flag.setEnabled(featureState.getEnabled()); + flag.setVariant(featureState.getVariant()); + flag.setReason(featureState.getReason()); + flag.setExperiment(featureState.getMetadata() != null + ? featureState.getMetadata().getExperiment() : null); return flag; } @@ -38,6 +56,23 @@ public static Flag fromApiFlag(JsonNode node) { flag.setFeatureName(node.get("feature").get("name").asText()); flag.setEnabled(node.get("enabled").booleanValue()); + JsonNode variant = node.get("variant"); + if (variant != null && !variant.isNull()) { + flag.setVariant(variant.asText()); + } + + JsonNode reason = node.get("reason"); + if (reason != null && !reason.isNull()) { + flag.setReason(reason.asText()); + } + + JsonNode metadata = node.get("metadata"); + if (metadata != null && !metadata.isNull()) { + flag.setExperiment(MapperFactory.getMapper() + .convertValue(metadata, FeatureStateMetadata.class) + .getExperiment()); + } + return flag; } } diff --git a/src/main/java/com/flagsmith/models/features/FeatureStateMetadata.java b/src/main/java/com/flagsmith/models/features/FeatureStateMetadata.java new file mode 100644 index 00000000..007a4e3c --- /dev/null +++ b/src/main/java/com/flagsmith/models/features/FeatureStateMetadata.java @@ -0,0 +1,13 @@ +package com.flagsmith.models.features; + +import com.flagsmith.models.ExperimentMetadata; +import lombok.Data; + +/** + * The {@code metadata} object returned alongside a remotely evaluated feature state. Keys other + * than {@code experiment} are ignored. + */ +@Data +public class FeatureStateMetadata { + private ExperimentMetadata experiment; +} diff --git a/src/main/java/com/flagsmith/models/features/FeatureStateModel.java b/src/main/java/com/flagsmith/models/features/FeatureStateModel.java index ccaef00f..a1c180db 100644 --- a/src/main/java/com/flagsmith/models/features/FeatureStateModel.java +++ b/src/main/java/com/flagsmith/models/features/FeatureStateModel.java @@ -20,4 +20,7 @@ public class FeatureStateModel extends BaseModel { private Object value; @JsonProperty("feature_segment") private FeatureSegmentModel featureSegment; -} \ No newline at end of file + private String variant; + private String reason; + private FeatureStateMetadata metadata; +} diff --git a/src/main/java/com/flagsmith/threads/EventProcessor.java b/src/main/java/com/flagsmith/threads/EventProcessor.java new file mode 100644 index 00000000..da9b0193 --- /dev/null +++ b/src/main/java/com/flagsmith/threads/EventProcessor.java @@ -0,0 +1,305 @@ +package com.flagsmith.threads; + +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.JsonNode; +import com.flagsmith.FlagsmithLogger; +import com.flagsmith.MapperFactory; +import com.flagsmith.Versions; +import com.flagsmith.config.Retry; +import com.flagsmith.interfaces.FlagsmithSdk; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; +import lombok.Getter; +import lombok.Setter; +import okhttp3.HttpUrl; +import okhttp3.MediaType; +import okhttp3.OkHttpClient; +import okhttp3.Request; +import okhttp3.RequestBody; + +/** + * Buffers experimentation events and ships them to the Flagsmith events API. + * + *

Events are flushed on a fixed interval, when the buffer fills up, and on {@link #close()}. + * Exposure events are deduplicated within a flush window. Nothing thrown here ever reaches caller + * code: every failure is logged instead. + */ +@Getter +public class EventProcessor { + + /** Name of the reserved event recorded when an identity is exposed to an experiment. */ + public static final String FLAG_EXPOSURE_EVENT = "$flag_exposure"; + + private static final String EVENTS_PATH = "v1/events"; + private static final String SDK_USER_AGENT_HEADER = "Flagsmith-SDK-User-Agent"; + private static final String SDK_USER_AGENT_PREFIX = "flagsmith-java-sdk/"; + private static final String SDK_VERSION_KEY = "sdk_version"; + private static final String EXPERIMENT_ID_KEY = "experiment_id"; + private static final String KEY_SEPARATOR = "\u0000"; + private static final MediaType JSON_MEDIA_TYPE = + MediaType.get("application/json; charset=utf-8"); + + private final HttpUrl eventsEndpoint; + private final int maxBufferItems; + private final int flushIntervalMillis; + private final int requestTimeoutMillis; + private final List> buffer = new ArrayList<>(); + private final Set dedupeKeys = new HashSet<>(); + private final Object lock = new Object(); + private final ScheduledExecutorService scheduler; + private final Set> inFlight = ConcurrentHashMap.newKeySet(); + private final RequestProcessor requestProcessor; + @Setter + private FlagsmithSdk api; + private FlagsmithLogger logger = new FlagsmithLogger(); + private ScheduledFuture scheduledFlush; + + /** + * Instantiate with an HTTP client. + * + * @param client client instance + * @param eventsUri base URI of the events API, e.g. https://events.api.flagsmith.com/ + * @param maxBufferItems number of buffered events that triggers an immediate flush + * @param flushIntervalMillis interval between timed flushes; 0 disables the timer + * @param requestTimeoutMillis how long {@link #close()} waits for each in-flight batch + */ + public EventProcessor(OkHttpClient client, HttpUrl eventsUri, int maxBufferItems, + int flushIntervalMillis, int requestTimeoutMillis) { + this(eventsUri, maxBufferItems, flushIntervalMillis, requestTimeoutMillis, + new RequestProcessor(client, new FlagsmithLogger(), buildRetry())); + } + + /** + * Instantiate with a request processor, for tests. + * + * @param eventsUri base URI of the events API + * @param maxBufferItems number of buffered events that triggers an immediate flush + * @param flushIntervalMillis interval between timed flushes; 0 disables the timer + * @param requestTimeoutMillis how long {@link #close()} waits for each in-flight batch + * @param requestProcessor request processor used to POST batches + */ + public EventProcessor(HttpUrl eventsUri, int maxBufferItems, int flushIntervalMillis, + int requestTimeoutMillis, RequestProcessor requestProcessor) { + this.eventsEndpoint = eventsUri.newBuilder(EVENTS_PATH).build(); + this.maxBufferItems = maxBufferItems; + this.flushIntervalMillis = flushIntervalMillis; + this.requestTimeoutMillis = requestTimeoutMillis; + this.requestProcessor = requestProcessor; + this.scheduler = Executors.newSingleThreadScheduledExecutor((runnable) -> { + Thread thread = new Thread(runnable, "flagsmith-events"); + thread.setDaemon(true); + return thread; + }); + } + + /** + * The retry policy for an event batch: at most one retry, on a connection failure or a 5xx, + * and never on a 4xx. + */ + private static Retry buildRetry() { + Retry retry = new Retry(2); + retry.setStatusForcelist(new HashSet<>(Arrays.asList(500, 502, 503, 504))); + retry.setStatusForcelistOnly(Boolean.TRUE); + return retry; + } + + /** + * Set the logger used by the processor and by its request processor. + * + * @param logger logger instance + */ + public void setLogger(FlagsmithLogger logger) { + this.logger = logger; + requestProcessor.setLogger(logger); + } + + /** + * Buffer a custom event. + * + * @param event event name + * @param identifier identity the event belongs to, may be null + * @param value event value, stringified before sending + * @param traits identity traits to attach, may be null + * @param metadata caller metadata to attach, may be null + */ + public void trackEvent(String event, String identifier, Object value, + Map traits, Map metadata) { + bufferEvent(event, null, identifier, value, traits, metadata, false); + } + + /** + * Buffer a flag exposure event. Exposures equal in feature, identifier, value and experiment + * are only sent once per flush window. + * + * @param featureName feature the identity was exposed to + * @param identifier identity the exposure belongs to + * @param value variant the identity was bucketed into + * @param traits identity traits to attach, may be null + * @param metadata caller metadata to attach, may be null + */ + public void trackExposureEvent(String featureName, String identifier, Object value, + Map traits, Map metadata) { + bufferEvent(FLAG_EXPOSURE_EVENT, featureName, identifier, value, traits, metadata, true); + } + + /** + * Send everything buffered so far. + * + * @return a future completing once every in-flight batch has been delivered or dropped + */ + public CompletableFuture flush() { + try { + List> batch = null; + + synchronized (lock) { + if (!buffer.isEmpty()) { + batch = new ArrayList<>(buffer); + buffer.clear(); + } + dedupeKeys.clear(); + } + + if (batch != null) { + send(batch); + } + } catch (RuntimeException e) { + logger.error("Failed to flush events.", e); + } + + return awaitInFlight(); + } + + /** + * Start the flush timer. Does nothing when the flush interval is not positive. + */ + public void start() { + if (flushIntervalMillis <= 0 || scheduledFlush != null) { + return; + } + + scheduledFlush = scheduler.scheduleWithFixedDelay( + this::flush, flushIntervalMillis, flushIntervalMillis, TimeUnit.MILLISECONDS); + } + + /** + * Stop the flush timer, ship whatever is left on a best-effort basis and release the HTTP + * resources. + */ + public void close() { + scheduler.shutdownNow(); + + try { + flush().get(requestTimeoutMillis * 2L, TimeUnit.MILLISECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + logger.error("Interrupted while flushing events on close.", e); + } catch (Exception e) { + logger.error("Failed to flush events on close.", e); + } + + requestProcessor.close(); + } + + private void bufferEvent(String event, String featureName, String identifier, Object value, + Map traits, Map metadata, boolean dedupe) { + try { + final String stringValue = value == null ? null : String.valueOf(value); + + Map eventMetadata = new HashMap<>(); + if (metadata != null) { + eventMetadata.putAll(metadata); + } + eventMetadata.put(SDK_VERSION_KEY, Versions.getVersion()); + + Map eventPayload = new LinkedHashMap<>(); + eventPayload.put("event", event); + eventPayload.put("feature_name", featureName); + eventPayload.put("identifier", identifier); + eventPayload.put("value", stringValue); + eventPayload.put("traits", traits); + eventPayload.put("metadata", eventMetadata); + eventPayload.put("timestamp", System.currentTimeMillis()); + + boolean isFull; + + synchronized (lock) { + if (dedupe && !dedupeKeys.add(dedupeKey( + event, featureName, identifier, stringValue, eventMetadata.get(EXPERIMENT_ID_KEY)))) { + return; + } + buffer.add(eventPayload); + isFull = maxBufferItems > 0 && buffer.size() >= maxBufferItems; + } + + if (isFull) { + flush(); + } + } catch (RuntimeException e) { + logger.error("Failed to buffer event " + event + ".", e); + } + } + + private static String dedupeKey(String event, String featureName, String identifier, + String value, Object experimentId) { + return String.join(KEY_SEPARATOR, + nullToEmpty(event), + nullToEmpty(featureName), + nullToEmpty(identifier), + nullToEmpty(value), + experimentId == null ? "" : String.valueOf(experimentId)); + } + + private static String nullToEmpty(String value) { + return value == null ? "" : value; + } + + private void send(List> batch) { + if (api == null) { + logger.error("Dropping {} events: the event processor has no API wrapper.", batch.size()); + return; + } + + String payload; + + try { + payload = MapperFactory.getMapper() + .writeValueAsString(Collections.singletonMap("events", batch)); + } catch (Exception e) { + logger.error("Error parsing event data to JSON.", e); + return; + } + + Request request = api + .newPostRequest(eventsEndpoint, RequestBody.create(payload, JSON_MEDIA_TYPE)) + .newBuilder() + .header(SDK_USER_AGENT_HEADER, SDK_USER_AGENT_PREFIX + Versions.getVersion()) + .build(); + + CompletableFuture tracked = new CompletableFuture<>(); + inFlight.add(tracked); + + requestProcessor + .submit(request, new TypeReference() {}, Boolean.FALSE, buildRetry()) + .whenComplete((response, error) -> { + inFlight.remove(tracked); + tracked.complete(null); + }); + } + + private CompletableFuture awaitInFlight() { + return CompletableFuture.allOf(inFlight.toArray(new CompletableFuture[0])); + } +} diff --git a/src/main/java/com/flagsmith/threads/RequestProcessor.java b/src/main/java/com/flagsmith/threads/RequestProcessor.java index 25f4867f..035c2a06 100644 --- a/src/main/java/com/flagsmith/threads/RequestProcessor.java +++ b/src/main/java/com/flagsmith/threads/RequestProcessor.java @@ -14,6 +14,7 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import lombok.Getter; +import lombok.Setter; import okhttp3.Call; import okhttp3.OkHttpClient; import okhttp3.Request; @@ -25,6 +26,7 @@ public class RequestProcessor { @Getter private OkHttpClient client; @Getter + @Setter private FlagsmithLogger logger; private Retry retries = new Retry(3); @@ -78,6 +80,23 @@ public Future executeAsync(Request request, Boolean doThrow) { */ public Future executeAsync( Request request, TypeReference clazz, Boolean doThrow, Retry retries) { + return submit(request, clazz, doThrow, retries); + } + + /** + * Execute the request in async mode, returning a CompletableFuture so callers can compose on + * its completion. + * + * @param request Request object + * @param clazz class type of response + * @param doThrow should throw Exception + * @param retries no of retries before failing + * @param Type inference for the response + * @return a future completed with the unmarshalled response, or null when the call failed and + * doThrow is false + */ + public CompletableFuture submit( + Request request, TypeReference clazz, Boolean doThrow, Retry retries) { CompletableFuture completableFuture = new CompletableFuture<>(); Retry localRetry = retries.toBuilder().build(); // run the execute method in a fixed thread with retries. diff --git a/src/test/java/com/flagsmith/FlagsmithClientTest.java b/src/test/java/com/flagsmith/FlagsmithClientTest.java index 5f8acc74..c002df39 100644 --- a/src/test/java/com/flagsmith/FlagsmithClientTest.java +++ b/src/test/java/com/flagsmith/FlagsmithClientTest.java @@ -16,6 +16,7 @@ import com.fasterxml.jackson.databind.JsonNode; import com.flagsmith.config.FlagsmithCacheConfig; import com.flagsmith.config.FlagsmithConfig; +import com.flagsmith.exceptions.FeatureNotFoundError; import com.flagsmith.exceptions.FlagsmithApiError; import com.flagsmith.exceptions.FlagsmithClientError; import com.flagsmith.exceptions.FlagsmithRuntimeError; @@ -26,12 +27,14 @@ import com.flagsmith.models.DefaultFlag; import com.flagsmith.models.environments.EnvironmentModel; import com.flagsmith.models.features.FeatureStateModel; +import com.flagsmith.models.Flag; import com.flagsmith.models.Flags; import com.flagsmith.models.SdkTraitModel; import com.flagsmith.models.Segment; import com.flagsmith.models.TraitConfig; import com.flagsmith.models.TraitModel; import com.flagsmith.responses.FlagsAndTraitsResponse; +import com.flagsmith.threads.EventProcessor; import com.flagsmith.threads.PollingManager; import com.flagsmith.threads.RequestProcessor; @@ -998,4 +1001,279 @@ public void testFlagsmithUsesOfflineHandlerIfSetAndNoAPIResponse() throws Flagsm assertTrue(environmentFlags.isFeatureEnabled("some_feature")); assertTrue(identityFlags.isFeatureEnabled("some_feature")); } + + private static FlagsmithConfig.Builder eventsConfigBuilder( + String baseUrl, MockInterceptor interceptor) { + return FlagsmithConfig.newBuilder() + .baseUri(baseUrl) + .addHttpInterceptor(interceptor) + .eventsUri("http://events-uri") + .withEnableEvents(Boolean.TRUE); + } + + private static void respondWithExperimentFlags(String baseUrl, MockInterceptor interceptor) { + interceptor.addRule() + .post(baseUrl + "/identities/") + .anyTimes() + .respond(FlagsmithTestHelper.getIdentitiesFlagsWithExperiment(), MEDIATYPE_JSON); + } + + @Test + public void testEventsConfigWithoutEnableEventsThrows() { + assertThrows(IllegalArgumentException.class, + () -> FlagsmithConfig.newBuilder().withEventsMaxBufferItems(10).build()); + assertThrows(IllegalArgumentException.class, + () -> FlagsmithConfig.newBuilder().withEventsFlushIntervalMillis(10).build()); + } + + @Test + public void testEventsInOfflineModeThrowsAtBuild() { + EventProcessor processor = mock(EventProcessor.class); + FlagsmithConfig config = FlagsmithConfig.newBuilder() + .withOfflineMode(Boolean.TRUE) + .withOfflineHandler(new DummyOfflineHandler()) + .withEventProcessor(processor) + .build(); + + FlagsmithClient.Builder clientBuilder = FlagsmithClient.newBuilder() + .withConfiguration(config) + .setApiKey("api-key"); + + FlagsmithRuntimeError ex = assertThrows(FlagsmithRuntimeError.class, clientBuilder::build); + assertEquals("Events cannot be enabled in offline mode.", ex.getMessage()); + } + + @Test + public void testEventApisThrowWhenEventsAreDisabled() { + FlagsmithClient client = FlagsmithClient.newBuilder().setApiKey("api-key").build(); + + assertThrows(FlagsmithRuntimeError.class, + () -> client.getExperimentFlag("checkout_cta", "user-1")); + assertThrows(FlagsmithRuntimeError.class, () -> client.trackEvent("purchase")); + assertThrows(FlagsmithRuntimeError.class, + () -> client.trackExposureEvent("checkout_cta", "user-1", "treatment")); + assertTrue(client.flushEvents().isDone()); + } + + @Test + public void testTrackEventRejectsReservedEventNames() { + EventProcessor processor = mock(EventProcessor.class); + FlagsmithClient client = FlagsmithClient.newBuilder() + .withConfiguration(FlagsmithConfig.newBuilder() + .withEventProcessor(processor) + .build()) + .setApiKey("api-key") + .build(); + + assertThrows(IllegalArgumentException.class, () -> client.trackEvent("$flag_exposure")); + verify(processor, never()).trackEvent(any(), any(), any(), any(), any()); + } + + @Test + public void testTrackExposureEventWithBlankIdentifierSendsNothing() { + EventProcessor processor = mock(EventProcessor.class); + FlagsmithClient client = FlagsmithClient.newBuilder() + .withConfiguration(FlagsmithConfig.newBuilder() + .withEventProcessor(processor) + .build()) + .setApiKey("api-key") + .build(); + + client.trackExposureEvent("checkout_cta", " ", "treatment"); + client.trackExposureEvent("checkout_cta", null, "treatment"); + + verify(processor, never()).trackExposureEvent(any(), any(), any(), any(), any()); + } + + @Test + public void testGetExperimentFlagRecordsAnExposureWhenEnrolled() throws FlagsmithClientError { + String baseUrl = "http://bad-url"; + MockInterceptor interceptor = new MockInterceptor(); + EventProcessor processor = mock(EventProcessor.class); + FlagsmithClient client = FlagsmithClient.newBuilder() + .withConfiguration(eventsConfigBuilder(baseUrl, interceptor) + .withEventProcessor(processor) + .build()) + .setApiKey("api-key") + .build(); + respondWithExperimentFlags(baseUrl, interceptor); + + BaseFlag flag = client.getExperimentFlag("checkout_cta", "user-1"); + + assertTrue(flag instanceof Flag); + assertEquals("treatment", ((Flag) flag).getVariant()); + assertEquals("SPLIT; weight=70.0", ((Flag) flag).getReason()); + assertEquals(42, ((Flag) flag).getExperiment().getId()); + assertEquals(Boolean.TRUE, ((Flag) flag).getExperiment().getInExperiment()); + + Map expectedMetadata = new HashMap<>(); + expectedMetadata.put("experiment_id", 42); + verify(processor, times(1)).trackExposureEvent( + eq("checkout_cta"), eq("user-1"), eq("treatment"), any(), eq(expectedMetadata)); + } + + @Test + public void testGetExperimentFlagRecordsNoExposureWhenNotEnrolledOrDisabled() + throws FlagsmithClientError { + String baseUrl = "http://bad-url"; + MockInterceptor interceptor = new MockInterceptor(); + EventProcessor processor = mock(EventProcessor.class); + FlagsmithClient client = FlagsmithClient.newBuilder() + .withConfiguration(eventsConfigBuilder(baseUrl, interceptor) + .withEventProcessor(processor) + .build()) + .setApiKey("api-key") + .build(); + respondWithExperimentFlags(baseUrl, interceptor); + + // bucketed but outside the rollout + assertEquals("control", + ((Flag) client.getExperimentFlag("pricing_page", "user-1")).getVariant()); + // no metadata at all + assertNull(((Flag) client.getExperimentFlag("some_feature", "user-1")).getExperiment()); + // enrolled, but the flag is off + assertEquals(Boolean.FALSE, + client.getExperimentFlag("disabled_feature", "user-1").getEnabled()); + + verify(processor, never()).trackExposureEvent(any(), any(), any(), any(), any()); + } + + @Test + public void testGetExperimentFlagUsesTheDefaultHandlerForAMissingFeature() + throws FlagsmithClientError { + String baseUrl = "http://bad-url"; + MockInterceptor interceptor = new MockInterceptor(); + EventProcessor processor = mock(EventProcessor.class); + FlagsmithClient client = FlagsmithClient.newBuilder() + .withConfiguration(eventsConfigBuilder(baseUrl, interceptor) + .withEventProcessor(processor) + .build()) + .setDefaultFlagValueFunction(FlagsmithClientTest::defaultHandler) + .setApiKey("api-key") + .build(); + respondWithExperimentFlags(baseUrl, interceptor); + + BaseFlag flag = client.getExperimentFlag("no_such_feature", "user-1"); + + assertTrue(flag instanceof DefaultFlag); + assertEquals(DEFAULT_FLAG_VALUE, flag.getValue()); + verify(processor, never()).trackExposureEvent(any(), any(), any(), any(), any()); + } + + @Test + public void testGetExperimentFlagThrowsForAMissingFeatureWithoutADefaultHandler() { + String baseUrl = "http://bad-url"; + MockInterceptor interceptor = new MockInterceptor(); + EventProcessor processor = mock(EventProcessor.class); + FlagsmithClient client = FlagsmithClient.newBuilder() + .withConfiguration(eventsConfigBuilder(baseUrl, interceptor) + .withEventProcessor(processor) + .build()) + .setApiKey("api-key") + .build(); + respondWithExperimentFlags(baseUrl, interceptor); + + assertThrows(FeatureNotFoundError.class, + () -> client.getExperimentFlag("no_such_feature", "user-1")); + } + + @Test + public void testGetExperimentFlagRecordsNoExposureWithLocalEvaluation() + throws JsonProcessingException, FlagsmithClientError { + String baseUrl = "http://bad-url"; + MockInterceptor interceptor = new MockInterceptor(); + EventProcessor processor = mock(EventProcessor.class); + interceptor.addRule() + .get(baseUrl + "/environment-document/") + .anyTimes() + .respond( + MapperFactory.getMapper() + .writeValueAsString(FlagsmithTestHelper.environmentModel()), + MEDIATYPE_JSON); + + FlagsmithClient client = FlagsmithClient.newBuilder() + .withConfiguration(eventsConfigBuilder(baseUrl, interceptor) + .withEventProcessor(processor) + .withLocalEvaluation(true) + .build()) + .setApiKey("ser.abcdefg") + .build(); + client.updateEnvironment(); + + BaseFlag flag = client.getExperimentFlag("some_feature", "user-1"); + + assertTrue(flag instanceof Flag); + assertNull(((Flag) flag).getVariant()); + assertNull(((Flag) flag).getExperiment()); + verify(processor, never()).trackExposureEvent(any(), any(), any(), any(), any()); + } + + @Test + public void testGetExperimentFlagRecordsOneExposurePerIdentity() throws FlagsmithClientError { + String baseUrl = "http://bad-url"; + MockInterceptor interceptor = new MockInterceptor(); + EventProcessor processor = mock(EventProcessor.class); + FlagsmithClient client = FlagsmithClient.newBuilder() + .withConfiguration(eventsConfigBuilder(baseUrl, interceptor) + .withEventProcessor(processor) + .build()) + .setApiKey("api-key") + .build(); + respondWithExperimentFlags(baseUrl, interceptor); + + client.getExperimentFlag("checkout_cta", "user-1"); + client.getExperimentFlag("checkout_cta", "user-2"); + + verify(processor, times(1)).trackExposureEvent( + eq("checkout_cta"), eq("user-1"), eq("treatment"), any(), any()); + verify(processor, times(1)).trackExposureEvent( + eq("checkout_cta"), eq("user-2"), eq("treatment"), any(), any()); + } + + @Test + public void testCloseFlushesBufferedEvents() throws FlagsmithClientError, IOException { + String baseUrl = "http://bad-url"; + MockInterceptor interceptor = new MockInterceptor(); + List eventBodies = new ArrayList<>(); + // Added before the MockInterceptor, which short-circuits the chain once it matches. + FlagsmithClient client = FlagsmithClient.newBuilder() + .withConfiguration(FlagsmithConfig.newBuilder() + .baseUri(baseUrl) + .addHttpInterceptor((chain) -> { + Request request = chain.request(); + if (request.url().toString().endsWith("/v1/events")) { + Buffer buffer = new Buffer(); + request.body().writeTo(buffer); + eventBodies.add(buffer.readUtf8()); + } + return chain.proceed(request); + }) + .addHttpInterceptor(interceptor) + .eventsUri("http://events-uri") + .withEnableEvents(Boolean.TRUE) + .withEventsFlushIntervalMillis(0) + .build()) + .setApiKey("api-key") + .build(); + respondWithExperimentFlags(baseUrl, interceptor); + interceptor.addRule() + .post("http://events-uri/v1/events") + .headerMatches("Flagsmith-SDK-User-Agent", Pattern.compile("flagsmith-java-sdk/.*")) + .anyTimes() + .respond("{\"accepted\": 1, \"rejected\": []}", MEDIATYPE_JSON); + + client.getExperimentFlag("checkout_cta", "user-1"); + assertTrue(eventBodies.isEmpty()); + + client.close(); + + assertEquals(1, eventBodies.size()); + JsonNode events = MapperFactory.getMapper().readTree(eventBodies.get(0)).get("events"); + assertEquals(1, events.size()); + assertEquals("$flag_exposure", events.get(0).get("event").asText()); + assertEquals("checkout_cta", events.get(0).get("feature_name").asText()); + assertEquals("user-1", events.get(0).get("identifier").asText()); + assertEquals("treatment", events.get(0).get("value").asText()); + assertEquals(42, events.get(0).get("metadata").get("experiment_id").asInt()); + } } diff --git a/src/test/java/com/flagsmith/FlagsmithTestHelper.java b/src/test/java/com/flagsmith/FlagsmithTestHelper.java index d1922d56..03aeb887 100644 --- a/src/test/java/com/flagsmith/FlagsmithTestHelper.java +++ b/src/test/java/com/flagsmith/FlagsmithTestHelper.java @@ -431,6 +431,90 @@ public static String getIdentitiesFlags() { return featureJson; } + /** + * An identity flags payload exercising every branch of the experiment gate: an enrolled flag, + * a bucketed but unenrolled flag, a flag with no metadata at all, and a disabled flag whose + * experiment is running. + */ + public static String getIdentitiesFlagsWithExperiment() { + return "{\n" + + " \"traits\": [],\n" + + " \"flags\": [\n" + + " {\n" + + " \"id\": 1,\n" + + " \"feature\": {\n" + + " \"id\": 1,\n" + + " \"name\": \"checkout_cta\",\n" + + " \"type\": \"MULTIVARIATE\",\n" + + " \"project\": 1\n" + + " },\n" + + " \"feature_state_value\": \"buy-now\",\n" + + " \"enabled\": true,\n" + + " \"variant\": \"treatment\",\n" + + " \"reason\": \"SPLIT; weight=70.0\",\n" + + " \"metadata\": {\n" + + " \"experiment\": {\n" + + " \"id\": 42,\n" + + " \"name\": \"checkout_experiment\",\n" + + " \"in_experiment\": true\n" + + " }\n" + + " }\n" + + " },\n" + + " {\n" + + " \"id\": 2,\n" + + " \"feature\": {\n" + + " \"id\": 2,\n" + + " \"name\": \"pricing_page\",\n" + + " \"type\": \"MULTIVARIATE\",\n" + + " \"project\": 1\n" + + " },\n" + + " \"feature_state_value\": \"old-pricing\",\n" + + " \"enabled\": true,\n" + + " \"variant\": \"control\",\n" + + " \"reason\": \"SPLIT; weight=30.0\",\n" + + " \"metadata\": {\n" + + " \"experiment\": {\n" + + " \"id\": 43,\n" + + " \"name\": \"pricing_experiment\",\n" + + " \"in_experiment\": false\n" + + " }\n" + + " }\n" + + " },\n" + + " {\n" + + " \"id\": 3,\n" + + " \"feature\": {\n" + + " \"id\": 3,\n" + + " \"name\": \"some_feature\",\n" + + " \"type\": \"STANDARD\",\n" + + " \"project\": 1\n" + + " },\n" + + " \"feature_state_value\": \"some-value\",\n" + + " \"enabled\": true\n" + + " },\n" + + " {\n" + + " \"id\": 4,\n" + + " \"feature\": {\n" + + " \"id\": 4,\n" + + " \"name\": \"disabled_feature\",\n" + + " \"type\": \"MULTIVARIATE\",\n" + + " \"project\": 1\n" + + " },\n" + + " \"feature_state_value\": \"off\",\n" + + " \"enabled\": false,\n" + + " \"variant\": \"treatment\",\n" + + " \"reason\": \"SPLIT; weight=50.0\",\n" + + " \"metadata\": {\n" + + " \"experiment\": {\n" + + " \"id\": 44,\n" + + " \"name\": \"disabled_experiment\",\n" + + " \"in_experiment\": true\n" + + " }\n" + + " }\n" + + " }\n" + + " ]\n" + + "}"; + } + public static Future futurableReturn(T response) { CompletableFuture promise = new CompletableFuture<>(); promise.complete(response); diff --git a/src/test/java/com/flagsmith/flagengine/models/FlagTest.java b/src/test/java/com/flagsmith/flagengine/models/FlagTest.java index c5aebcee..757a6157 100644 --- a/src/test/java/com/flagsmith/flagengine/models/FlagTest.java +++ b/src/test/java/com/flagsmith/flagengine/models/FlagTest.java @@ -1,9 +1,15 @@ package com.flagsmith.flagengine.models; +import com.flagsmith.MapperFactory; +import com.flagsmith.models.ExperimentMetadata; import com.flagsmith.models.Flag; -import org.junit.Test; +import com.flagsmith.models.features.FeatureStateModel; +import com.fasterxml.jackson.core.JsonProcessingException; +import org.junit.jupiter.api.Test; -import static org.junit.Assert.assertEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; public class FlagTest { @Test @@ -15,13 +21,56 @@ public void testToString() { flag.setValue("foo"); flag.setFeatureId(1); - String expected = String.join("Flag(super=", - "BaseFlag(enabled=true, ", - "value=foo, ", - "featureName=my_feature), ", - "featureId=1, ", - "isDefault=false)"); + // BaseModel has no toString(), so the "super=" part carries an identity hash. Only the + // fields Lombok renders are asserted. + String expected = ", enabled=true, " + + "value=foo, " + + "featureName=my_feature), " + + "featureId=1, " + + "isDefault=false, " + + "variant=null, " + + "reason=null, " + + "experiment=null)"; - assertEquals(expected, flag.toString()); + assertTrue(flag.toString().startsWith("Flag(super=BaseFlag(super="), flag.toString()); + assertTrue(flag.toString().endsWith(expected), flag.toString()); + } + + @Test + public void fromFeatureStateModel_copiesExperimentMetadata() throws JsonProcessingException { + FeatureStateModel featureState = MapperFactory.getMapper().readValue( + "{\"feature\": {\"id\": 1, \"name\": \"checkout_cta\"}," + + " \"enabled\": true," + + " \"feature_state_value\": \"buy-now\"," + + " \"variant\": \"treatment\"," + + " \"reason\": \"SPLIT; weight=70.0\"," + + " \"metadata\": {\"experiment\": {\"id\": 42, \"name\": \"exp\"," + + " \"in_experiment\": true}}}", + FeatureStateModel.class); + + Flag flag = Flag.fromFeatureStateModel(featureState); + + assertEquals("treatment", flag.getVariant()); + assertEquals("SPLIT; weight=70.0", flag.getReason()); + + ExperimentMetadata experiment = flag.getExperiment(); + assertEquals(42, experiment.getId()); + assertEquals("exp", experiment.getName()); + assertEquals(Boolean.TRUE, experiment.getInExperiment()); + } + + @Test + public void fromFeatureStateModel_leavesExperimentMetadataNullWhenAbsent() + throws JsonProcessingException { + FeatureStateModel featureState = MapperFactory.getMapper().readValue( + "{\"feature\": {\"id\": 1, \"name\": \"some_feature\"}," + + " \"enabled\": true, \"feature_state_value\": \"some-value\"}", + FeatureStateModel.class); + + Flag flag = Flag.fromFeatureStateModel(featureState); + + assertNull(flag.getVariant()); + assertNull(flag.getReason()); + assertNull(flag.getExperiment()); } } diff --git a/src/test/java/com/flagsmith/models/FeatureStateModelTest.java b/src/test/java/com/flagsmith/models/FeatureStateModelTest.java new file mode 100644 index 00000000..1e4e120f --- /dev/null +++ b/src/test/java/com/flagsmith/models/FeatureStateModelTest.java @@ -0,0 +1,71 @@ +package com.flagsmith.models; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.flagsmith.MapperFactory; +import com.flagsmith.models.features.FeatureStateModel; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; + +public class FeatureStateModelTest { + + private static FeatureStateModel parse(String json) throws JsonProcessingException { + return MapperFactory.getMapper().readValue(json, FeatureStateModel.class); + } + + @Test + public void parsesVariantReasonAndExperiment() throws JsonProcessingException { + FeatureStateModel featureState = parse( + "{\"feature\": {\"id\": 220175, \"name\": \"checkout_cta\", \"type\": \"MULTIVARIATE\"}," + + " \"enabled\": true," + + " \"feature_state_value\": \"buy-now\"," + + " \"variant\": \"treatment\"," + + " \"reason\": \"SPLIT; weight=70.0\"," + + " \"metadata\": {\"experiment\": {\"id\": 167, \"name\": \"flutter_demo_exp\"," + + " \"in_experiment\": true}}}"); + + assertEquals("treatment", featureState.getVariant()); + assertEquals("SPLIT; weight=70.0", featureState.getReason()); + + ExperimentMetadata experiment = featureState.getMetadata().getExperiment(); + assertEquals(167, experiment.getId()); + assertEquals("flutter_demo_exp", experiment.getName()); + assertEquals(Boolean.TRUE, experiment.getInExperiment()); + } + + @Test + public void ignoresUnknownMetadataKeys() throws JsonProcessingException { + FeatureStateModel featureState = parse( + "{\"feature\": {\"id\": 1, \"name\": \"checkout_cta\"}," + + " \"enabled\": true," + + " \"metadata\": {\"something_else\": {\"a\": 1}, \"experiment\": {\"id\": 3," + + " \"in_experiment\": true}}}"); + + assertNotNull(featureState.getMetadata()); + assertEquals(3, featureState.getMetadata().getExperiment().getId()); + } + + @Test + public void inExperimentDefaultsToFalseWhenMissing() throws JsonProcessingException { + FeatureStateModel featureState = parse( + "{\"feature\": {\"id\": 1, \"name\": \"checkout_cta\"}," + + " \"enabled\": true," + + " \"metadata\": {\"experiment\": {\"id\": 3, \"name\": \"exp\"}}}"); + + assertEquals(Boolean.FALSE, featureState.getMetadata().getExperiment().getInExperiment()); + } + + @Test + public void parsesPayloadWithoutAnyExperimentFields() throws JsonProcessingException { + FeatureStateModel featureState = parse( + "{\"feature\": {\"id\": 1, \"name\": \"some_feature\"}," + + " \"enabled\": true, \"feature_state_value\": \"some-value\"}"); + + assertNull(featureState.getVariant()); + assertNull(featureState.getReason()); + assertNull(featureState.getMetadata()); + assertEquals("some-value", featureState.getValue()); + } +} diff --git a/src/test/java/com/flagsmith/threads/EventProcessorTest.java b/src/test/java/com/flagsmith/threads/EventProcessorTest.java new file mode 100644 index 00000000..6af0e55a --- /dev/null +++ b/src/test/java/com/flagsmith/threads/EventProcessorTest.java @@ -0,0 +1,378 @@ +package com.flagsmith.threads; + +import static okhttp3.mock.MediaTypes.MEDIATYPE_JSON; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.fasterxml.jackson.databind.JsonNode; +import com.flagsmith.FlagsmithLogger; +import com.flagsmith.MapperFactory; +import com.flagsmith.interfaces.FlagsmithSdk; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.concurrent.TimeUnit; +import java.util.regex.Pattern; +import lombok.SneakyThrows; +import okhttp3.HttpUrl; +import okhttp3.Interceptor; +import okhttp3.OkHttpClient; +import okhttp3.Request; +import okhttp3.RequestBody; +import okhttp3.Response; +import okhttp3.mock.MockInterceptor; +import okio.Buffer; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class EventProcessorTest { + + private static final String EVENTS_URI = "http://events-uri/"; + private static final String EVENTS_ENDPOINT = EVENTS_URI + "v1/events"; + private static final String ACCEPTED_BODY = "{\"accepted\": 1, \"rejected\": []}"; + /** Every wait in this test is bounded so a broken retry loop fails instead of hanging. */ + private static final long WAIT_SECONDS = 10L; + + private MockInterceptor interceptor; + private RecordingInterceptor recorder; + private FlagsmithSdk api; + private EventProcessor eventProcessor; + + @BeforeEach + public void init() { + interceptor = new MockInterceptor(); + recorder = new RecordingInterceptor(); + api = mock(FlagsmithSdk.class); + when(api.newPostRequest(any(), any())).thenAnswer((invocation) -> new Request.Builder() + .url((HttpUrl) invocation.getArgument(0)) + .header("X-Environment-Key", "api-key") + .header("User-Agent", "flagsmith-java-sdk/test") + .addHeader("Accept", "application/json") + .post((RequestBody) invocation.getArgument(1)) + .build()); + } + + @AfterEach + public void tearDown() { + if (eventProcessor != null) { + // Appended last, so it only catches whatever close() flushes out of the buffer. + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(ACCEPTED_BODY, MEDIATYPE_JSON); + eventProcessor.close(); + eventProcessor = null; + } + } + + private EventProcessor newProcessor(int maxBufferItems, int flushIntervalMillis) { + OkHttpClient client = new OkHttpClient.Builder() + .addInterceptor(recorder) + .addInterceptor(interceptor) + .build(); + eventProcessor = new EventProcessor( + HttpUrl.get(EVENTS_URI), + maxBufferItems, + flushIntervalMillis, + 3000, + new RequestProcessor(client, new FlagsmithLogger())); + eventProcessor.setApi(api); + return eventProcessor; + } + + @SneakyThrows + private void flushAndWait(EventProcessor processor) { + processor.flush().get(WAIT_SECONDS, TimeUnit.SECONDS); + } + + @Test + public void trackEvent_buffersStringifiedValueSdkVersionAndTimestamp() { + EventProcessor processor = newProcessor(1000, 0); + + Map traits = new HashMap<>(); + traits.put("plan", "premium"); + Map metadata = new HashMap<>(); + metadata.put("source", "checkout"); + + long before = System.currentTimeMillis(); + processor.trackEvent("purchase", "user-123", 49.0, traits, metadata); + + assertEquals(1, processor.getBuffer().size()); + + Map event = processor.getBuffer().get(0); + assertEquals( + Arrays.asList("event", "feature_name", "identifier", "value", "traits", "metadata", + "timestamp"), + new ArrayList<>(event.keySet())); + assertEquals("purchase", event.get("event")); + assertNull(event.get("feature_name")); + assertEquals("user-123", event.get("identifier")); + assertEquals("49.0", event.get("value")); + assertEquals(traits, event.get("traits")); + + @SuppressWarnings("unchecked") + Map eventMetadata = (Map) event.get("metadata"); + assertEquals("checkout", eventMetadata.get("source")); + assertNotNull(eventMetadata.get("sdk_version")); + + long timestamp = (Long) event.get("timestamp"); + assertTrue(timestamp >= before); + } + + @Test + public void trackEvent_buffersNullValueAsNull() { + EventProcessor processor = newProcessor(1000, 0); + + processor.trackEvent("purchase", "user-123", null, null, null); + + Map event = processor.getBuffer().get(0); + assertNull(event.get("value")); + assertNull(event.get("traits")); + } + + @Test + public void trackExposureEvent_dedupesIdenticalExposuresWithinTheFlushWindow() { + EventProcessor processor = newProcessor(1000, 0); + Map metadata = Collections.singletonMap("experiment_id", 42); + + processor.trackExposureEvent("checkout_cta", "user-1", "treatment", null, metadata); + processor.trackExposureEvent("checkout_cta", "user-1", "treatment", null, metadata); + + assertEquals(1, processor.getBuffer().size()); + } + + @Test + public void trackExposureEvent_doesNotDedupeWhenAnyKeyPartDiffers() { + EventProcessor processor = newProcessor(1000, 0); + Map metadata = Collections.singletonMap("experiment_id", 42); + + processor.trackExposureEvent("checkout_cta", "user-1", "treatment", null, metadata); + processor.trackExposureEvent("checkout_cta", "user-2", "treatment", null, metadata); + processor.trackExposureEvent("checkout_cta", "user-1", "control", null, metadata); + processor.trackExposureEvent("pricing_page", "user-1", "treatment", null, metadata); + processor.trackExposureEvent("checkout_cta", "user-1", "treatment", null, + Collections.singletonMap("experiment_id", 43)); + + assertEquals(5, processor.getBuffer().size()); + } + + @Test + public void trackExposureEvent_buffersAgainAfterAFlush() { + EventProcessor processor = newProcessor(1000, 0); + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(ACCEPTED_BODY, MEDIATYPE_JSON); + Map metadata = Collections.singletonMap("experiment_id", 42); + + processor.trackExposureEvent("checkout_cta", "user-1", "treatment", null, metadata); + processor.trackExposureEvent("checkout_cta", "user-1", "treatment", null, metadata); + flushAndWait(processor); + + processor.trackExposureEvent("checkout_cta", "user-1", "treatment", null, metadata); + + assertEquals(1, processor.getBuffer().size()); + assertEquals(1, recorder.count()); + } + + @Test + public void trackEvent_neverDedupesCustomEvents() { + EventProcessor processor = newProcessor(1000, 0); + + processor.trackEvent("purchase", "user-1", "49.00", null, null); + processor.trackEvent("purchase", "user-1", "49.00", null, null); + + assertEquals(2, processor.getBuffer().size()); + } + + @Test + @SneakyThrows + public void flush_postsTheBatchToTheEventsEndpoint() { + EventProcessor processor = newProcessor(1000, 0); + interceptor.addRule() + .post(EVENTS_ENDPOINT) + .headerMatches("X-Environment-Key", Pattern.compile("api-key")) + .headerMatches("Flagsmith-SDK-User-Agent", Pattern.compile("flagsmith-java-sdk/.*")) + .headerMatches("Content-Type", Pattern.compile("application/json; charset=utf-8")) + .anyTimes() + .respond(ACCEPTED_BODY, MEDIATYPE_JSON); + + processor.trackExposureEvent("checkout_cta", "user-1", "treatment", null, + Collections.singletonMap("experiment_id", 42)); + flushAndWait(processor); + + assertEquals(1, recorder.count()); + + JsonNode body = MapperFactory.getMapper().readTree(recorder.bodies().get(0)); + assertEquals(1, body.get("events").size()); + + JsonNode event = body.get("events").get(0); + assertEquals("$flag_exposure", event.get("event").asText()); + assertEquals("checkout_cta", event.get("feature_name").asText()); + assertEquals("user-1", event.get("identifier").asText()); + assertEquals("treatment", event.get("value").asText()); + assertEquals(42, event.get("metadata").get("experiment_id").asInt()); + assertTrue(event.get("metadata").has("sdk_version")); + assertTrue(event.get("timestamp").isNumber()); + assertTrue(processor.getBuffer().isEmpty()); + } + + @Test + @SneakyThrows + public void flush_doesNotPostWhenTheBufferIsEmpty() { + EventProcessor processor = newProcessor(1000, 0); + + flushAndWait(processor); + + assertEquals(0, recorder.count()); + } + + @Test + @SneakyThrows + public void trackEvent_flushesWhenTheBufferIsFull() { + EventProcessor processor = newProcessor(2, 0); + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(ACCEPTED_BODY, MEDIATYPE_JSON); + + processor.trackEvent("purchase", "user-1", "1", null, null); + assertEquals(1, processor.getBuffer().size()); + + processor.trackEvent("purchase", "user-2", "2", null, null); + assertTrue(processor.getBuffer().isEmpty()); + + flushAndWait(processor); + + assertEquals(1, recorder.count()); + JsonNode body = MapperFactory.getMapper().readTree(recorder.bodies().get(0)); + assertEquals(2, body.get("events").size()); + } + + @Test + @SneakyThrows + public void flush_completesOnlyAfterTheInFlightPostCompletes() { + EventProcessor processor = newProcessor(1000, 0); + interceptor.addRule() + .post(EVENTS_ENDPOINT) + .anyTimes() + .delay(600) + .respond(ACCEPTED_BODY, MEDIATYPE_JSON); + + processor.trackEvent("purchase", "user-1", "1", null, null); + + long start = System.currentTimeMillis(); + flushAndWait(processor); + long elapsed = System.currentTimeMillis() - start; + + assertEquals(1, recorder.count()); + assertTrue(elapsed >= 500, "flush() returned after " + elapsed + "ms, before the POST"); + } + + @Test + @SneakyThrows + public void flush_retriesOnceOnServerErrorThenDelivers() { + EventProcessor processor = newProcessor(1000, 0); + interceptor.addRule().post(EVENTS_ENDPOINT).times(1).respond(503); + interceptor.addRule().post(EVENTS_ENDPOINT).times(1).respond(ACCEPTED_BODY, MEDIATYPE_JSON); + + processor.trackEvent("purchase", "user-1", "1", null, null); + flushAndWait(processor); + + assertEquals(2, recorder.count()); + assertEquals(recorder.bodies().get(0), recorder.bodies().get(1)); + } + + @Test + @SneakyThrows + public void flush_dropsTheBatchAfterTwoServerErrors() { + EventProcessor processor = newProcessor(1000, 0); + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(500); + + processor.trackEvent("purchase", "user-1", "1", null, null); + flushAndWait(processor); + + assertEquals(2, recorder.count()); + assertTrue(processor.getBuffer().isEmpty()); + } + + @Test + @SneakyThrows + public void flush_doesNotRetryOnClientError() { + EventProcessor processor = newProcessor(1000, 0); + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(400); + + processor.trackEvent("purchase", "user-1", "1", null, null); + flushAndWait(processor); + + assertEquals(1, recorder.count()); + assertTrue(processor.getBuffer().isEmpty()); + } + + @Test + @SneakyThrows + public void start_flushesOnTheTimer() { + EventProcessor processor = newProcessor(1000, 500); + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(ACCEPTED_BODY, MEDIATYPE_JSON); + + processor.start(); + processor.trackEvent("purchase", "user-1", "1", null, null); + Thread.sleep(800); + + assertEquals(1, recorder.count()); + assertTrue(processor.getBuffer().isEmpty()); + } + + @Test + @SneakyThrows + public void close_flushesAndStopsTheSchedulerThread() { + EventProcessor processor = newProcessor(1000, 500); + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(ACCEPTED_BODY, MEDIATYPE_JSON); + + processor.start(); + processor.trackEvent("purchase", "user-1", "1", null, null); + processor.close(); + eventProcessor = null; + + assertEquals(1, recorder.count()); + assertTrue(processor.getScheduler().awaitTermination(WAIT_SECONDS, TimeUnit.SECONDS)); + assertTrue(processor.getScheduler().isTerminated()); + } + + @Test + public void trackEvent_neverThrowsWhenTheApiIsMissing() { + EventProcessor processor = newProcessor(1, 0); + processor.setApi(null); + + processor.trackEvent("purchase", "user-1", "1", null, null); + + assertEquals(0, recorder.count()); + assertTrue(processor.getBuffer().isEmpty()); + } + + /** Records every request that reaches the network, with its body. */ + private static class RecordingInterceptor implements Interceptor { + + private final List bodies = Collections.synchronizedList(new ArrayList<>()); + + @Override + public Response intercept(Chain chain) throws IOException { + Request request = chain.request(); + Buffer buffer = new Buffer(); + if (request.body() != null) { + request.body().writeTo(buffer); + } + bodies.add(buffer.readUtf8()); + return chain.proceed(request); + } + + int count() { + return bodies.size(); + } + + List bodies() { + return new ArrayList<>(bodies); + } + } +} From 431a86793345dc46ef795091aa3d49249793fe7f Mon Sep 17 00:00:00 2001 From: wadii Date: Tue, 22 Sep 2026 16:35:08 +0200 Subject: [PATCH 02/16] fix: settle every event batch and never ship transient traits Three defects found in adversarial review of the event processor. flush() registered a batch as in-flight only after serialising it and building the request, both outside the buffer lock. A concurrent flush() could observe an empty buffer and an in-flight set that did not yet contain the batch, and return an already-completed future. The batch is now created and added to inFlight inside the same synchronized block that empties the buffer. send() added the tracking future to inFlight before submitting. If submit threw - RejectedExecutionException once the request processor is closed, or anything out of newPostRequest - the future was left pending forever, wedging every later flush() and burning the full close() timeout. send() now settles it in a finally block on every path, and buffering is a no-op once the processor is closed. Traits were put on the wire verbatim, so a TraitConfig value serialised as {"value":..,"isTransient":..} instead of the flat map the events API expects, and a trait the caller marked transient was shipped to the event store. Values are now unwrapped through TraitConfig, transient traits are dropped, and the map is copied at buffer time so a caller mutating it cannot change a buffered event. Also covers the retry paths that had no tests: connection failures, and Retry.isRetry under statusForcelistOnly, whose attempts-budget branch is what stops a permanently failing endpoint from retrying forever. The timer flush test now waits on a latch instead of sleeping. --- .../com/flagsmith/threads/EventProcessor.java | 123 +++++++---- .../com/flagsmith/FlagsmithRetryTest.java | 57 +++++ .../flagsmith/threads/EventProcessorTest.java | 196 +++++++++++++++++- 3 files changed, 329 insertions(+), 47 deletions(-) diff --git a/src/main/java/com/flagsmith/threads/EventProcessor.java b/src/main/java/com/flagsmith/threads/EventProcessor.java index da9b0193..e38bd5b5 100644 --- a/src/main/java/com/flagsmith/threads/EventProcessor.java +++ b/src/main/java/com/flagsmith/threads/EventProcessor.java @@ -7,6 +7,7 @@ import com.flagsmith.Versions; import com.flagsmith.config.Retry; import com.flagsmith.interfaces.FlagsmithSdk; +import com.flagsmith.models.TraitConfig; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -22,6 +23,7 @@ import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import lombok.Getter; import lombok.Setter; import okhttp3.HttpUrl; @@ -65,6 +67,7 @@ public class EventProcessor { @Setter private FlagsmithSdk api; private FlagsmithLogger logger = new FlagsmithLogger(); + private final AtomicBoolean closed = new AtomicBoolean(false); private ScheduledFuture scheduledFlush; /** @@ -161,22 +164,23 @@ public void trackExposureEvent(String featureName, String identifier, Object val * @return a future completing once every in-flight batch has been delivered or dropped */ public CompletableFuture flush() { - try { - List> batch = null; - - synchronized (lock) { - if (!buffer.isEmpty()) { - batch = new ArrayList<>(buffer); - buffer.clear(); - } - dedupeKeys.clear(); + List> batch = null; + CompletableFuture tracked = null; + + // The batch is registered as in-flight under the same lock that empties the buffer, so a + // concurrent flush() can never observe both an empty buffer and an unregistered batch. + synchronized (lock) { + if (!buffer.isEmpty()) { + batch = new ArrayList<>(buffer); + buffer.clear(); + tracked = new CompletableFuture<>(); + inFlight.add(tracked); } + dedupeKeys.clear(); + } - if (batch != null) { - send(batch); - } - } catch (RuntimeException e) { - logger.error("Failed to flush events.", e); + if (batch != null) { + send(batch, tracked); } return awaitInFlight(); @@ -199,6 +203,7 @@ public void start() { * resources. */ public void close() { + closed.set(true); scheduler.shutdownNow(); try { @@ -215,6 +220,11 @@ public void close() { private void bufferEvent(String event, String featureName, String identifier, Object value, Map traits, Map metadata, boolean dedupe) { + if (closed.get()) { + logger.info("Not buffering event {}: the event processor is closed.", event); + return; + } + try { final String stringValue = value == null ? null : String.valueOf(value); @@ -229,7 +239,7 @@ private void bufferEvent(String event, String featureName, String identifier, Ob eventPayload.put("feature_name", featureName); eventPayload.put("identifier", identifier); eventPayload.put("value", stringValue); - eventPayload.put("traits", traits); + eventPayload.put("traits", eventTraits(traits)); eventPayload.put("metadata", eventMetadata); eventPayload.put("timestamp", System.currentTimeMillis()); @@ -252,6 +262,30 @@ private void bufferEvent(String event, String featureName, String identifier, Ob } } + /** + * Flatten a caller trait map into the flat map of trait values the events API expects. Values + * wrapped in a {@link TraitConfig} are unwrapped, and traits the caller marked transient are + * dropped: transient means "do not persist this against the identity", and an event store keeps + * what it is sent. The result is a copy, so a caller mutating its map afterwards cannot change + * an event already buffered. + */ + private static Map eventTraits(Map traits) { + if (traits == null) { + return null; + } + + Map flattened = new LinkedHashMap<>(); + + for (Map.Entry entry : traits.entrySet()) { + TraitConfig traitConfig = TraitConfig.fromObject(entry.getValue()); + if (!traitConfig.getIsTransient()) { + flattened.put(entry.getKey(), traitConfig.getValue()); + } + } + + return flattened; + } + private static String dedupeKey(String event, String featureName, String identifier, String value, Object experimentId) { return String.join(KEY_SEPARATOR, @@ -266,37 +300,46 @@ private static String nullToEmpty(String value) { return value == null ? "" : value; } - private void send(List> batch) { - if (api == null) { - logger.error("Dropping {} events: the event processor has no API wrapper.", batch.size()); - return; - } - - String payload; + /** + * Hand a batch to the request processor. {@code tracked} is settled exactly once, on every path + * out of here: leaving it pending would wedge every later {@link #flush()}, since those wait on + * it. + */ + private void send(List> batch, CompletableFuture tracked) { + boolean submitted = false; try { - payload = MapperFactory.getMapper() + if (api == null) { + logger.error("Dropping " + batch.size() + + " events: the event processor has no API wrapper."); + return; + } + + String payload = MapperFactory.getMapper() .writeValueAsString(Collections.singletonMap("events", batch)); + + Request request = api + .newPostRequest(eventsEndpoint, RequestBody.create(payload, JSON_MEDIA_TYPE)) + .newBuilder() + .header(SDK_USER_AGENT_HEADER, SDK_USER_AGENT_PREFIX + Versions.getVersion()) + .build(); + + requestProcessor + .submit(request, new TypeReference() {}, Boolean.FALSE, buildRetry()) + .whenComplete((response, error) -> settle(tracked)); + submitted = true; } catch (Exception e) { - logger.error("Error parsing event data to JSON.", e); - return; + logger.error("Dropping " + batch.size() + " events: failed to send them.", e); + } finally { + if (!submitted) { + settle(tracked); + } } + } - Request request = api - .newPostRequest(eventsEndpoint, RequestBody.create(payload, JSON_MEDIA_TYPE)) - .newBuilder() - .header(SDK_USER_AGENT_HEADER, SDK_USER_AGENT_PREFIX + Versions.getVersion()) - .build(); - - CompletableFuture tracked = new CompletableFuture<>(); - inFlight.add(tracked); - - requestProcessor - .submit(request, new TypeReference() {}, Boolean.FALSE, buildRetry()) - .whenComplete((response, error) -> { - inFlight.remove(tracked); - tracked.complete(null); - }); + private void settle(CompletableFuture tracked) { + inFlight.remove(tracked); + tracked.complete(null); } private CompletableFuture awaitInFlight() { diff --git a/src/test/java/com/flagsmith/FlagsmithRetryTest.java b/src/test/java/com/flagsmith/FlagsmithRetryTest.java index cd021087..b2d21ec5 100644 --- a/src/test/java/com/flagsmith/FlagsmithRetryTest.java +++ b/src/test/java/com/flagsmith/FlagsmithRetryTest.java @@ -6,6 +6,8 @@ import static org.junit.jupiter.api.Assertions.*; import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; import java.util.List; public class FlagsmithRetryTest { @@ -66,6 +68,61 @@ public void FlagsmithRetry_validateSleep() { assertTrue(attempts.equals(3)); } + private static Retry oneRetryOnServerErrors() { + Retry retry = new Retry(2); + retry.setStatusForcelist(new HashSet<>(Arrays.asList(500, 502, 503, 504))); + retry.setStatusForcelistOnly(Boolean.TRUE); + return retry; + } + + @Test + public void FlagsmithRetry_statusForcelistOnly_retriesForcedStatusWithinBudget() { + Retry retry = oneRetryOnServerErrors(); + + retry.retryAttempted(); + assertTrue(retry.isRetry(503), "a force-listed status should retry while attempts remain"); + } + + @Test + public void FlagsmithRetry_statusForcelistOnly_stopsAtTheAttemptsBudget() { + Retry retry = oneRetryOnServerErrors(); + + retry.retryAttempted(); + retry.retryAttempted(); + // This is the branch the one-retry-then-drop guarantee rests on: without it, a permanently + // failing endpoint loops forever. + assertFalse(retry.isRetry(503), "a force-listed status must not retry past the budget"); + assertFalse(retry.isRetry(null), "a connection failure must not retry past the budget"); + } + + @Test + public void FlagsmithRetry_statusForcelistOnly_doesNotRetryUnlistedStatus() { + Retry retry = oneRetryOnServerErrors(); + + retry.retryAttempted(); + assertFalse(retry.isRetry(400), "a 4xx must not be retried"); + assertFalse(retry.isRetry(404), "a 4xx must not be retried"); + } + + @Test + public void FlagsmithRetry_statusForcelistOnly_retriesConnectionFailuresWithinBudget() { + Retry retry = oneRetryOnServerErrors(); + + retry.retryAttempted(); + assertTrue(retry.isRetry(null), "a connection failure should retry while attempts remain"); + } + + @Test + public void FlagsmithRetry_defaultPolicyIsUnchanged() { + Retry retry = new Retry(1); + + assertFalse(retry.getStatusForcelistOnly()); + retry.retryAttempted(); + // Historical behaviour: a force-listed status retries regardless of the budget. + assertTrue(retry.isRetry(503)); + assertFalse(retry.isRetry(401)); + } + @Test public void FlagsmithRetry_shouldNotExceedBackoffMax() { Retry retryObject = new Retry(7); diff --git a/src/test/java/com/flagsmith/threads/EventProcessorTest.java b/src/test/java/com/flagsmith/threads/EventProcessorTest.java index 6af0e55a..821dda08 100644 --- a/src/test/java/com/flagsmith/threads/EventProcessorTest.java +++ b/src/test/java/com/flagsmith/threads/EventProcessorTest.java @@ -2,10 +2,13 @@ import static okhttp3.mock.MediaTypes.MEDIATYPE_JSON; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -13,14 +16,19 @@ import com.flagsmith.FlagsmithLogger; import com.flagsmith.MapperFactory; import com.flagsmith.interfaces.FlagsmithSdk; +import com.flagsmith.models.TraitConfig; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import java.util.regex.Pattern; import lombok.SneakyThrows; import okhttp3.HttpUrl; @@ -73,10 +81,16 @@ public void tearDown() { } private EventProcessor newProcessor(int maxBufferItems, int flushIntervalMillis) { - OkHttpClient client = new OkHttpClient.Builder() - .addInterceptor(recorder) - .addInterceptor(interceptor) - .build(); + return newProcessor(maxBufferItems, flushIntervalMillis, null); + } + + private EventProcessor newProcessor( + int maxBufferItems, int flushIntervalMillis, Interceptor extra) { + OkHttpClient.Builder clientBuilder = new OkHttpClient.Builder().addInterceptor(recorder); + if (extra != null) { + clientBuilder.addInterceptor(extra); + } + OkHttpClient client = clientBuilder.addInterceptor(interceptor).build(); eventProcessor = new EventProcessor( HttpUrl.get(EVENTS_URI), maxBufferItems, @@ -310,17 +324,161 @@ public void flush_doesNotRetryOnClientError() { assertTrue(processor.getBuffer().isEmpty()); } + @Test + @SneakyThrows + public void flush_retriesOnceOnAConnectionFailureThenDelivers() { + EventProcessor processor = newProcessor(1000, 0, new FailingInterceptor(1)); + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(ACCEPTED_BODY, MEDIATYPE_JSON); + + processor.trackEvent("purchase", "user-1", "1", null, null); + flushAndWait(processor); + + assertEquals(2, recorder.count()); + assertEquals(recorder.bodies().get(0), recorder.bodies().get(1)); + } + + @Test + @SneakyThrows + public void flush_dropsTheBatchAfterTwoConnectionFailures() { + EventProcessor processor = newProcessor(1000, 0, new FailingInterceptor(Integer.MAX_VALUE)); + + processor.trackEvent("purchase", "user-1", "1", null, null); + flushAndWait(processor); + + assertEquals(2, recorder.count()); + assertTrue(processor.getBuffer().isEmpty()); + } + + @Test + @SneakyThrows + public void flush_waitsForABatchAnotherThreadIsAlreadySending() { + EventProcessor processor = newProcessor(1000, 0); + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(ACCEPTED_BODY, MEDIATYPE_JSON); + + CountDownLatch insideSend = new CountDownLatch(1); + CountDownLatch release = new CountDownLatch(1); + doAnswer((invocation) -> { + insideSend.countDown(); + release.await(WAIT_SECONDS, TimeUnit.SECONDS); + return new Request.Builder() + .url((HttpUrl) invocation.getArgument(0)) + .header("X-Environment-Key", "api-key") + .post((RequestBody) invocation.getArgument(1)) + .build(); + }).when(api).newPostRequest(any(), any()); + + processor.trackEvent("purchase", "user-1", "1", null, null); + + Thread sender = new Thread(processor::flush, "test-sender"); + sender.start(); + assertTrue(insideSend.await(WAIT_SECONDS, TimeUnit.SECONDS)); + + // The buffer is already empty here, but the batch is still on its way out. + CompletableFuture second = processor.flush(); + assertFalse(second.isDone(), "flush() completed while another thread was mid-send"); + + release.countDown(); + second.get(WAIT_SECONDS, TimeUnit.SECONDS); + sender.join(TimeUnit.SECONDS.toMillis(WAIT_SECONDS)); + + assertEquals(1, recorder.count()); + } + + @Test + @SneakyThrows + public void flush_completesWhenBuildingTheRequestThrows() { + EventProcessor processor = newProcessor(1000, 0); + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(ACCEPTED_BODY, MEDIATYPE_JSON); + doThrow(new IllegalStateException("boom")).when(api).newPostRequest(any(), any()); + + processor.trackEvent("purchase", "user-1", "1", null, null); + flushAndWait(processor); + + assertEquals(0, recorder.count()); + // A later flush must not inherit a batch that was never settled. + flushAndWait(processor); + } + + @Test + @SneakyThrows + public void flush_completesWhenTheRequestProcessorIsAlreadyShutDown() { + EventProcessor processor = newProcessor(1000, 0); + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(ACCEPTED_BODY, MEDIATYPE_JSON); + + processor.trackEvent("purchase", "user-1", "1", null, null); + processor.getRequestProcessor().close(); + + flushAndWait(processor); + + assertEquals(0, recorder.count()); + } + + @Test + @SneakyThrows + public void trackEvent_isANoOpAfterClose() { + EventProcessor processor = newProcessor(2, 0); + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(ACCEPTED_BODY, MEDIATYPE_JSON); + + processor.close(); + eventProcessor = null; + int postsAfterClose = recorder.count(); + + processor.trackEvent("purchase", "user-1", "1", null, null); + processor.trackEvent("purchase", "user-2", "2", null, null); + processor.trackExposureEvent("checkout_cta", "user-1", "treatment", null, null); + + assertTrue(processor.getBuffer().isEmpty()); + flushAndWait(processor); + assertEquals(postsAfterClose, recorder.count()); + } + + @Test + public void trackExposureEvent_unwrapsTraitConfigsAndDropsTransientTraits() { + EventProcessor processor = newProcessor(1000, 0); + + Map traits = new LinkedHashMap<>(); + traits.put("plan", "premium"); + traits.put("tier", new TraitConfig("gold", false)); + traits.put("session_id", new TraitConfig("abc123", true)); + + processor.trackExposureEvent("checkout_cta", "user-1", "treatment", traits, null); + + @SuppressWarnings("unchecked") + Map buffered = + (Map) processor.getBuffer().get(0).get("traits"); + assertEquals(2, buffered.size()); + assertEquals("premium", buffered.get("plan")); + assertEquals("gold", buffered.get("tier")); + assertFalse(buffered.containsKey("session_id"), "a transient trait reached the events API"); + } + + @Test + public void trackEvent_copiesTheTraitMapAtBufferTime() { + EventProcessor processor = newProcessor(1000, 0); + + Map traits = new LinkedHashMap<>(); + traits.put("plan", "premium"); + processor.trackEvent("purchase", "user-1", "1", traits, null); + + traits.put("plan", "mutated"); + traits.put("added_later", "nope"); + + @SuppressWarnings("unchecked") + Map buffered = + (Map) processor.getBuffer().get(0).get("traits"); + assertEquals(Collections.singletonMap("plan", "premium"), buffered); + } + @Test @SneakyThrows public void start_flushesOnTheTimer() { - EventProcessor processor = newProcessor(1000, 500); + EventProcessor processor = newProcessor(1000, 100); interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(ACCEPTED_BODY, MEDIATYPE_JSON); processor.start(); processor.trackEvent("purchase", "user-1", "1", null, null); - Thread.sleep(800); - assertEquals(1, recorder.count()); + assertTrue(recorder.awaitFirstRequest(WAIT_SECONDS), "the timer never flushed"); assertTrue(processor.getBuffer().isEmpty()); } @@ -355,6 +513,7 @@ public void trackEvent_neverThrowsWhenTheApiIsMissing() { private static class RecordingInterceptor implements Interceptor { private final List bodies = Collections.synchronizedList(new ArrayList<>()); + private final CountDownLatch firstRequest = new CountDownLatch(1); @Override public Response intercept(Chain chain) throws IOException { @@ -364,6 +523,7 @@ public Response intercept(Chain chain) throws IOException { request.body().writeTo(buffer); } bodies.add(buffer.readUtf8()); + firstRequest.countDown(); return chain.proceed(request); } @@ -374,5 +534,27 @@ int count() { List bodies() { return new ArrayList<>(bodies); } + + boolean awaitFirstRequest(long seconds) throws InterruptedException { + return firstRequest.await(seconds, TimeUnit.SECONDS); + } + } + + /** Simulates a connection failure for the first n attempts. */ + private static class FailingInterceptor implements Interceptor { + + private final AtomicInteger remainingFailures; + + FailingInterceptor(int failures) { + this.remainingFailures = new AtomicInteger(failures); + } + + @Override + public Response intercept(Chain chain) throws IOException { + if (remainingFailures.getAndDecrement() > 0) { + throw new IOException("connection refused"); + } + return chain.proceed(chain.request()); + } } } From bc0a81e4feab41d58c539539a80d29c0171548ad Mon Sep 17 00:00:00 2001 From: wadii Date: Tue, 22 Sep 2026 16:35:22 +0200 Subject: [PATCH 03/16] fix: allow a custom events URI without enabling events eventsUri() marked the events config as touched, so setting a custom events host threw at build() unless that same config also enabled events. That blocked the ordinary case of a shared configuration carrying the URL while only some services opt in. The spec only requires the buffer size and flush interval to be gated, which they still are. --- .../com/flagsmith/config/FlagsmithConfig.java | 5 +- .../com/flagsmith/FlagsmithClientTest.java | 70 +++++++++++++++++++ 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/flagsmith/config/FlagsmithConfig.java b/src/main/java/com/flagsmith/config/FlagsmithConfig.java index b711c916..9eeecbd5 100644 --- a/src/main/java/com/flagsmith/config/FlagsmithConfig.java +++ b/src/main/java/com/flagsmith/config/FlagsmithConfig.java @@ -299,7 +299,9 @@ public Builder withEnableAnalytics(Boolean enable) { } /** - * Set the base URL of the Flagsmith events API, overriding the default one. + * Set the base URL of the Flagsmith events API, overriding the default one. Harmless when + * events are not enabled, so that a shared configuration can carry the URL for the services + * that do enable them. * * @param eventsUri the new base URI for the events API * @return the Builder @@ -307,7 +309,6 @@ public Builder withEnableAnalytics(Boolean enable) { public Builder eventsUri(String eventsUri) { if (eventsUri != null) { this.eventsUri = HttpUrl.get(eventsUri.endsWith("/") ? eventsUri : eventsUri + "/"); - this.eventsConfigured = Boolean.TRUE; } return this; } diff --git a/src/test/java/com/flagsmith/FlagsmithClientTest.java b/src/test/java/com/flagsmith/FlagsmithClientTest.java index c002df39..198dd4e6 100644 --- a/src/test/java/com/flagsmith/FlagsmithClientTest.java +++ b/src/test/java/com/flagsmith/FlagsmithClientTest.java @@ -7,6 +7,7 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -39,6 +40,7 @@ import com.flagsmith.threads.RequestProcessor; import java.io.IOException; +import java.time.Duration; import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -1026,6 +1028,74 @@ public void testEventsConfigWithoutEnableEventsThrows() { () -> FlagsmithConfig.newBuilder().withEventsFlushIntervalMillis(10).build()); } + @Test + public void testEventsUriWithoutEnableEventsIsHarmless() { + // A shared configuration may carry the events URL for the services that do enable events. + FlagsmithConfig config = FlagsmithConfig.newBuilder() + .eventsUri("http://events-uri") + .build(); + + assertNull(config.getEventProcessor()); + assertEquals("http://events-uri/", config.getEventsUri().toString()); + } + + @Test + public void testGetExperimentFlagPassesTraitsThroughToTheExposure() + throws FlagsmithClientError { + String baseUrl = "http://bad-url"; + MockInterceptor interceptor = new MockInterceptor(); + EventProcessor processor = mock(EventProcessor.class); + FlagsmithClient client = FlagsmithClient.newBuilder() + .withConfiguration(eventsConfigBuilder(baseUrl, interceptor) + .withEventProcessor(processor) + .build()) + .setApiKey("api-key") + .build(); + respondWithExperimentFlags(baseUrl, interceptor); + + Map traits = new HashMap<>(); + traits.put("plan", "premium"); + traits.put("session_id", new TraitConfig("abc123", true)); + + client.getExperimentFlag("checkout_cta", "user-1", traits); + verify(processor, times(1)).trackExposureEvent( + eq("checkout_cta"), eq("user-1"), eq("treatment"), eq(traits), any()); + + // The two-argument overload passes an empty map rather than null. + client.getExperimentFlag("checkout_cta", "user-2"); + verify(processor, times(1)).trackExposureEvent( + eq("checkout_cta"), eq("user-2"), eq("treatment"), eq(new HashMap<>()), any()); + } + + @Test + public void testCloseDoesNotWedgeLaterFlushes() throws FlagsmithClientError { + String baseUrl = "http://bad-url"; + MockInterceptor interceptor = new MockInterceptor(); + FlagsmithClient client = FlagsmithClient.newBuilder() + .withConfiguration(eventsConfigBuilder(baseUrl, interceptor) + .withEventsMaxBufferItems(1) + .withEventsFlushIntervalMillis(0) + .build()) + .setApiKey("api-key") + .build(); + interceptor.addRule() + .post("http://events-uri/v1/events") + .anyTimes() + .respond("{\"accepted\": 0, \"rejected\": []}", MEDIATYPE_JSON); + + client.close(); + + // Buffering after close is a no-op, and nothing left behind may stall a later flush. + client.trackEvent("purchase", "user-1"); + client.trackExposureEvent("checkout_cta", "user-1", "treatment"); + + assertTrue(assertTimeoutPreemptively(Duration.ofSeconds(10), + () -> { + client.flushEvents().join(); + return Boolean.TRUE; + })); + } + @Test public void testEventsInOfflineModeThrowsAtBuild() { EventProcessor processor = mock(EventProcessor.class); From 34ca69d68c9a8d0b1526bf3924cc7dc12e4ea9b0 Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 23 Sep 2026 09:50:44 +0200 Subject: [PATCH 04/16] refactor: stop publishing EventProcessor internals as public API The class-level @Getter made buffer, lock, dedupeKeys, scheduler, inFlight, requestProcessor, logger and api public getters. Once released, every one of them is API the SDK has to keep; the buffer getter also handed out a list guarded by a private lock. Only the four immutable settings stay public. The test constructor and the scheduler/request processor accessors become package-private, and tests read the buffer through a snapshot taken under the lock. --- .../com/flagsmith/threads/EventProcessor.java | 39 ++++++++++++++----- .../flagsmith/threads/EventProcessorTest.java | 36 ++++++++--------- 2 files changed, 47 insertions(+), 28 deletions(-) diff --git a/src/main/java/com/flagsmith/threads/EventProcessor.java b/src/main/java/com/flagsmith/threads/EventProcessor.java index e38bd5b5..b4d233ea 100644 --- a/src/main/java/com/flagsmith/threads/EventProcessor.java +++ b/src/main/java/com/flagsmith/threads/EventProcessor.java @@ -24,6 +24,7 @@ import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import lombok.AccessLevel; import lombok.Getter; import lombok.Setter; import okhttp3.HttpUrl; @@ -39,7 +40,6 @@ * Exposure events are deduplicated within a flush window. Nothing thrown here ever reaches caller * code: every failure is logged instead. */ -@Getter public class EventProcessor { /** Name of the reserved event recorded when an identity is exposed to an experiment. */ @@ -54,16 +54,29 @@ public class EventProcessor { private static final MediaType JSON_MEDIA_TYPE = MediaType.get("application/json; charset=utf-8"); + /** The URL batches are POSTed to. */ + @Getter private final HttpUrl eventsEndpoint; + /** The number of buffered events that triggers an immediate flush. */ + @Getter private final int maxBufferItems; + /** The interval between timed flushes; 0 means there is no timer. */ + @Getter private final int flushIntervalMillis; + /** How long a single POST is expected to take; {@link #close()} waits up to twice this. */ + @Getter private final int requestTimeoutMillis; + // Everything below is internal state, deliberately not exposed: once published, a getter + // would be public API for as long as the SDK is supported. private final List> buffer = new ArrayList<>(); private final Set dedupeKeys = new HashSet<>(); private final Object lock = new Object(); + @Getter(AccessLevel.PACKAGE) private final ScheduledExecutorService scheduler; private final Set> inFlight = ConcurrentHashMap.newKeySet(); + @Getter(AccessLevel.PACKAGE) private final RequestProcessor requestProcessor; + /** The API wrapper used to build requests; injected by {@code FlagsmithClient.Builder}. */ @Setter private FlagsmithSdk api; private FlagsmithLogger logger = new FlagsmithLogger(); @@ -77,7 +90,8 @@ public class EventProcessor { * @param eventsUri base URI of the events API, e.g. https://events.api.flagsmith.com/ * @param maxBufferItems number of buffered events that triggers an immediate flush * @param flushIntervalMillis interval between timed flushes; 0 disables the timer - * @param requestTimeoutMillis how long {@link #close()} waits for each in-flight batch + * @param requestTimeoutMillis how long a single POST is expected to take; {@link #close()} + * waits up to twice this for in-flight batches */ public EventProcessor(OkHttpClient client, HttpUrl eventsUri, int maxBufferItems, int flushIntervalMillis, int requestTimeoutMillis) { @@ -86,15 +100,10 @@ public EventProcessor(OkHttpClient client, HttpUrl eventsUri, int maxBufferItems } /** - * Instantiate with a request processor, for tests. - * - * @param eventsUri base URI of the events API - * @param maxBufferItems number of buffered events that triggers an immediate flush - * @param flushIntervalMillis interval between timed flushes; 0 disables the timer - * @param requestTimeoutMillis how long {@link #close()} waits for each in-flight batch - * @param requestProcessor request processor used to POST batches + * Instantiate with a request processor. Package-private: it exists for tests, and is not + * something callers should come to depend on. */ - public EventProcessor(HttpUrl eventsUri, int maxBufferItems, int flushIntervalMillis, + EventProcessor(HttpUrl eventsUri, int maxBufferItems, int flushIntervalMillis, int requestTimeoutMillis, RequestProcessor requestProcessor) { this.eventsEndpoint = eventsUri.newBuilder(EVENTS_PATH).build(); this.maxBufferItems = maxBufferItems; @@ -345,4 +354,14 @@ private void settle(CompletableFuture tracked) { private CompletableFuture awaitInFlight() { return CompletableFuture.allOf(inFlight.toArray(new CompletableFuture[0])); } + + /** + * A snapshot of the buffer, for tests. Taken under the lock, so a test never iterates the live + * list while another thread is appending to it. + */ + List> bufferedEvents() { + synchronized (lock) { + return new ArrayList<>(buffer); + } + } } diff --git a/src/test/java/com/flagsmith/threads/EventProcessorTest.java b/src/test/java/com/flagsmith/threads/EventProcessorTest.java index 821dda08..78253677 100644 --- a/src/test/java/com/flagsmith/threads/EventProcessorTest.java +++ b/src/test/java/com/flagsmith/threads/EventProcessorTest.java @@ -118,9 +118,9 @@ public void trackEvent_buffersStringifiedValueSdkVersionAndTimestamp() { long before = System.currentTimeMillis(); processor.trackEvent("purchase", "user-123", 49.0, traits, metadata); - assertEquals(1, processor.getBuffer().size()); + assertEquals(1, processor.bufferedEvents().size()); - Map event = processor.getBuffer().get(0); + Map event = processor.bufferedEvents().get(0); assertEquals( Arrays.asList("event", "feature_name", "identifier", "value", "traits", "metadata", "timestamp"), @@ -146,7 +146,7 @@ public void trackEvent_buffersNullValueAsNull() { processor.trackEvent("purchase", "user-123", null, null, null); - Map event = processor.getBuffer().get(0); + Map event = processor.bufferedEvents().get(0); assertNull(event.get("value")); assertNull(event.get("traits")); } @@ -159,7 +159,7 @@ public void trackExposureEvent_dedupesIdenticalExposuresWithinTheFlushWindow() { processor.trackExposureEvent("checkout_cta", "user-1", "treatment", null, metadata); processor.trackExposureEvent("checkout_cta", "user-1", "treatment", null, metadata); - assertEquals(1, processor.getBuffer().size()); + assertEquals(1, processor.bufferedEvents().size()); } @Test @@ -174,7 +174,7 @@ public void trackExposureEvent_doesNotDedupeWhenAnyKeyPartDiffers() { processor.trackExposureEvent("checkout_cta", "user-1", "treatment", null, Collections.singletonMap("experiment_id", 43)); - assertEquals(5, processor.getBuffer().size()); + assertEquals(5, processor.bufferedEvents().size()); } @Test @@ -189,7 +189,7 @@ public void trackExposureEvent_buffersAgainAfterAFlush() { processor.trackExposureEvent("checkout_cta", "user-1", "treatment", null, metadata); - assertEquals(1, processor.getBuffer().size()); + assertEquals(1, processor.bufferedEvents().size()); assertEquals(1, recorder.count()); } @@ -200,7 +200,7 @@ public void trackEvent_neverDedupesCustomEvents() { processor.trackEvent("purchase", "user-1", "49.00", null, null); processor.trackEvent("purchase", "user-1", "49.00", null, null); - assertEquals(2, processor.getBuffer().size()); + assertEquals(2, processor.bufferedEvents().size()); } @Test @@ -232,7 +232,7 @@ public void flush_postsTheBatchToTheEventsEndpoint() { assertEquals(42, event.get("metadata").get("experiment_id").asInt()); assertTrue(event.get("metadata").has("sdk_version")); assertTrue(event.get("timestamp").isNumber()); - assertTrue(processor.getBuffer().isEmpty()); + assertTrue(processor.bufferedEvents().isEmpty()); } @Test @@ -252,10 +252,10 @@ public void trackEvent_flushesWhenTheBufferIsFull() { interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(ACCEPTED_BODY, MEDIATYPE_JSON); processor.trackEvent("purchase", "user-1", "1", null, null); - assertEquals(1, processor.getBuffer().size()); + assertEquals(1, processor.bufferedEvents().size()); processor.trackEvent("purchase", "user-2", "2", null, null); - assertTrue(processor.getBuffer().isEmpty()); + assertTrue(processor.bufferedEvents().isEmpty()); flushAndWait(processor); @@ -308,7 +308,7 @@ public void flush_dropsTheBatchAfterTwoServerErrors() { flushAndWait(processor); assertEquals(2, recorder.count()); - assertTrue(processor.getBuffer().isEmpty()); + assertTrue(processor.bufferedEvents().isEmpty()); } @Test @@ -321,7 +321,7 @@ public void flush_doesNotRetryOnClientError() { flushAndWait(processor); assertEquals(1, recorder.count()); - assertTrue(processor.getBuffer().isEmpty()); + assertTrue(processor.bufferedEvents().isEmpty()); } @Test @@ -346,7 +346,7 @@ public void flush_dropsTheBatchAfterTwoConnectionFailures() { flushAndWait(processor); assertEquals(2, recorder.count()); - assertTrue(processor.getBuffer().isEmpty()); + assertTrue(processor.bufferedEvents().isEmpty()); } @Test @@ -427,7 +427,7 @@ public void trackEvent_isANoOpAfterClose() { processor.trackEvent("purchase", "user-2", "2", null, null); processor.trackExposureEvent("checkout_cta", "user-1", "treatment", null, null); - assertTrue(processor.getBuffer().isEmpty()); + assertTrue(processor.bufferedEvents().isEmpty()); flushAndWait(processor); assertEquals(postsAfterClose, recorder.count()); } @@ -445,7 +445,7 @@ public void trackExposureEvent_unwrapsTraitConfigsAndDropsTransientTraits() { @SuppressWarnings("unchecked") Map buffered = - (Map) processor.getBuffer().get(0).get("traits"); + (Map) processor.bufferedEvents().get(0).get("traits"); assertEquals(2, buffered.size()); assertEquals("premium", buffered.get("plan")); assertEquals("gold", buffered.get("tier")); @@ -465,7 +465,7 @@ public void trackEvent_copiesTheTraitMapAtBufferTime() { @SuppressWarnings("unchecked") Map buffered = - (Map) processor.getBuffer().get(0).get("traits"); + (Map) processor.bufferedEvents().get(0).get("traits"); assertEquals(Collections.singletonMap("plan", "premium"), buffered); } @@ -479,7 +479,7 @@ public void start_flushesOnTheTimer() { processor.trackEvent("purchase", "user-1", "1", null, null); assertTrue(recorder.awaitFirstRequest(WAIT_SECONDS), "the timer never flushed"); - assertTrue(processor.getBuffer().isEmpty()); + assertTrue(processor.bufferedEvents().isEmpty()); } @Test @@ -506,7 +506,7 @@ public void trackEvent_neverThrowsWhenTheApiIsMissing() { processor.trackEvent("purchase", "user-1", "1", null, null); assertEquals(0, recorder.count()); - assertTrue(processor.getBuffer().isEmpty()); + assertTrue(processor.bufferedEvents().isEmpty()); } /** Records every request that reaches the network, with its body. */ From d2fee3482d196a3a7951d8c2282b76bd2f35761c Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 23 Sep 2026 09:52:03 +0200 Subject: [PATCH 05/16] fix: serialise each event when it is buffered, not with its batch Traits and metadata are arbitrary caller objects, and were only serialised when the whole batch was. A single value Jackson cannot handle (a java.time type, a bean without properties) failed that serialisation and dropped every event in the batch, up to 1000. Converting traits and metadata to JSON trees at buffer time drops and logs only the offending event. It also deep-copies them, where the previous copy was shallow and a caller mutating a nested map could still change an event already buffered. --- .../com/flagsmith/threads/EventProcessor.java | 20 +++++-- .../flagsmith/threads/EventProcessorTest.java | 60 ++++++++++++++----- 2 files changed, 58 insertions(+), 22 deletions(-) diff --git a/src/main/java/com/flagsmith/threads/EventProcessor.java b/src/main/java/com/flagsmith/threads/EventProcessor.java index b4d233ea..e16e2470 100644 --- a/src/main/java/com/flagsmith/threads/EventProcessor.java +++ b/src/main/java/com/flagsmith/threads/EventProcessor.java @@ -2,6 +2,7 @@ import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; import com.flagsmith.FlagsmithLogger; import com.flagsmith.MapperFactory; import com.flagsmith.Versions; @@ -242,21 +243,29 @@ private void bufferEvent(String event, String featureName, String identifier, Ob eventMetadata.putAll(metadata); } eventMetadata.put(SDK_VERSION_KEY, Versions.getVersion()); + final Object experimentId = eventMetadata.get(EXPERIMENT_ID_KEY); + + // Traits and metadata are caller objects of any shape. Turning them into JSON trees here, + // rather than at flush time, means a value Jackson cannot serialise drops this one event + // (logged below) instead of failing the whole batch it would later be sent in. It is also + // a deep copy, so a caller mutating a nested map afterwards cannot change a buffered event. + ObjectMapper mapper = MapperFactory.getMapper(); + Map eventTraits = eventTraits(traits); Map eventPayload = new LinkedHashMap<>(); eventPayload.put("event", event); eventPayload.put("feature_name", featureName); eventPayload.put("identifier", identifier); eventPayload.put("value", stringValue); - eventPayload.put("traits", eventTraits(traits)); - eventPayload.put("metadata", eventMetadata); + eventPayload.put("traits", eventTraits == null ? null : mapper.valueToTree(eventTraits)); + eventPayload.put("metadata", mapper.valueToTree(eventMetadata)); eventPayload.put("timestamp", System.currentTimeMillis()); boolean isFull; synchronized (lock) { - if (dedupe && !dedupeKeys.add(dedupeKey( - event, featureName, identifier, stringValue, eventMetadata.get(EXPERIMENT_ID_KEY)))) { + if (dedupe && !dedupeKeys.add( + dedupeKey(event, featureName, identifier, stringValue, experimentId))) { return; } buffer.add(eventPayload); @@ -275,8 +284,7 @@ private void bufferEvent(String event, String featureName, String identifier, Ob * Flatten a caller trait map into the flat map of trait values the events API expects. Values * wrapped in a {@link TraitConfig} are unwrapped, and traits the caller marked transient are * dropped: transient means "do not persist this against the identity", and an event store keeps - * what it is sent. The result is a copy, so a caller mutating its map afterwards cannot change - * an event already buffered. + * what it is sent. */ private static Map eventTraits(Map traits) { if (traits == null) { diff --git a/src/test/java/com/flagsmith/threads/EventProcessorTest.java b/src/test/java/com/flagsmith/threads/EventProcessorTest.java index 78253677..0be77807 100644 --- a/src/test/java/com/flagsmith/threads/EventProcessorTest.java +++ b/src/test/java/com/flagsmith/threads/EventProcessorTest.java @@ -129,11 +129,10 @@ public void trackEvent_buffersStringifiedValueSdkVersionAndTimestamp() { assertNull(event.get("feature_name")); assertEquals("user-123", event.get("identifier")); assertEquals("49.0", event.get("value")); - assertEquals(traits, event.get("traits")); + assertEquals(MapperFactory.getMapper().valueToTree(traits), event.get("traits")); - @SuppressWarnings("unchecked") - Map eventMetadata = (Map) event.get("metadata"); - assertEquals("checkout", eventMetadata.get("source")); + JsonNode eventMetadata = (JsonNode) event.get("metadata"); + assertEquals("checkout", eventMetadata.get("source").asText()); assertNotNull(eventMetadata.get("sdk_version")); long timestamp = (Long) event.get("timestamp"); @@ -443,30 +442,59 @@ public void trackExposureEvent_unwrapsTraitConfigsAndDropsTransientTraits() { processor.trackExposureEvent("checkout_cta", "user-1", "treatment", traits, null); - @SuppressWarnings("unchecked") - Map buffered = - (Map) processor.bufferedEvents().get(0).get("traits"); + JsonNode buffered = (JsonNode) processor.bufferedEvents().get(0).get("traits"); assertEquals(2, buffered.size()); - assertEquals("premium", buffered.get("plan")); - assertEquals("gold", buffered.get("tier")); - assertFalse(buffered.containsKey("session_id"), "a transient trait reached the events API"); + assertEquals("premium", buffered.get("plan").asText()); + assertEquals("gold", buffered.get("tier").asText()); + assertFalse(buffered.has("session_id"), "a transient trait reached the events API"); } @Test - public void trackEvent_copiesTheTraitMapAtBufferTime() { + public void trackEvent_copiesTraitsAndMetadataDeeplyAtBufferTime() { EventProcessor processor = newProcessor(1000, 0); Map traits = new LinkedHashMap<>(); traits.put("plan", "premium"); - processor.trackEvent("purchase", "user-1", "1", traits, null); + Map nested = new HashMap<>(); + nested.put("step", "payment"); + Map metadata = new HashMap<>(); + metadata.put("context", nested); + processor.trackEvent("purchase", "user-1", "1", traits, metadata); traits.put("plan", "mutated"); traits.put("added_later", "nope"); + nested.put("step", "mutated"); + + Map event = processor.bufferedEvents().get(0); + assertEquals( + MapperFactory.getMapper().valueToTree(Collections.singletonMap("plan", "premium")), + event.get("traits")); + assertEquals("payment", + ((JsonNode) event.get("metadata")).get("context").get("step").asText()); + } + + @Test + @SneakyThrows + public void trackEvent_dropsOnlyTheEventWhoseValuesCannotBeSerialised() { + EventProcessor processor = newProcessor(1000, 0); + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(ACCEPTED_BODY, MEDIATYPE_JSON); + + processor.trackEvent("purchase", "user-1", "1", null, null); + // Jackson has no serialiser for a bean without properties. + processor.trackEvent("purchase", "user-2", "2", + Collections.singletonMap("opaque", new Object()), null); + processor.trackEvent("purchase", "user-3", "3", null, + Collections.singletonMap("opaque", new Object())); + processor.trackEvent("purchase", "user-4", "4", null, null); - @SuppressWarnings("unchecked") - Map buffered = - (Map) processor.bufferedEvents().get(0).get("traits"); - assertEquals(Collections.singletonMap("plan", "premium"), buffered); + assertEquals(2, processor.bufferedEvents().size()); + flushAndWait(processor); + + assertEquals(1, recorder.count()); + JsonNode events = MapperFactory.getMapper().readTree(recorder.bodies().get(0)).get("events"); + assertEquals(2, events.size()); + assertEquals("user-1", events.get(0).get("identifier").asText()); + assertEquals("user-4", events.get(1).get("identifier").asText()); } @Test From 66daa749f369bf0bf81f05bbdc14f1b81ddfde39 Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 23 Sep 2026 09:54:34 +0200 Subject: [PATCH 06/16] fix: bound in-flight event batches and log events the API rejects While the events API is slow or down, each batch can hold a request thread for two timeouts plus backoff, and the request processor's queue is unbounded. Traffic kept producing batches faster than they were given up on, so an outage grew memory with the host app's load. A flush now drops its batch, with an error log, once ten batches are already waiting. The API answers 202 even when it rejects some events, listing them under 'rejected'. Those were discarded unread; the count and the first rejection are now logged. --- .../com/flagsmith/threads/EventProcessor.java | 43 ++++++- .../flagsmith/threads/EventProcessorTest.java | 105 ++++++++++++++++++ 2 files changed, 144 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/flagsmith/threads/EventProcessor.java b/src/main/java/com/flagsmith/threads/EventProcessor.java index e16e2470..4975424b 100644 --- a/src/main/java/com/flagsmith/threads/EventProcessor.java +++ b/src/main/java/com/flagsmith/threads/EventProcessor.java @@ -54,6 +54,13 @@ public class EventProcessor { private static final String KEY_SEPARATOR = "\u0000"; private static final MediaType JSON_MEDIA_TYPE = MediaType.get("application/json; charset=utf-8"); + /** + * The most batches that may be waiting on the events API at once. Beyond it a flush drops its + * batch instead of queueing it: while the API is slow or down, batches are produced faster + * than they are retried and given up on, and an unbounded queue of them would grow with the + * host application's traffic until it ran out of memory. + */ + static final int MAX_IN_FLIGHT_BATCHES = 10; /** The URL batches are POSTed to. */ @Getter @@ -176,19 +183,29 @@ public void trackExposureEvent(String featureName, String identifier, Object val public CompletableFuture flush() { List> batch = null; CompletableFuture tracked = null; + int dropped = 0; // The batch is registered as in-flight under the same lock that empties the buffer, so a // concurrent flush() can never observe both an empty buffer and an unregistered batch. synchronized (lock) { if (!buffer.isEmpty()) { - batch = new ArrayList<>(buffer); + if (inFlight.size() >= MAX_IN_FLIGHT_BATCHES) { + dropped = buffer.size(); + } else { + batch = new ArrayList<>(buffer); + tracked = new CompletableFuture<>(); + inFlight.add(tracked); + } buffer.clear(); - tracked = new CompletableFuture<>(); - inFlight.add(tracked); } dedupeKeys.clear(); } + if (dropped > 0) { + logger.error("Dropping " + dropped + " events: " + MAX_IN_FLIGHT_BATCHES + + " earlier batches are still waiting on the events API."); + } + if (batch != null) { send(batch, tracked); } @@ -343,7 +360,13 @@ private void send(List> batch, CompletableFuture track requestProcessor .submit(request, new TypeReference() {}, Boolean.FALSE, buildRetry()) - .whenComplete((response, error) -> settle(tracked)); + .whenComplete((response, error) -> { + try { + logRejections(response, batch.size()); + } finally { + settle(tracked); + } + }); submitted = true; } catch (Exception e) { logger.error("Dropping " + batch.size() + " events: failed to send them.", e); @@ -354,6 +377,18 @@ private void send(List> batch, CompletableFuture track } } + /** + * The events API accepts a batch with a 202 even when it rejects some of its events, listing + * those under {@code rejected}. Without this they would vanish without a trace. + */ + private void logRejections(JsonNode response, int batchSize) { + JsonNode rejected = response == null ? null : response.get("rejected"); + if (rejected != null && rejected.isArray() && rejected.size() > 0) { + logger.error("The events API rejected " + rejected.size() + " of " + batchSize + + " events. First rejection: " + rejected.get(0)); + } + } + private void settle(CompletableFuture tracked) { inFlight.remove(tracked); tracked.complete(null); diff --git a/src/test/java/com/flagsmith/threads/EventProcessorTest.java b/src/test/java/com/flagsmith/threads/EventProcessorTest.java index 0be77807..4edd9a27 100644 --- a/src/test/java/com/flagsmith/threads/EventProcessorTest.java +++ b/src/test/java/com/flagsmith/threads/EventProcessorTest.java @@ -7,9 +7,12 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.contains; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.fasterxml.jackson.databind.JsonNode; @@ -33,10 +36,13 @@ import lombok.SneakyThrows; import okhttp3.HttpUrl; import okhttp3.Interceptor; +import okhttp3.MediaType; import okhttp3.OkHttpClient; +import okhttp3.Protocol; import okhttp3.Request; import okhttp3.RequestBody; import okhttp3.Response; +import okhttp3.ResponseBody; import okhttp3.mock.MockInterceptor; import okio.Buffer; import org.junit.jupiter.api.AfterEach; @@ -537,6 +543,105 @@ public void trackEvent_neverThrowsWhenTheApiIsMissing() { assertTrue(processor.bufferedEvents().isEmpty()); } + @Test + @SneakyThrows + public void flush_dropsBatchesBeyondTheInFlightLimitInsteadOfQueueingThem() { + BlockingInterceptor blocking = new BlockingInterceptor(); + EventProcessor processor = newProcessor(1000, 0, blocking); + FlagsmithLogger logger = mock(FlagsmithLogger.class); + processor.setLogger(logger); + + // The events API hangs: every batch stays in flight until it is released. + for (int i = 0; i < EventProcessor.MAX_IN_FLIGHT_BATCHES; i++) { + processor.trackEvent("purchase", "user-" + i, "1", null, null); + processor.flush(); + } + processor.trackEvent("purchase", "one-too-many", "1", null, null); + CompletableFuture all = processor.flush(); + + assertTrue(processor.bufferedEvents().isEmpty(), "the dropped batch stayed in the buffer"); + verify(logger).error(contains("Dropping 1 events")); + assertFalse(all.isDone()); + + blocking.release(); + all.get(WAIT_SECONDS, TimeUnit.SECONDS); + + assertEquals(EventProcessor.MAX_IN_FLIGHT_BATCHES, recorder.count()); + for (String body : recorder.bodies()) { + assertFalse(body.contains("one-too-many")); + } + + // Once the backlog clears, batches flow again. + processor.trackEvent("purchase", "after", "1", null, null); + flushAndWait(processor); + assertEquals(EventProcessor.MAX_IN_FLIGHT_BATCHES + 1, recorder.count()); + } + + @Test + @SneakyThrows + public void flush_logsEventsTheApiRejects() { + EventProcessor processor = newProcessor(1000, 0); + FlagsmithLogger logger = mock(FlagsmithLogger.class); + processor.setLogger(logger); + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond( + "{\"accepted\": 1, \"rejected\": [{\"index\": 1, \"error\": \"event too long\"}]}", + MEDIATYPE_JSON); + + processor.trackEvent("purchase", "user-1", "1", null, null); + processor.trackEvent("purchase", "user-2", "2", null, null); + flushAndWait(processor); + + verify(logger).error(contains("rejected 1 of 2 events")); + verify(logger).error(contains("event too long")); + } + + @Test + @SneakyThrows + public void flush_logsNothingWhenEveryEventIsAccepted() { + EventProcessor processor = newProcessor(1000, 0); + FlagsmithLogger logger = mock(FlagsmithLogger.class); + processor.setLogger(logger); + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(ACCEPTED_BODY, MEDIATYPE_JSON); + + processor.trackEvent("purchase", "user-1", "1", null, null); + flushAndWait(processor); + + assertEquals(1, recorder.count()); + verify(logger, never()).error(any()); + verify(logger, never()).error(any(), any()); + } + + /** + * Holds every request until released, like an events API that has stopped answering, then + * accepts it. It answers itself rather than deferring to the MockInterceptor, whose canned + * response bodies share one buffer and break under concurrent calls. + */ + private static class BlockingInterceptor implements Interceptor { + + private final CountDownLatch released = new CountDownLatch(1); + + void release() { + released.countDown(); + } + + @Override + public Response intercept(Chain chain) throws IOException { + try { + released.await(WAIT_SECONDS, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException(e); + } + return new Response.Builder() + .request(chain.request()) + .protocol(Protocol.HTTP_1_1) + .code(202) + .message("Accepted") + .body(ResponseBody.create(ACCEPTED_BODY, MediaType.get("application/json"))) + .build(); + } + } + /** Records every request that reaches the network, with its body. */ private static class RecordingInterceptor implements Interceptor { From a717dc6ede1e45eb6091df2efa859eb427fceed8 Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 23 Sep 2026 09:55:44 +0200 Subject: [PATCH 07/16] fix: close the event processor's shutdown and startup races - An event whose closed-check ran before close() could still land in the buffer after close()'s final flush and be lost unlogged. The check is repeated under the buffer lock, which the final flush takes after the flag is set. - start() on a closed processor threw RejectedExecutionException out of FlagsmithClient.Builder.build(), which happens when a FlagsmithConfig is reused after closing a client built from it. It now logs and does nothing. - build() started the flush timer before its local-evaluation checks, so a build that then failed left the timer running with no client to close it. The processor is now wired last; the offline-mode check moves up with the other offline checks. --- .../java/com/flagsmith/FlagsmithClient.java | 20 ++++++------ .../com/flagsmith/threads/EventProcessor.java | 32 ++++++++++++++++--- .../com/flagsmith/FlagsmithClientTest.java | 17 ++++++++++ .../flagsmith/threads/EventProcessorTest.java | 14 ++++++++ 4 files changed, 69 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/flagsmith/FlagsmithClient.java b/src/main/java/com/flagsmith/FlagsmithClient.java index 4dd259fe..632969a7 100644 --- a/src/main/java/com/flagsmith/FlagsmithClient.java +++ b/src/main/java/com/flagsmith/FlagsmithClient.java @@ -692,6 +692,9 @@ public FlagsmithClient build() { if (configuration.getOfflineHandler() == null) { throw new FlagsmithRuntimeError("Offline handler must be provided to use offline mode."); } + if (configuration.getEventProcessor() != null) { + throw new FlagsmithRuntimeError("Events cannot be enabled in offline mode."); + } } if (this.flagsmithApiWrapper != null) { @@ -716,15 +719,6 @@ public FlagsmithClient build() { configuration.getAnalyticsProcessor().setLogger(client.logger); } - if (configuration.getEventProcessor() != null) { - if (configuration.getOfflineMode()) { - throw new FlagsmithRuntimeError("Events cannot be enabled in offline mode."); - } - configuration.getEventProcessor().setApi(client.flagsmithSdk); - configuration.getEventProcessor().setLogger(client.logger); - configuration.getEventProcessor().start(); - } - if (configuration.getEnableLocalEvaluation()) { if (configuration.getOfflineHandler() != null) { throw new FlagsmithRuntimeError( @@ -757,6 +751,14 @@ public FlagsmithClient build() { configuration.getOfflineHandler().getEnvironment()); } + // Last, once nothing else can throw: starting the processor starts its flush timer, which a + // failed build would otherwise leave running with no client to close it. + if (configuration.getEventProcessor() != null) { + configuration.getEventProcessor().setApi(client.flagsmithSdk); + configuration.getEventProcessor().setLogger(client.logger); + configuration.getEventProcessor().start(); + } + return this.client; } } diff --git a/src/main/java/com/flagsmith/threads/EventProcessor.java b/src/main/java/com/flagsmith/threads/EventProcessor.java index 4975424b..43497943 100644 --- a/src/main/java/com/flagsmith/threads/EventProcessor.java +++ b/src/main/java/com/flagsmith/threads/EventProcessor.java @@ -214,13 +214,22 @@ public CompletableFuture flush() { } /** - * Start the flush timer. Does nothing when the flush interval is not positive. + * Start the flush timer. Does nothing when the flush interval is not positive, when the timer + * is already running, or once the processor is closed. */ - public void start() { + public synchronized void start() { if (flushIntervalMillis <= 0 || scheduledFlush != null) { return; } + if (closed.get()) { + // The scheduler is shut down, and scheduling on it would throw. This happens when a + // FlagsmithConfig, which owns the processor, is reused after a client built from it was + // closed. + logger.error("Not starting the event processor: it has been closed."); + return; + } + scheduledFlush = scheduler.scheduleWithFixedDelay( this::flush, flushIntervalMillis, flushIntervalMillis, TimeUnit.MILLISECONDS); } @@ -230,8 +239,10 @@ public void start() { * resources. */ public void close() { - closed.set(true); - scheduler.shutdownNow(); + synchronized (this) { + closed.set(true); + scheduler.shutdownNow(); + } try { flush().get(requestTimeoutMillis * 2L, TimeUnit.MILLISECONDS); @@ -248,7 +259,7 @@ public void close() { private void bufferEvent(String event, String featureName, String identifier, Object value, Map traits, Map metadata, boolean dedupe) { if (closed.get()) { - logger.info("Not buffering event {}: the event processor is closed.", event); + logClosed(event); return; } @@ -281,6 +292,13 @@ private void bufferEvent(String event, String featureName, String identifier, Ob boolean isFull; synchronized (lock) { + // Checked again under the lock: close() sets the flag before its final flush takes the + // lock, so an event either lands in the buffer ahead of that flush or is refused here. + // Checking only above would let an event slip in after the final flush and be lost. + if (closed.get()) { + logClosed(event); + return; + } if (dedupe && !dedupeKeys.add( dedupeKey(event, featureName, identifier, stringValue, experimentId))) { return; @@ -297,6 +315,10 @@ private void bufferEvent(String event, String featureName, String identifier, Ob } } + private void logClosed(String event) { + logger.info("Not buffering event {}: the event processor is closed.", event); + } + /** * Flatten a caller trait map into the flat map of trait values the events API expects. Values * wrapped in a {@link TraitConfig} are unwrapped, and traits the caller marked transient are diff --git a/src/test/java/com/flagsmith/FlagsmithClientTest.java b/src/test/java/com/flagsmith/FlagsmithClientTest.java index 198dd4e6..1bee02a8 100644 --- a/src/test/java/com/flagsmith/FlagsmithClientTest.java +++ b/src/test/java/com/flagsmith/FlagsmithClientTest.java @@ -1113,6 +1113,23 @@ public void testEventsInOfflineModeThrowsAtBuild() { assertEquals("Events cannot be enabled in offline mode.", ex.getMessage()); } + @Test + public void testFailedBuildDoesNotStartTheEventProcessor() { + EventProcessor processor = mock(EventProcessor.class); + FlagsmithConfig config = FlagsmithConfig.newBuilder() + .withLocalEvaluation(true) + .withEventProcessor(processor) + .build(); + + FlagsmithClient.Builder clientBuilder = FlagsmithClient.newBuilder() + .withConfiguration(config) + // Local evaluation needs a server key, so this build fails. + .setApiKey("api-key"); + + assertThrows(FlagsmithRuntimeError.class, clientBuilder::build); + verify(processor, never()).start(); + } + @Test public void testEventApisThrowWhenEventsAreDisabled() { FlagsmithClient client = FlagsmithClient.newBuilder().setApiKey("api-key").build(); diff --git a/src/test/java/com/flagsmith/threads/EventProcessorTest.java b/src/test/java/com/flagsmith/threads/EventProcessorTest.java index 4edd9a27..abb74e58 100644 --- a/src/test/java/com/flagsmith/threads/EventProcessorTest.java +++ b/src/test/java/com/flagsmith/threads/EventProcessorTest.java @@ -532,6 +532,20 @@ public void close_flushesAndStopsTheSchedulerThread() { assertTrue(processor.getScheduler().isTerminated()); } + @Test + @SneakyThrows + public void start_isANoOpAfterClose() { + EventProcessor processor = newProcessor(1000, 100); + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(ACCEPTED_BODY, MEDIATYPE_JSON); + processor.close(); + eventProcessor = null; + + // Scheduling on the shut-down scheduler would throw RejectedExecutionException. + processor.start(); + + assertTrue(processor.getScheduler().isShutdown()); + } + @Test public void trackEvent_neverThrowsWhenTheApiIsMissing() { EventProcessor processor = newProcessor(1, 0); From 9b9118527b4ec8ac9843c09048dc9714ce7c55d4 Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 23 Sep 2026 09:57:05 +0200 Subject: [PATCH 08/16] fix: reject blank event names and unbounded event buffer settings - trackEvent buffered a null or blank event name, which the events API rejects; it now throws IllegalArgumentException, like the reserved '$' prefix already did. trackExposureEvent does the same for a blank feature name. A blank identifier is still logged and skipped, since an anonymous visitor is an ordinary runtime case, not a caller bug. - withEventsMaxBufferItems(0) switched off the size trigger, and with the timer also off the buffer grew without bound. build() now rejects a limit below 1 and a negative flush interval. - withEnableEvents(null) threw a NullPointerException from build(); it now leaves events disabled. --- .../java/com/flagsmith/FlagsmithClient.java | 22 +++++-- .../com/flagsmith/config/FlagsmithConfig.java | 7 +- .../com/flagsmith/threads/EventProcessor.java | 15 ++++- .../com/flagsmith/FlagsmithClientTest.java | 66 +++++++++++++++++++ 4 files changed, 99 insertions(+), 11 deletions(-) diff --git a/src/main/java/com/flagsmith/FlagsmithClient.java b/src/main/java/com/flagsmith/FlagsmithClient.java index 632969a7..999e6f17 100644 --- a/src/main/java/com/flagsmith/FlagsmithClient.java +++ b/src/main/java/com/flagsmith/FlagsmithClient.java @@ -272,7 +272,7 @@ public BaseFlag getExperimentFlag( * * @param event event name * @throws FlagsmithRuntimeError when events are not enabled - * @throws IllegalArgumentException when the event name starts with "$" + * @throws IllegalArgumentException when the event name is blank or starts with "$" */ public void trackEvent(String event) { trackEvent(event, null, null, null, null); @@ -284,7 +284,7 @@ public void trackEvent(String event) { * @param event event name * @param identifier identifier string * @throws FlagsmithRuntimeError when events are not enabled - * @throws IllegalArgumentException when the event name starts with "$" + * @throws IllegalArgumentException when the event name is blank or starts with "$" */ public void trackEvent(String event, String identifier) { trackEvent(event, identifier, null, null, null); @@ -299,13 +299,16 @@ public void trackEvent(String event, String identifier) { * @param traits a map of trait keys to trait values * @param metadata a map of metadata to attach to the event * @throws FlagsmithRuntimeError when events are not enabled - * @throws IllegalArgumentException when the event name starts with "$" + * @throws IllegalArgumentException when the event name is blank or starts with "$" */ public void trackEvent(String event, String identifier, Object value, Map traits, Map metadata) { EventProcessor processor = requireEventProcessor("track events"); - if (event != null && event.startsWith("$")) { + if (StringUtils.isBlank(event)) { + throw new IllegalArgumentException("An event name is required."); + } + if (event.startsWith("$")) { throw new IllegalArgumentException("Event names starting with \"$\" are reserved; use " + "trackExposureEvent to record \"" + EventProcessor.FLAG_EXPOSURE_EVENT + "\"."); } @@ -320,7 +323,8 @@ public void trackEvent(String event, String identifier, Object value, * @param featureName feature the identity was exposed to * @param identifier identifier string * @param value variant the identity was bucketed into - * @throws FlagsmithRuntimeError when events are not enabled + * @throws FlagsmithRuntimeError when events are not enabled + * @throws IllegalArgumentException when the feature name is blank */ public void trackExposureEvent(String featureName, String identifier, Object value) { trackExposureEvent(featureName, identifier, value, null, null); @@ -335,12 +339,18 @@ public void trackExposureEvent(String featureName, String identifier, Object val * @param value variant the identity was bucketed into * @param traits a map of trait keys to trait values * @param metadata a map of metadata to attach to the event - * @throws FlagsmithRuntimeError when events are not enabled + * @throws FlagsmithRuntimeError when events are not enabled + * @throws IllegalArgumentException when the feature name is blank */ public void trackExposureEvent(String featureName, String identifier, Object value, Map traits, Map metadata) { EventProcessor processor = requireEventProcessor("track exposure events"); + // A missing feature name is a bug in the caller, and the events API rejects the exposure. A + // missing identifier is ordinary at runtime (an anonymous visitor), so it is logged instead. + if (StringUtils.isBlank(featureName)) { + throw new IllegalArgumentException("An exposure requires a feature name."); + } if (StringUtils.isBlank(identifier)) { logger.info("Not sending {} for feature {}: an exposure requires an identifier.", EventProcessor.FLAG_EXPOSURE_EVENT, featureName); diff --git a/src/main/java/com/flagsmith/config/FlagsmithConfig.java b/src/main/java/com/flagsmith/config/FlagsmithConfig.java index 9eeecbd5..02fcda63 100644 --- a/src/main/java/com/flagsmith/config/FlagsmithConfig.java +++ b/src/main/java/com/flagsmith/config/FlagsmithConfig.java @@ -97,7 +97,7 @@ protected FlagsmithConfig(Builder builder) { this.eventsUri = builder.eventsUri; - if (builder.enableEvents) { + if (Boolean.TRUE.equals(builder.enableEvents)) { eventProcessor = builder.eventProcessor != null ? builder.eventProcessor : new EventProcessor(httpClient, eventsUri, builder.eventsMaxBufferItems, @@ -338,7 +338,7 @@ public Builder withEventProcessor(EventProcessor processor) { /** * Set the number of buffered events that triggers an immediate flush. Requires events to be - * enabled. + * enabled; {@link #build()} throws IllegalArgumentException when it is below 1. * * @param items the maximum number of buffered events * @return the Builder @@ -351,7 +351,8 @@ public Builder withEventsMaxBufferItems(int items) { /** * Set the interval between timed event flushes, in milliseconds. Zero disables the timer. - * Requires events to be enabled. + * Requires events to be enabled; {@link #build()} throws IllegalArgumentException when it is + * negative. * * @param millis the flush interval in milliseconds * @return the Builder diff --git a/src/main/java/com/flagsmith/threads/EventProcessor.java b/src/main/java/com/flagsmith/threads/EventProcessor.java index 43497943..a5ff2203 100644 --- a/src/main/java/com/flagsmith/threads/EventProcessor.java +++ b/src/main/java/com/flagsmith/threads/EventProcessor.java @@ -96,10 +96,13 @@ public class EventProcessor { * * @param client client instance * @param eventsUri base URI of the events API, e.g. https://events.api.flagsmith.com/ - * @param maxBufferItems number of buffered events that triggers an immediate flush + * @param maxBufferItems number of buffered events that triggers an immediate flush; at + * least 1 * @param flushIntervalMillis interval between timed flushes; 0 disables the timer * @param requestTimeoutMillis how long a single POST is expected to take; {@link #close()} * waits up to twice this for in-flight batches + * @throws IllegalArgumentException when maxBufferItems is below 1 or flushIntervalMillis is + * negative */ public EventProcessor(OkHttpClient client, HttpUrl eventsUri, int maxBufferItems, int flushIntervalMillis, int requestTimeoutMillis) { @@ -113,6 +116,14 @@ public EventProcessor(OkHttpClient client, HttpUrl eventsUri, int maxBufferItems */ EventProcessor(HttpUrl eventsUri, int maxBufferItems, int flushIntervalMillis, int requestTimeoutMillis, RequestProcessor requestProcessor) { + // Without a positive buffer limit nothing bounds the buffer between timed flushes, and with + // the timer off as well it would grow for as long as the process runs. + if (maxBufferItems < 1) { + throw new IllegalArgumentException("maxBufferItems must be at least 1."); + } + if (flushIntervalMillis < 0) { + throw new IllegalArgumentException("flushIntervalMillis must not be negative."); + } this.eventsEndpoint = eventsUri.newBuilder(EVENTS_PATH).build(); this.maxBufferItems = maxBufferItems; this.flushIntervalMillis = flushIntervalMillis; @@ -304,7 +315,7 @@ private void bufferEvent(String event, String featureName, String identifier, Ob return; } buffer.add(eventPayload); - isFull = maxBufferItems > 0 && buffer.size() >= maxBufferItems; + isFull = buffer.size() >= maxBufferItems; } if (isFull) { diff --git a/src/test/java/com/flagsmith/FlagsmithClientTest.java b/src/test/java/com/flagsmith/FlagsmithClientTest.java index 1bee02a8..62d0449b 100644 --- a/src/test/java/com/flagsmith/FlagsmithClientTest.java +++ b/src/test/java/com/flagsmith/FlagsmithClientTest.java @@ -1156,6 +1156,72 @@ public void testTrackEventRejectsReservedEventNames() { verify(processor, never()).trackEvent(any(), any(), any(), any(), any()); } + @Test + public void testTrackEventRejectsBlankEventNames() { + EventProcessor processor = mock(EventProcessor.class); + FlagsmithClient client = FlagsmithClient.newBuilder() + .withConfiguration(FlagsmithConfig.newBuilder() + .withEventProcessor(processor) + .build()) + .setApiKey("api-key") + .build(); + + assertThrows(IllegalArgumentException.class, () -> client.trackEvent(null)); + assertThrows(IllegalArgumentException.class, () -> client.trackEvent(" ", "user-1")); + verify(processor, never()).trackEvent(any(), any(), any(), any(), any()); + } + + @Test + public void testTrackExposureEventRejectsBlankFeatureNames() { + EventProcessor processor = mock(EventProcessor.class); + FlagsmithClient client = FlagsmithClient.newBuilder() + .withConfiguration(FlagsmithConfig.newBuilder() + .withEventProcessor(processor) + .build()) + .setApiKey("api-key") + .build(); + + assertThrows(IllegalArgumentException.class, + () -> client.trackExposureEvent(null, "user-1", "treatment")); + assertThrows(IllegalArgumentException.class, + () -> client.trackExposureEvent("", "user-1", "treatment")); + verify(processor, never()).trackExposureEvent(any(), any(), any(), any(), any()); + } + + @Test + public void testInvalidEventProcessorSettingsThrowAtBuild() { + assertThrows(IllegalArgumentException.class, () -> FlagsmithConfig.newBuilder() + .withEnableEvents(Boolean.TRUE) + .withEventsMaxBufferItems(0) + .build()); + assertThrows(IllegalArgumentException.class, () -> FlagsmithConfig.newBuilder() + .withEnableEvents(Boolean.TRUE) + .withEventsFlushIntervalMillis(-1) + .build()); + } + + @Test + public void testEventsSettingsReachTheProcessor() { + FlagsmithConfig config = FlagsmithConfig.newBuilder() + .eventsUri("http://events-uri") + .withEnableEvents(Boolean.TRUE) + .withEventsMaxBufferItems(5) + .withEventsFlushIntervalMillis(0) + .build(); + + EventProcessor processor = config.getEventProcessor(); + assertEquals("http://events-uri/v1/events", processor.getEventsEndpoint().toString()); + assertEquals(5, processor.getMaxBufferItems()); + assertEquals(0, processor.getFlushIntervalMillis()); + } + + @Test + public void testNullEnableEventsLeavesEventsDisabled() { + FlagsmithConfig config = FlagsmithConfig.newBuilder().withEnableEvents(null).build(); + + assertNull(config.getEventProcessor()); + } + @Test public void testTrackExposureEventWithBlankIdentifierSendsNothing() { EventProcessor processor = mock(EventProcessor.class); From ae482c1c278e9fd398e4d066090f38f17c78dd88 Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 23 Sep 2026 15:04:21 +0200 Subject: [PATCH 09/16] fix: serve the default flag when identity flags time out in getExperimentFlag FlagsmithApiWrapper.identifyUserWithTraits returns null, rather than throwing, when the identities request times out or is interrupted. getExperimentFlag dereferenced that null, so an API slower than the 15s future timeout surfaced as a NullPointerException and bypassed the default flag handler. A null result now returns the default handler's flag, with no exposure recorded, and throws FlagsmithApiError when no handler is configured. --- .../java/com/flagsmith/FlagsmithClient.java | 17 +++++++ .../com/flagsmith/FlagsmithClientTest.java | 50 +++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/src/main/java/com/flagsmith/FlagsmithClient.java b/src/main/java/com/flagsmith/FlagsmithClient.java index 999e6f17..851aec68 100644 --- a/src/main/java/com/flagsmith/FlagsmithClient.java +++ b/src/main/java/com/flagsmith/FlagsmithClient.java @@ -214,6 +214,8 @@ public List getIdentitySegments(String identifier, Map * @param identifier identifier string * @return the flag for the given feature * @throws FlagsmithRuntimeError when events are not enabled + * @throws FlagsmithApiError when identity flags are unavailable and no default flag handler + * is configured */ public BaseFlag getExperimentFlag(String featureName, String identifier) throws FlagsmithClientError { @@ -233,6 +235,8 @@ public BaseFlag getExperimentFlag(String featureName, String identifier) * @param traits a map of trait keys to trait values * @return the flag for the given feature * @throws FlagsmithRuntimeError when events are not enabled + * @throws FlagsmithApiError when identity flags are unavailable and no default flag handler + * is configured */ public BaseFlag getExperimentFlag( String featureName, String identifier, Map traits) @@ -240,6 +244,19 @@ public BaseFlag getExperimentFlag( requireEventProcessor("get experiment flags"); Flags flags = getIdentityFlags(identifier, traits); + + if (flags == null) { + // The API wrapper returns null rather than throwing when the identities request times out + // or is interrupted. Serve the default flag, as getFlag would with the API unavailable. + FlagsmithFlagDefaults defaults = getConfig().getFlagsmithFlagDefaults(); + if (defaults == null) { + throw new FlagsmithApiError("Failed to get feature flags."); + } + logger.info("Not recording an exposure for feature {}: identity flags are unavailable, so " + + "the default flag handler served it.", featureName); + return defaults.evaluateDefaultFlag(featureName); + } + BaseFlag flag = flags.getFlag(featureName); if (!(flag instanceof Flag)) { diff --git a/src/test/java/com/flagsmith/FlagsmithClientTest.java b/src/test/java/com/flagsmith/FlagsmithClientTest.java index 62d0449b..ada3806f 100644 --- a/src/test/java/com/flagsmith/FlagsmithClientTest.java +++ b/src/test/java/com/flagsmith/FlagsmithClientTest.java @@ -1113,6 +1113,56 @@ public void testEventsInOfflineModeThrowsAtBuild() { assertEquals("Events cannot be enabled in offline mode.", ex.getMessage()); } + /** + * A client whose API wrapper returns null identity flags, as FlagsmithApiWrapper does when + * the identities request times out or is interrupted. + */ + private static FlagsmithClient clientWithUnavailableIdentityFlags( + FlagsmithConfig config) { + FlagsmithApiWrapper mockApiWrapper = mock(FlagsmithApiWrapper.class); + when(mockApiWrapper.getConfig()).thenReturn(config); + when(mockApiWrapper.identifyUserWithTraits(any(), any(), anyBoolean(), anyBoolean())) + .thenReturn(null); + + return FlagsmithClient.newBuilder() + .withFlagsmithApiWrapper(mockApiWrapper) + .withConfiguration(config) + .setApiKey("api-key") + .build(); + } + + @Test + public void testGetExperimentFlagServesTheDefaultWhenIdentityFlagsAreUnavailable() + throws FlagsmithClientError { + EventProcessor processor = mock(EventProcessor.class); + FlagsmithConfig config = FlagsmithConfig.newBuilder() + .withEventProcessor(processor) + .build(); + FlagsmithFlagDefaults defaults = new FlagsmithFlagDefaults(); + defaults.setDefaultFlagValueFunc(FlagsmithClientTest::defaultHandler); + config.setFlagsmithFlagDefaults(defaults); + FlagsmithClient client = clientWithUnavailableIdentityFlags(config); + + BaseFlag flag = client.getExperimentFlag("checkout_cta", "user-1"); + + assertTrue(flag instanceof DefaultFlag); + assertEquals(DEFAULT_FLAG_VALUE, flag.getValue()); + verify(processor, never()).trackExposureEvent(any(), any(), any(), any(), any()); + } + + @Test + public void testGetExperimentFlagThrowsWhenIdentityFlagsAreUnavailableWithoutADefault() { + EventProcessor processor = mock(EventProcessor.class); + FlagsmithConfig config = FlagsmithConfig.newBuilder() + .withEventProcessor(processor) + .build(); + FlagsmithClient client = clientWithUnavailableIdentityFlags(config); + + assertThrows(FlagsmithApiError.class, + () -> client.getExperimentFlag("checkout_cta", "user-1")); + verify(processor, never()).trackExposureEvent(any(), any(), any(), any(), any()); + } + @Test public void testFailedBuildDoesNotStartTheEventProcessor() { EventProcessor processor = mock(EventProcessor.class); From 07c24727c577e1bcf5c758e468ee348ecc2998a2 Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 23 Sep 2026 15:07:04 +0200 Subject: [PATCH 10/16] fix: cap in-flight events, not batches, and rate-limit the drop log Capping in-flight batches over a fixed three-thread pool made the limit a throughput ceiling of about 3 x maxBufferItems per round trip, which throttled hardest the smaller the configured buffer: a healthy API dropped most events at a small buffer size, and even at defaults a burst dropped two thirds. The cap now counts events (10,000, ten default batches), tracked under the buffer lock and given back when a batch settles. The memory bound at defaults is unchanged, and throughput no longer depends on buffer size. Drops are reported at once, then at most every ten seconds with the count accumulated in between, so a saturated caller cannot emit an error line per flush. The completion callback also captured the whole batch list just for its size, keeping a second copy of every in-flight batch alive; it now captures the int. --- .../com/flagsmith/threads/EventProcessor.java | 72 ++++++++++--- .../flagsmith/threads/EventProcessorTest.java | 102 +++++++++++++++--- 2 files changed, 141 insertions(+), 33 deletions(-) diff --git a/src/main/java/com/flagsmith/threads/EventProcessor.java b/src/main/java/com/flagsmith/threads/EventProcessor.java index a5ff2203..c092cba9 100644 --- a/src/main/java/com/flagsmith/threads/EventProcessor.java +++ b/src/main/java/com/flagsmith/threads/EventProcessor.java @@ -55,12 +55,19 @@ public class EventProcessor { private static final MediaType JSON_MEDIA_TYPE = MediaType.get("application/json; charset=utf-8"); /** - * The most batches that may be waiting on the events API at once. Beyond it a flush drops its - * batch instead of queueing it: while the API is slow or down, batches are produced faster + * The most events that may be waiting on the events API at once. Once reached, a flush drops + * its batch instead of queueing it: while the API is slow or down, batches are produced faster * than they are retried and given up on, and an unbounded queue of them would grow with the * host application's traffic until it ran out of memory. + * + *

It counts events, not batches, so that it bounds memory without capping throughput on a + * healthy API: a batch count would throttle harder the smaller the configured buffer. At the + * default buffer size it is ten batches. A batch is admitted while the count is below the + * limit, so the bound is this plus one buffer's worth. */ - static final int MAX_IN_FLIGHT_BATCHES = 10; + static final int MAX_IN_FLIGHT_EVENTS = 10_000; + /** The least time between two error lines reporting dropped events. */ + private static final long DROP_LOG_INTERVAL_NANOS = TimeUnit.SECONDS.toNanos(10); /** The URL batches are POSTed to. */ @Getter @@ -82,6 +89,9 @@ public class EventProcessor { @Getter(AccessLevel.PACKAGE) private final ScheduledExecutorService scheduler; private final Set> inFlight = ConcurrentHashMap.newKeySet(); + private int inFlightEvents = 0; // guarded by lock + private int droppedSinceLastReport = 0; // guarded by lock + private Long lastDropReportNanos = null; // guarded by lock @Getter(AccessLevel.PACKAGE) private final RequestProcessor requestProcessor; /** The API wrapper used to build requests; injected by {@code FlagsmithClient.Builder}. */ @@ -194,27 +204,29 @@ public void trackExposureEvent(String featureName, String identifier, Object val public CompletableFuture flush() { List> batch = null; CompletableFuture tracked = null; - int dropped = 0; + int droppedToReport = 0; // The batch is registered as in-flight under the same lock that empties the buffer, so a // concurrent flush() can never observe both an empty buffer and an unregistered batch. synchronized (lock) { if (!buffer.isEmpty()) { - if (inFlight.size() >= MAX_IN_FLIGHT_BATCHES) { - dropped = buffer.size(); + if (inFlightEvents >= MAX_IN_FLIGHT_EVENTS) { + droppedToReport = recordDrop(buffer.size()); } else { batch = new ArrayList<>(buffer); tracked = new CompletableFuture<>(); inFlight.add(tracked); + inFlightEvents += batch.size(); } buffer.clear(); } dedupeKeys.clear(); } - if (dropped > 0) { - logger.error("Dropping " + dropped + " events: " + MAX_IN_FLIGHT_BATCHES - + " earlier batches are still waiting on the events API."); + if (droppedToReport > 0) { + logger.error("Dropped " + droppedToReport + " events: at least " + MAX_IN_FLIGHT_EVENTS + + " earlier events are still waiting on the events API. Further drops are reported at" + + " most every " + TimeUnit.NANOSECONDS.toSeconds(DROP_LOG_INTERVAL_NANOS) + "s."); } if (batch != null) { @@ -373,11 +385,14 @@ private static String nullToEmpty(String value) { * it. */ private void send(List> batch, CompletableFuture tracked) { + // The completion callback outlives this call by as long as the POST takes, so it captures + // the size rather than the list: the serialised request already holds the batch's content. + final int batchSize = batch.size(); boolean submitted = false; try { if (api == null) { - logger.error("Dropping " + batch.size() + logger.error("Dropping " + batchSize + " events: the event processor has no API wrapper."); return; } @@ -395,17 +410,17 @@ private void send(List> batch, CompletableFuture track .submit(request, new TypeReference() {}, Boolean.FALSE, buildRetry()) .whenComplete((response, error) -> { try { - logRejections(response, batch.size()); + logRejections(response, batchSize); } finally { - settle(tracked); + settle(tracked, batchSize); } }); submitted = true; } catch (Exception e) { - logger.error("Dropping " + batch.size() + " events: failed to send them.", e); + logger.error("Dropping " + batchSize + " events: failed to send them.", e); } finally { if (!submitted) { - settle(tracked); + settle(tracked, batchSize); } } } @@ -422,11 +437,36 @@ private void logRejections(JsonNode response, int batchSize) { } } - private void settle(CompletableFuture tracked) { - inFlight.remove(tracked); + private void settle(CompletableFuture tracked, int batchSize) { + // Idempotent: only the call that actually removes the batch gives its events back. + if (inFlight.remove(tracked)) { + synchronized (lock) { + inFlightEvents -= batchSize; + } + } tracked.complete(null); } + /** + * Count dropped events, and say whether it is time to report them. The first drop is reported + * at once; later ones accumulate and are reported at most once per + * {@link #DROP_LOG_INTERVAL_NANOS}, so a caller saturating the processor during an outage + * cannot turn every flush into an error line. Called under the lock. + * + * @return the number of drops to report now, or 0 to stay quiet + */ + private int recordDrop(int count) { + droppedSinceLastReport += count; + long now = System.nanoTime(); + if (lastDropReportNanos != null && now - lastDropReportNanos < DROP_LOG_INTERVAL_NANOS) { + return 0; + } + lastDropReportNanos = now; + int toReport = droppedSinceLastReport; + droppedSinceLastReport = 0; + return toReport; + } + private CompletableFuture awaitInFlight() { return CompletableFuture.allOf(inFlight.toArray(new CompletableFuture[0])); } diff --git a/src/test/java/com/flagsmith/threads/EventProcessorTest.java b/src/test/java/com/flagsmith/threads/EventProcessorTest.java index abb74e58..4e06a45a 100644 --- a/src/test/java/com/flagsmith/threads/EventProcessorTest.java +++ b/src/test/java/com/flagsmith/threads/EventProcessorTest.java @@ -8,10 +8,13 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.contains; +import static org.mockito.ArgumentMatchers.startsWith; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockingDetails; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -45,6 +48,7 @@ import okhttp3.ResponseBody; import okhttp3.mock.MockInterceptor; import okio.Buffer; +import org.mockito.invocation.Invocation; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -559,36 +563,89 @@ public void trackEvent_neverThrowsWhenTheApiIsMissing() { @Test @SneakyThrows - public void flush_dropsBatchesBeyondTheInFlightLimitInsteadOfQueueingThem() { - BlockingInterceptor blocking = new BlockingInterceptor(); - EventProcessor processor = newProcessor(1000, 0, blocking); + public void flush_dropsEventsBeyondTheInFlightLimitInsteadOfQueueingThem() { + AcceptingInterceptor eventsApi = AcceptingInterceptor.blocked(); + EventProcessor processor = newProcessor(1000, 0, eventsApi); FlagsmithLogger logger = mock(FlagsmithLogger.class); processor.setLogger(logger); - // The events API hangs: every batch stays in flight until it is released. - for (int i = 0; i < EventProcessor.MAX_IN_FLIGHT_BATCHES; i++) { + // The events API hangs, so every batch stays in flight until it is released. Each full + // buffer flushes itself, which puts exactly the limit in flight. + for (int i = 0; i < EventProcessor.MAX_IN_FLIGHT_EVENTS; i++) { processor.trackEvent("purchase", "user-" + i, "1", null, null); - processor.flush(); } processor.trackEvent("purchase", "one-too-many", "1", null, null); CompletableFuture all = processor.flush(); assertTrue(processor.bufferedEvents().isEmpty(), "the dropped batch stayed in the buffer"); - verify(logger).error(contains("Dropping 1 events")); + verify(logger).error(contains("Dropped 1 events")); assertFalse(all.isDone()); - blocking.release(); + // A caller saturating the processor does not get an error line per flush. + for (int i = 0; i < 100; i++) { + processor.trackEvent("purchase", "also-dropped-" + i, "1", null, null); + processor.flush(); + } + verify(logger, times(1)).error(startsWith("Dropped")); + + eventsApi.release(); all.get(WAIT_SECONDS, TimeUnit.SECONDS); - assertEquals(EventProcessor.MAX_IN_FLIGHT_BATCHES, recorder.count()); + assertEquals(EventProcessor.MAX_IN_FLIGHT_EVENTS, deliveredEvents()); for (String body : recorder.bodies()) { assertFalse(body.contains("one-too-many")); + assertFalse(body.contains("also-dropped")); } // Once the backlog clears, batches flow again. processor.trackEvent("purchase", "after", "1", null, null); flushAndWait(processor); - assertEquals(EventProcessor.MAX_IN_FLIGHT_BATCHES + 1, recorder.count()); + assertEquals(EventProcessor.MAX_IN_FLIGHT_EVENTS + 1, deliveredEvents()); + } + + @Test + @SneakyThrows + public void flush_neverDropsEventsOnAHealthyApiWithASmallBuffer() { + // A one-event buffer sends a batch per event. Capping batches rather than events throttled + // exactly this configuration, dropping most events even against an instant API. + EventProcessor processor = newProcessor(1, 0, AcceptingInterceptor.open()); + FlagsmithLogger logger = mock(FlagsmithLogger.class); + processor.setLogger(logger); + int events = 2000; + + for (int i = 0; i < events; i++) { + processor.trackEvent("purchase", "user-" + i, "1", null, null); + } + flushAndWait(processor); + + assertEquals(events, deliveredEvents()); + assertEquals(Collections.emptyList(), errorCalls(logger)); + } + + /** + * Every error-level call made on a mocked logger, whatever its arguments. Matching on the + * invocations rather than with verify(...) avoids Mockito's varargs matching, under which a + * matcher list silently misses calls with a different number of arguments. + */ + private static List errorCalls(FlagsmithLogger logger) { + List calls = new ArrayList<>(); + for (Invocation invocation : mockingDetails(logger).getInvocations()) { + String method = invocation.getMethod().getName(); + if (method.equals("error") || method.equals("httpError")) { + calls.add(invocation.toString()); + } + } + return calls; + } + + /** The number of events across every request the events API received. */ + @SneakyThrows + private int deliveredEvents() { + int delivered = 0; + for (String body : recorder.bodies()) { + delivered += MapperFactory.getMapper().readTree(body).get("events").size(); + } + return delivered; } @Test @@ -621,18 +678,29 @@ public void flush_logsNothingWhenEveryEventIsAccepted() { flushAndWait(processor); assertEquals(1, recorder.count()); - verify(logger, never()).error(any()); - verify(logger, never()).error(any(), any()); + assertEquals(Collections.emptyList(), errorCalls(logger)); } /** - * Holds every request until released, like an events API that has stopped answering, then - * accepts it. It answers itself rather than deferring to the MockInterceptor, whose canned - * response bodies share one buffer and break under concurrent calls. + * Accepts every request, optionally holding each one until released, like an events API that + * has stopped answering. It answers itself rather than deferring to the MockInterceptor, whose + * canned response bodies share one buffer and break under concurrent calls. */ - private static class BlockingInterceptor implements Interceptor { + private static class AcceptingInterceptor implements Interceptor { + + private final CountDownLatch released; + + private AcceptingInterceptor(boolean blocked) { + this.released = new CountDownLatch(blocked ? 1 : 0); + } + + static AcceptingInterceptor blocked() { + return new AcceptingInterceptor(true); + } - private final CountDownLatch released = new CountDownLatch(1); + static AcceptingInterceptor open() { + return new AcceptingInterceptor(false); + } void release() { released.countDown(); From 59723839004bbb86e6aa99ced91db8c498e3f17c Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 23 Sep 2026 15:09:44 +0200 Subject: [PATCH 11/16] fix: bound close() by the worst case of a batch under the real timeouts close() waited requestTimeoutMillis x 2, and FlagsmithConfig always passed the SDK's default read timeout, ignoring the one the caller configured. Even at defaults the wait was 10s against a worst case of about 24s for one batch (connect + write + read, twice, plus backoff), so the final batch was routinely abandoned during an outage. The processor now derives the wait from its HTTP client: the call timeout when set, otherwise connect + write + read, for every attempt the retry policy allows, plus the backoff between them. With a timeout switched off nothing bounds a request, and close() waits as long as it does. The unreleased requestTimeoutMillis constructor parameter and getter go, since the client already carries the timeouts. The request processor is still shut down, not interrupted: interrupting a POST loses its batch, where letting it finish delivers it. --- .../com/flagsmith/config/FlagsmithConfig.java | 2 +- .../com/flagsmith/threads/EventProcessor.java | 68 +++++++++++++--- .../flagsmith/threads/EventProcessorTest.java | 81 ++++++++++++++++++- 3 files changed, 138 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/flagsmith/config/FlagsmithConfig.java b/src/main/java/com/flagsmith/config/FlagsmithConfig.java index 02fcda63..0c6f58a5 100644 --- a/src/main/java/com/flagsmith/config/FlagsmithConfig.java +++ b/src/main/java/com/flagsmith/config/FlagsmithConfig.java @@ -101,7 +101,7 @@ protected FlagsmithConfig(Builder builder) { eventProcessor = builder.eventProcessor != null ? builder.eventProcessor : new EventProcessor(httpClient, eventsUri, builder.eventsMaxBufferItems, - builder.eventsFlushIntervalMillis, DEFAULT_READ_TIMEOUT_MILLIS); + builder.eventsFlushIntervalMillis); } else if (builder.eventsConfigured) { throw new IllegalArgumentException( "Events must be enabled with withEnableEvents(true) to configure the event processor."); diff --git a/src/main/java/com/flagsmith/threads/EventProcessor.java b/src/main/java/com/flagsmith/threads/EventProcessor.java index c092cba9..c861ca14 100644 --- a/src/main/java/com/flagsmith/threads/EventProcessor.java +++ b/src/main/java/com/flagsmith/threads/EventProcessor.java @@ -24,6 +24,7 @@ import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import lombok.AccessLevel; import lombok.Getter; @@ -66,6 +67,8 @@ public class EventProcessor { * limit, so the bound is this plus one buffer's worth. */ static final int MAX_IN_FLIGHT_EVENTS = 10_000; + /** The close timeout when the HTTP client's timeouts do not bound a request. */ + static final long UNBOUNDED = -1L; /** The least time between two error lines reporting dropped events. */ private static final long DROP_LOG_INTERVAL_NANOS = TimeUnit.SECONDS.toNanos(10); @@ -78,9 +81,6 @@ public class EventProcessor { /** The interval between timed flushes; 0 means there is no timer. */ @Getter private final int flushIntervalMillis; - /** How long a single POST is expected to take; {@link #close()} waits up to twice this. */ - @Getter - private final int requestTimeoutMillis; // Everything below is internal state, deliberately not exposed: once published, a getter // would be public API for as long as the SDK is supported. private final List> buffer = new ArrayList<>(); @@ -94,6 +94,12 @@ public class EventProcessor { private Long lastDropReportNanos = null; // guarded by lock @Getter(AccessLevel.PACKAGE) private final RequestProcessor requestProcessor; + /** + * How long {@link #close()} waits for in-flight batches: the worst case of one batch under the + * HTTP client's timeouts and the retry policy, or {@link #UNBOUNDED} when a timeout is off. + */ + @Getter(AccessLevel.PACKAGE) + private final long closeTimeoutMillis; /** The API wrapper used to build requests; injected by {@code FlagsmithClient.Builder}. */ @Setter private FlagsmithSdk api; @@ -109,14 +115,12 @@ public class EventProcessor { * @param maxBufferItems number of buffered events that triggers an immediate flush; at * least 1 * @param flushIntervalMillis interval between timed flushes; 0 disables the timer - * @param requestTimeoutMillis how long a single POST is expected to take; {@link #close()} - * waits up to twice this for in-flight batches * @throws IllegalArgumentException when maxBufferItems is below 1 or flushIntervalMillis is * negative */ public EventProcessor(OkHttpClient client, HttpUrl eventsUri, int maxBufferItems, - int flushIntervalMillis, int requestTimeoutMillis) { - this(eventsUri, maxBufferItems, flushIntervalMillis, requestTimeoutMillis, + int flushIntervalMillis) { + this(eventsUri, maxBufferItems, flushIntervalMillis, new RequestProcessor(client, new FlagsmithLogger(), buildRetry())); } @@ -125,7 +129,7 @@ public EventProcessor(OkHttpClient client, HttpUrl eventsUri, int maxBufferItems * something callers should come to depend on. */ EventProcessor(HttpUrl eventsUri, int maxBufferItems, int flushIntervalMillis, - int requestTimeoutMillis, RequestProcessor requestProcessor) { + RequestProcessor requestProcessor) { // Without a positive buffer limit nothing bounds the buffer between timed flushes, and with // the timer off as well it would grow for as long as the process runs. if (maxBufferItems < 1) { @@ -137,8 +141,8 @@ public EventProcessor(OkHttpClient client, HttpUrl eventsUri, int maxBufferItems this.eventsEndpoint = eventsUri.newBuilder(EVENTS_PATH).build(); this.maxBufferItems = maxBufferItems; this.flushIntervalMillis = flushIntervalMillis; - this.requestTimeoutMillis = requestTimeoutMillis; this.requestProcessor = requestProcessor; + this.closeTimeoutMillis = worstCaseBatchMillis(requestProcessor.getClient(), buildRetry()); this.scheduler = Executors.newSingleThreadScheduledExecutor((runnable) -> { Thread thread = new Thread(runnable, "flagsmith-events"); thread.setDaemon(true); @@ -157,6 +161,36 @@ private static Retry buildRetry() { return retry; } + /** + * The longest one batch can take to be delivered or given up on: every attempt the retry + * policy allows, each running to the client's timeouts, plus the backoff between them. An + * attempt is bounded by the call timeout when one is set, and otherwise by connect, write and + * read in turn. When none of those bounds it, neither is the batch. + * + * @return the worst case in milliseconds, or {@link #UNBOUNDED} + */ + static long worstCaseBatchMillis(OkHttpClient client, Retry retry) { + long attemptMillis; + if (client.callTimeoutMillis() > 0) { + attemptMillis = client.callTimeoutMillis(); + } else if (client.connectTimeoutMillis() > 0 && client.writeTimeoutMillis() > 0 + && client.readTimeoutMillis() > 0) { + attemptMillis = (long) client.connectTimeoutMillis() + client.writeTimeoutMillis() + + client.readTimeoutMillis(); + } else { + return UNBOUNDED; + } + + // Walk a copy of the policy the way RequestProcessor does: back off, then attempt. + Retry walk = retry.toBuilder().build(); + long total = 0; + for (int attempt = 0; attempt < walk.getTotal(); attempt++) { + total += walk.calculateSleepTime() + attemptMillis; + walk.retryAttempted(); + } + return total; + } + /** * Set the logger used by the processor and by its request processor. * @@ -259,7 +293,9 @@ public synchronized void start() { /** * Stop the flush timer, ship whatever is left on a best-effort basis and release the HTTP - * resources. + * resources. Blocks until in-flight batches settle, for at most the worst case of one batch + * under the HTTP client's timeouts and the retry policy; with a timeout switched off there is + * no such bound, and it waits as long as the request does. */ public void close() { synchronized (this) { @@ -268,14 +304,24 @@ public void close() { } try { - flush().get(requestTimeoutMillis * 2L, TimeUnit.MILLISECONDS); + CompletableFuture remaining = flush(); + if (closeTimeoutMillis == UNBOUNDED) { + remaining.get(); + } else { + remaining.get(closeTimeoutMillis, TimeUnit.MILLISECONDS); + } } catch (InterruptedException e) { Thread.currentThread().interrupt(); logger.error("Interrupted while flushing events on close.", e); + } catch (TimeoutException e) { + logger.error("Stopped waiting for events to be delivered after " + closeTimeoutMillis + + "ms on close; batches still in flight carry on in the background."); } catch (Exception e) { logger.error("Failed to flush events on close.", e); } + // shutdown(), never shutdownNow(): interrupting a POST mid-flight loses its batch, where + // letting it finish delivers it if the JVM stays up long enough. requestProcessor.close(); } diff --git a/src/test/java/com/flagsmith/threads/EventProcessorTest.java b/src/test/java/com/flagsmith/threads/EventProcessorTest.java index 4e06a45a..f459c227 100644 --- a/src/test/java/com/flagsmith/threads/EventProcessorTest.java +++ b/src/test/java/com/flagsmith/threads/EventProcessorTest.java @@ -21,6 +21,8 @@ import com.fasterxml.jackson.databind.JsonNode; import com.flagsmith.FlagsmithLogger; import com.flagsmith.MapperFactory; +import com.flagsmith.config.FlagsmithConfig; +import com.flagsmith.config.Retry; import com.flagsmith.interfaces.FlagsmithSdk; import com.flagsmith.models.TraitConfig; import java.io.IOException; @@ -105,7 +107,6 @@ private EventProcessor newProcessor( HttpUrl.get(EVENTS_URI), maxBufferItems, flushIntervalMillis, - 3000, new RequestProcessor(client, new FlagsmithLogger())); eventProcessor.setApi(api); return eventProcessor; @@ -550,6 +551,84 @@ public void start_isANoOpAfterClose() { assertTrue(processor.getScheduler().isShutdown()); } + @Test + public void worstCaseBatchMillis_coversEveryAttemptAtTheClientTimeoutsPlusBackoff() { + OkHttpClient client = new OkHttpClient.Builder() + .connectTimeout(1000, TimeUnit.MILLISECONDS) + .writeTimeout(2000, TimeUnit.MILLISECONDS) + .readTimeout(3000, TimeUnit.MILLISECONDS) + .build(); + Retry retry = new Retry(2); + + // Two attempts of connect + write + read, and the 200ms backoff before the second. + assertEquals(2 * 6000 + 200, EventProcessor.worstCaseBatchMillis(client, retry)); + } + + @Test + public void worstCaseBatchMillis_prefersTheCallTimeout() { + OkHttpClient client = new OkHttpClient.Builder() + .callTimeout(4000, TimeUnit.MILLISECONDS) + .build(); + + assertEquals(2 * 4000 + 200, + EventProcessor.worstCaseBatchMillis(client, new Retry(2))); + } + + @Test + public void worstCaseBatchMillis_isUnboundedWhenATimeoutIsOff() { + OkHttpClient client = new OkHttpClient.Builder() + .readTimeout(0, TimeUnit.MILLISECONDS) + .build(); + + assertEquals(EventProcessor.UNBOUNDED, + EventProcessor.worstCaseBatchMillis(client, new Retry(2))); + } + + @Test + @SneakyThrows + public void close_stopsWaitingAtTheWorstCaseBound() { + // A call timeout of 100ms bounds a batch at 2 x 100ms + 200ms backoff. The hung API below + // ignores the cancellation, as a stuck interceptor or proxy would. + AcceptingInterceptor eventsApi = AcceptingInterceptor.blocked(); + OkHttpClient client = new OkHttpClient.Builder() + .callTimeout(100, TimeUnit.MILLISECONDS) + .addInterceptor(eventsApi) + .build(); + EventProcessor processor = new EventProcessor( + HttpUrl.get(EVENTS_URI), 1000, 0, new RequestProcessor(client, new FlagsmithLogger())); + processor.setApi(api); + FlagsmithLogger logger = mock(FlagsmithLogger.class); + processor.setLogger(logger); + assertEquals(400, processor.getCloseTimeoutMillis()); + + try { + processor.trackEvent("purchase", "user-1", "1", null, null); + long start = System.nanoTime(); + processor.close(); + long elapsedMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start); + + verify(logger).error(contains("Stopped waiting")); + assertTrue(elapsedMillis < TimeUnit.SECONDS.toMillis(WAIT_SECONDS) / 2, + "close() waited " + elapsedMillis + "ms"); + } finally { + eventsApi.release(); + } + } + + @Test + public void closeTimeout_followsTheConfiguredTimeouts() { + FlagsmithConfig config = FlagsmithConfig.newBuilder() + .connectTimeout(1000) + .writeTimeout(2000) + .readTimeout(30000) + .withEnableEvents(Boolean.TRUE) + .build(); + + // The read timeout the caller configured, not the SDK default. + assertEquals(2 * (1000 + 2000 + 30000) + 200, + config.getEventProcessor().getCloseTimeoutMillis()); + } + @Test public void trackEvent_neverThrowsWhenTheApiIsMissing() { EventProcessor processor = newProcessor(1, 0); From ac499a2c62a59d15f1ee7be433b011593fd27b41 Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 23 Sep 2026 15:11:11 +0200 Subject: [PATCH 12/16] test: pin the closed re-check under the buffer lock, drop a vacuous test The re-check that stops an event racing close() from being stranded in the buffer had no test. A trait whose getter blocks parks the tracking thread between the first check and the lock while close() runs its final flush; removing the re-check makes it fail. FlagsmithClientTest.testCloseDoesNotWedgeLaterFlushes buffered nothing once tracking after close became a no-op, so it could not fail. The paths it meant to cover are pinned in EventProcessorTest by flush_completesWhenTheRequestProcessorIsAlreadyShutDown and trackEvent_isANoOpAfterClose. --- .../com/flagsmith/FlagsmithClientTest.java | 31 ----------------- .../flagsmith/threads/EventProcessorTest.java | 34 +++++++++++++++++++ 2 files changed, 34 insertions(+), 31 deletions(-) diff --git a/src/test/java/com/flagsmith/FlagsmithClientTest.java b/src/test/java/com/flagsmith/FlagsmithClientTest.java index ada3806f..aa54a85a 100644 --- a/src/test/java/com/flagsmith/FlagsmithClientTest.java +++ b/src/test/java/com/flagsmith/FlagsmithClientTest.java @@ -7,7 +7,6 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -40,7 +39,6 @@ import com.flagsmith.threads.RequestProcessor; import java.io.IOException; -import java.time.Duration; import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -1067,35 +1065,6 @@ public void testGetExperimentFlagPassesTraitsThroughToTheExposure() eq("checkout_cta"), eq("user-2"), eq("treatment"), eq(new HashMap<>()), any()); } - @Test - public void testCloseDoesNotWedgeLaterFlushes() throws FlagsmithClientError { - String baseUrl = "http://bad-url"; - MockInterceptor interceptor = new MockInterceptor(); - FlagsmithClient client = FlagsmithClient.newBuilder() - .withConfiguration(eventsConfigBuilder(baseUrl, interceptor) - .withEventsMaxBufferItems(1) - .withEventsFlushIntervalMillis(0) - .build()) - .setApiKey("api-key") - .build(); - interceptor.addRule() - .post("http://events-uri/v1/events") - .anyTimes() - .respond("{\"accepted\": 0, \"rejected\": []}", MEDIATYPE_JSON); - - client.close(); - - // Buffering after close is a no-op, and nothing left behind may stall a later flush. - client.trackEvent("purchase", "user-1"); - client.trackExposureEvent("checkout_cta", "user-1", "treatment"); - - assertTrue(assertTimeoutPreemptively(Duration.ofSeconds(10), - () -> { - client.flushEvents().join(); - return Boolean.TRUE; - })); - } - @Test public void testEventsInOfflineModeThrowsAtBuild() { EventProcessor processor = mock(EventProcessor.class); diff --git a/src/test/java/com/flagsmith/threads/EventProcessorTest.java b/src/test/java/com/flagsmith/threads/EventProcessorTest.java index f459c227..9c46102f 100644 --- a/src/test/java/com/flagsmith/threads/EventProcessorTest.java +++ b/src/test/java/com/flagsmith/threads/EventProcessorTest.java @@ -442,6 +442,40 @@ public void trackEvent_isANoOpAfterClose() { assertEquals(postsAfterClose, recorder.count()); } + @Test + @SneakyThrows + public void trackEvent_refusesAnEventThatRacesCloseInsteadOfStrandingIt() { + EventProcessor processor = newProcessor(1000, 0); + interceptor.addRule().post(EVENTS_ENDPOINT).anyTimes().respond(ACCEPTED_BODY, MEDIATYPE_JSON); + + // Serialising the traits happens after the first closed check and before the buffer lock, + // so a getter that blocks parks the tracking thread exactly in that window. + CountDownLatch serialising = new CountDownLatch(1); + CountDownLatch proceed = new CountDownLatch(1); + Object slowTrait = new Object() { + @SuppressWarnings("unused") + public String getValue() throws InterruptedException { + serialising.countDown(); + proceed.await(WAIT_SECONDS, TimeUnit.SECONDS); + return "slow"; + } + }; + Thread tracker = new Thread(() -> processor.trackEvent( + "purchase", "user-1", "1", Collections.singletonMap("slow", slowTrait), null)); + tracker.start(); + assertTrue(serialising.await(WAIT_SECONDS, TimeUnit.SECONDS)); + + // close() runs its final flush while the event is still on its way into the buffer. + processor.close(); + eventProcessor = null; + proceed.countDown(); + tracker.join(TimeUnit.SECONDS.toMillis(WAIT_SECONDS)); + + // Without the check under the lock, the event lands in a buffer nothing will flush again. + assertTrue(processor.bufferedEvents().isEmpty(), "an event was stranded after close()"); + assertEquals(0, recorder.count()); + } + @Test public void trackExposureEvent_unwrapsTraitConfigsAndDropsTransientTraits() { EventProcessor processor = newProcessor(1000, 0); From d4e6185af32e70996fbb9a0ed005afeb853f41ff Mon Sep 17 00:00:00 2001 From: wadii Date: Wed, 23 Sep 2026 15:13:57 +0200 Subject: [PATCH 13/16] fix: keep a self-referencing trait map from escaping trackEvent as an Error Serialising traits and metadata with valueToTree at buffer time let a map or list that contains itself throw a raw StackOverflowError out of trackEvent. The older flush-time serialisation had reported the same input as a JsonMappingException, so this was a regression from moving serialisation earlier. Each event's traits and metadata are now written with writeValueAsString, which reports every cycle as a checked JsonMappingException, and are held as RawValue that Jackson emits verbatim in the batch. The offending event is dropped and logged; the rest are unaffected. Nothing catches Error, and the class Javadoc now says so. Buffered JSON text is also more compact to hold than a tree. --- .../com/flagsmith/threads/EventProcessor.java | 33 +++++++++++------ .../flagsmith/threads/EventProcessorTest.java | 36 ++++++++++++++++--- 2 files changed, 53 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/flagsmith/threads/EventProcessor.java b/src/main/java/com/flagsmith/threads/EventProcessor.java index c861ca14..554830c5 100644 --- a/src/main/java/com/flagsmith/threads/EventProcessor.java +++ b/src/main/java/com/flagsmith/threads/EventProcessor.java @@ -1,8 +1,9 @@ package com.flagsmith.threads; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.util.RawValue; import com.flagsmith.FlagsmithLogger; import com.flagsmith.MapperFactory; import com.flagsmith.Versions; @@ -39,8 +40,9 @@ * Buffers experimentation events and ships them to the Flagsmith events API. * *

Events are flushed on a fixed interval, when the buffer fills up, and on {@link #close()}. - * Exposure events are deduplicated within a flush window. Nothing thrown here ever reaches caller - * code: every failure is logged instead. + * Exposure events are deduplicated within a flush window. No exception thrown here reaches caller + * code: every failure is logged instead, including traits or metadata that cannot be serialised, + * which drop only their own event. {@link Error}s such as {@code OutOfMemoryError} are not caught. */ public class EventProcessor { @@ -342,11 +344,15 @@ private void bufferEvent(String event, String featureName, String identifier, Ob eventMetadata.put(SDK_VERSION_KEY, Versions.getVersion()); final Object experimentId = eventMetadata.get(EXPERIMENT_ID_KEY); - // Traits and metadata are caller objects of any shape. Turning them into JSON trees here, - // rather than at flush time, means a value Jackson cannot serialise drops this one event - // (logged below) instead of failing the whole batch it would later be sent in. It is also - // a deep copy, so a caller mutating a nested map afterwards cannot change a buffered event. - ObjectMapper mapper = MapperFactory.getMapper(); + // Traits and metadata are caller objects of any shape. Serialising them here, rather than + // at flush time, means a value Jackson cannot serialise drops this one event (logged + // below) instead of failing the whole batch it would later be sent in. The JSON text is + // also a deep copy, so a caller mutating a nested map afterwards cannot change a buffered + // event, and is more compact to hold than the objects or a JSON tree. + // + // It must be writeValueAsString, not valueToTree: on a map or list that contains itself, + // valueToTree lets a raw StackOverflowError escape, where writeValueAsString reports it as + // a JsonMappingException that the catch below handles. Map eventTraits = eventTraits(traits); Map eventPayload = new LinkedHashMap<>(); @@ -354,8 +360,8 @@ private void bufferEvent(String event, String featureName, String identifier, Ob eventPayload.put("feature_name", featureName); eventPayload.put("identifier", identifier); eventPayload.put("value", stringValue); - eventPayload.put("traits", eventTraits == null ? null : mapper.valueToTree(eventTraits)); - eventPayload.put("metadata", mapper.valueToTree(eventMetadata)); + eventPayload.put("traits", eventTraits == null ? null : toJson(eventTraits)); + eventPayload.put("metadata", toJson(eventMetadata)); eventPayload.put("timestamp", System.currentTimeMillis()); boolean isFull; @@ -379,11 +385,16 @@ private void bufferEvent(String event, String featureName, String identifier, Ob if (isFull) { flush(); } - } catch (RuntimeException e) { + } catch (JsonProcessingException | RuntimeException e) { logger.error("Failed to buffer event " + event + ".", e); } } + /** JSON text Jackson writes out verbatim when the batch is serialised. */ + private static RawValue toJson(Object value) throws JsonProcessingException { + return new RawValue(MapperFactory.getMapper().writeValueAsString(value)); + } + private void logClosed(String event) { logger.info("Not buffering event {}: the event processor is closed.", event); } diff --git a/src/test/java/com/flagsmith/threads/EventProcessorTest.java b/src/test/java/com/flagsmith/threads/EventProcessorTest.java index 9c46102f..9cb8480b 100644 --- a/src/test/java/com/flagsmith/threads/EventProcessorTest.java +++ b/src/test/java/com/flagsmith/threads/EventProcessorTest.java @@ -19,6 +19,7 @@ import static org.mockito.Mockito.when; import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.util.RawValue; import com.flagsmith.FlagsmithLogger; import com.flagsmith.MapperFactory; import com.flagsmith.config.FlagsmithConfig; @@ -117,6 +118,12 @@ private void flushAndWait(EventProcessor processor) { processor.flush().get(WAIT_SECONDS, TimeUnit.SECONDS); } + /** Parse a buffered event's pre-serialised traits or metadata. */ + @SneakyThrows + private static JsonNode json(Object buffered) { + return MapperFactory.getMapper().readTree(((RawValue) buffered).rawValue().toString()); + } + @Test public void trackEvent_buffersStringifiedValueSdkVersionAndTimestamp() { EventProcessor processor = newProcessor(1000, 0); @@ -140,9 +147,9 @@ public void trackEvent_buffersStringifiedValueSdkVersionAndTimestamp() { assertNull(event.get("feature_name")); assertEquals("user-123", event.get("identifier")); assertEquals("49.0", event.get("value")); - assertEquals(MapperFactory.getMapper().valueToTree(traits), event.get("traits")); + assertEquals(MapperFactory.getMapper().valueToTree(traits), json(event.get("traits"))); - JsonNode eventMetadata = (JsonNode) event.get("metadata"); + JsonNode eventMetadata = json(event.get("metadata")); assertEquals("checkout", eventMetadata.get("source").asText()); assertNotNull(eventMetadata.get("sdk_version")); @@ -487,7 +494,7 @@ public void trackExposureEvent_unwrapsTraitConfigsAndDropsTransientTraits() { processor.trackExposureEvent("checkout_cta", "user-1", "treatment", traits, null); - JsonNode buffered = (JsonNode) processor.bufferedEvents().get(0).get("traits"); + JsonNode buffered = json(processor.bufferedEvents().get(0).get("traits")); assertEquals(2, buffered.size()); assertEquals("premium", buffered.get("plan").asText()); assertEquals("gold", buffered.get("tier").asText()); @@ -513,9 +520,9 @@ public void trackEvent_copiesTraitsAndMetadataDeeplyAtBufferTime() { Map event = processor.bufferedEvents().get(0); assertEquals( MapperFactory.getMapper().valueToTree(Collections.singletonMap("plan", "premium")), - event.get("traits")); + json(event.get("traits"))); assertEquals("payment", - ((JsonNode) event.get("metadata")).get("context").get("step").asText()); + json(event.get("metadata")).get("context").get("step").asText()); } @Test @@ -542,6 +549,25 @@ public void trackEvent_dropsOnlyTheEventWhoseValuesCannotBeSerialised() { assertEquals("user-4", events.get(1).get("identifier").asText()); } + @Test + public void trackEvent_dropsAnEventWhoseTraitsOrMetadataContainThemselves() { + EventProcessor processor = newProcessor(1000, 0); + Map cyclic = new HashMap<>(); + cyclic.put("self", cyclic); + List cyclicList = new ArrayList<>(); + cyclicList.add(cyclicList); + + // A caller bug, but it must stay out of caller code: serialising these recurses without + // end, and it has to surface as a dropped event, not a StackOverflowError. + processor.trackEvent("purchase", "user-1", "1", cyclic, null); + processor.trackEvent("purchase", "user-2", "2", null, + Collections.singletonMap("list", cyclicList)); + processor.trackEvent("purchase", "user-3", "3", null, null); + + assertEquals(1, processor.bufferedEvents().size()); + assertEquals("user-3", processor.bufferedEvents().get(0).get("identifier")); + } + @Test @SneakyThrows public void start_flushesOnTheTimer() { From 9213caac47aa16e490c6fa0744c69a685be952f5 Mon Sep 17 00:00:00 2001 From: wadii Date: Thu, 24 Sep 2026 10:03:38 +0200 Subject: [PATCH 14/16] refactor: trim comments --- .../java/com/flagsmith/FlagsmithClient.java | 19 +--- .../com/flagsmith/config/FlagsmithConfig.java | 9 +- src/main/java/com/flagsmith/config/Retry.java | 10 +- .../com/flagsmith/threads/EventProcessor.java | 105 ++++++------------ .../flagsmith/threads/RequestProcessor.java | 13 +-- .../com/flagsmith/FlagsmithClientTest.java | 1 - .../com/flagsmith/FlagsmithRetryTest.java | 3 +- .../flagsmith/threads/EventProcessorTest.java | 17 +-- 8 files changed, 63 insertions(+), 114 deletions(-) diff --git a/src/main/java/com/flagsmith/FlagsmithClient.java b/src/main/java/com/flagsmith/FlagsmithClient.java index 851aec68..1d3a133f 100644 --- a/src/main/java/com/flagsmith/FlagsmithClient.java +++ b/src/main/java/com/flagsmith/FlagsmithClient.java @@ -203,12 +203,7 @@ public List getIdentitySegments(String identifier, Map } /** - * Resolve a flag for an identity and record one {@code $flag_exposure} event when the identity - * is enrolled in a running experiment on that feature. Identity flags are fetched exactly as - * {@link #getIdentityFlags(String)} fetches them. - * - *

Experiment metadata is only carried by remote evaluation. With local evaluation or offline - * mode the flag is still returned but no exposure is recorded. + * As {@link #getExperimentFlag(String, String, Map)}, with no traits. * * @param featureName feature name * @param identifier identifier string @@ -223,12 +218,9 @@ public BaseFlag getExperimentFlag(String featureName, String identifier) } /** - * Resolve a flag for an identity and record one {@code $flag_exposure} event when the identity - * is enrolled in a running experiment on that feature. Identity flags are fetched exactly as - * {@link #getIdentityFlags(String, Map)} fetches them. - * - *

Experiment metadata is only carried by remote evaluation. With local evaluation or offline - * mode the flag is still returned but no exposure is recorded. + * Get an identity's flag, recording one {@code $flag_exposure} event if the identity is enrolled + * in a running experiment on it. Only remote evaluation carries experiment metadata, so local + * evaluation and offline mode record no exposure. * * @param featureName feature name * @param identifier identifier string @@ -246,8 +238,7 @@ public BaseFlag getExperimentFlag( Flags flags = getIdentityFlags(identifier, traits); if (flags == null) { - // The API wrapper returns null rather than throwing when the identities request times out - // or is interrupted. Serve the default flag, as getFlag would with the API unavailable. + // The API wrapper returns null, not throws, on a timed-out or interrupted request. FlagsmithFlagDefaults defaults = getConfig().getFlagsmithFlagDefaults(); if (defaults == null) { throw new FlagsmithApiError("Failed to get feature flags."); diff --git a/src/main/java/com/flagsmith/config/FlagsmithConfig.java b/src/main/java/com/flagsmith/config/FlagsmithConfig.java index 0c6f58a5..ad15769d 100644 --- a/src/main/java/com/flagsmith/config/FlagsmithConfig.java +++ b/src/main/java/com/flagsmith/config/FlagsmithConfig.java @@ -299,9 +299,8 @@ public Builder withEnableAnalytics(Boolean enable) { } /** - * Set the base URL of the Flagsmith events API, overriding the default one. Harmless when - * events are not enabled, so that a shared configuration can carry the URL for the services - * that do enable them. + * Override the events API base URL. Allowed with events disabled, so a shared configuration + * can carry it. * * @param eventsUri the new base URI for the events API * @return the Builder @@ -325,9 +324,9 @@ public Builder withEnableEvents(Boolean enable) { } /** - * Set the event processor. + * Use a custom event processor. Also enables events. * - * @param processor event processor object + * @param processor the processor that buffers and sends events * @return the Builder */ public Builder withEventProcessor(EventProcessor processor) { diff --git a/src/main/java/com/flagsmith/config/Retry.java b/src/main/java/com/flagsmith/config/Retry.java index fa70151f..4a411fa8 100644 --- a/src/main/java/com/flagsmith/config/Retry.java +++ b/src/main/java/com/flagsmith/config/Retry.java @@ -26,11 +26,9 @@ public class Retry { add(503); }}; /** - * When true, a response is only retried if its status code is in {@link #statusForcelist}, and - * never beyond the {@link #total} attempts budget. A connection failure (a null status code) is - * still retried while attempts remain. Defaults to false, which keeps the historical behaviour - * of retrying any status while attempts remain, and retrying a force-listed status regardless of - * the budget. + * When true, only force-listed statuses and connection failures (null status) are retried, and + * never past {@link #total} attempts. False keeps the historical policy: any status retries + * within the budget, and a force-listed one regardless of it. */ private Boolean statusForcelistOnly = Boolean.FALSE; @@ -39,7 +37,7 @@ public Retry(Integer total) { } /** - * Instantiate without a status forcelist policy, which keeps the historical behaviour. + * Create a policy with {@code statusForcelistOnly} off. * * @param total number of attempts before giving up * @param attempts attempts made so far diff --git a/src/main/java/com/flagsmith/threads/EventProcessor.java b/src/main/java/com/flagsmith/threads/EventProcessor.java index 554830c5..b5cfa5a0 100644 --- a/src/main/java/com/flagsmith/threads/EventProcessor.java +++ b/src/main/java/com/flagsmith/threads/EventProcessor.java @@ -39,10 +39,9 @@ /** * Buffers experimentation events and ships them to the Flagsmith events API. * - *

Events are flushed on a fixed interval, when the buffer fills up, and on {@link #close()}. - * Exposure events are deduplicated within a flush window. No exception thrown here reaches caller - * code: every failure is logged instead, including traits or metadata that cannot be serialised, - * which drop only their own event. {@link Error}s such as {@code OutOfMemoryError} are not caught. + *

Events are flushed on a timer, when the buffer fills, and on {@link #close()}. Exposures are + * deduplicated within a flush window. Exceptions are logged, never thrown to the caller; + * {@link Error}s are not caught. */ public class EventProcessor { @@ -58,15 +57,9 @@ public class EventProcessor { private static final MediaType JSON_MEDIA_TYPE = MediaType.get("application/json; charset=utf-8"); /** - * The most events that may be waiting on the events API at once. Once reached, a flush drops - * its batch instead of queueing it: while the API is slow or down, batches are produced faster - * than they are retried and given up on, and an unbounded queue of them would grow with the - * host application's traffic until it ran out of memory. - * - *

It counts events, not batches, so that it bounds memory without capping throughput on a - * healthy API: a batch count would throttle harder the smaller the configured buffer. At the - * default buffer size it is ten batches. A batch is admitted while the count is below the - * limit, so the bound is this plus one buffer's worth. + * Cap on events awaiting the events API. Past it a flush drops its batch rather than queue it, + * so an outage cannot grow memory with the host's traffic. It counts events, not batches, so a + * small buffer is not throttled on a healthy API. The true bound is this plus one buffer. */ static final int MAX_IN_FLIGHT_EVENTS = 10_000; /** The close timeout when the HTTP client's timeouts do not bound a request. */ @@ -83,8 +76,7 @@ public class EventProcessor { /** The interval between timed flushes; 0 means there is no timer. */ @Getter private final int flushIntervalMillis; - // Everything below is internal state, deliberately not exposed: once published, a getter - // would be public API for as long as the SDK is supported. + // No public getters below: each one would become supported API. private final List> buffer = new ArrayList<>(); private final Set dedupeKeys = new HashSet<>(); private final Object lock = new Object(); @@ -110,9 +102,9 @@ public class EventProcessor { private ScheduledFuture scheduledFlush; /** - * Instantiate with an HTTP client. + * Create a processor that sends batches through {@code client}. * - * @param client client instance + * @param client HTTP client; its timeouts also bound {@link #close()} * @param eventsUri base URI of the events API, e.g. https://events.api.flagsmith.com/ * @param maxBufferItems number of buffered events that triggers an immediate flush; at * least 1 @@ -126,14 +118,10 @@ public EventProcessor(OkHttpClient client, HttpUrl eventsUri, int maxBufferItems new RequestProcessor(client, new FlagsmithLogger(), buildRetry())); } - /** - * Instantiate with a request processor. Package-private: it exists for tests, and is not - * something callers should come to depend on. - */ + /** For tests: sends through the given request processor. */ EventProcessor(HttpUrl eventsUri, int maxBufferItems, int flushIntervalMillis, RequestProcessor requestProcessor) { - // Without a positive buffer limit nothing bounds the buffer between timed flushes, and with - // the timer off as well it would grow for as long as the process runs. + // The buffer limit is the only bound on the buffer between timed flushes, or with no timer. if (maxBufferItems < 1) { throw new IllegalArgumentException("maxBufferItems must be at least 1."); } @@ -164,10 +152,9 @@ private static Retry buildRetry() { } /** - * The longest one batch can take to be delivered or given up on: every attempt the retry - * policy allows, each running to the client's timeouts, plus the backoff between them. An - * attempt is bounded by the call timeout when one is set, and otherwise by connect, write and - * read in turn. When none of those bounds it, neither is the batch. + * The longest one batch can take: every attempt the retry policy allows, each at the client's + * timeouts, plus backoff. An attempt is bounded by the call timeout if set, else by connect, + * write and read in turn. * * @return the worst case in milliseconds, or {@link #UNBOUNDED} */ @@ -194,9 +181,9 @@ static long worstCaseBatchMillis(OkHttpClient client, Retry retry) { } /** - * Set the logger used by the processor and by its request processor. + * Set the logger, for this processor and its request processor. * - * @param logger logger instance + * @param logger the client's logger, so event failures appear alongside its other output */ public void setLogger(FlagsmithLogger logger) { this.logger = logger; @@ -282,9 +269,8 @@ public synchronized void start() { } if (closed.get()) { - // The scheduler is shut down, and scheduling on it would throw. This happens when a - // FlagsmithConfig, which owns the processor, is reused after a client built from it was - // closed. + // Scheduling on the shut-down scheduler would throw. Reached when a FlagsmithConfig is + // reused after a client built from it was closed. logger.error("Not starting the event processor: it has been closed."); return; } @@ -294,10 +280,8 @@ public synchronized void start() { } /** - * Stop the flush timer, ship whatever is left on a best-effort basis and release the HTTP - * resources. Blocks until in-flight batches settle, for at most the worst case of one batch - * under the HTTP client's timeouts and the retry policy; with a timeout switched off there is - * no such bound, and it waits as long as the request does. + * Stop the timer, flush what is left and release HTTP resources. Blocks until in-flight + * batches settle, for at most the worst case of one batch; unbounded if a client timeout is off. */ public void close() { synchronized (this) { @@ -322,8 +306,7 @@ public void close() { logger.error("Failed to flush events on close.", e); } - // shutdown(), never shutdownNow(): interrupting a POST mid-flight loses its batch, where - // letting it finish delivers it if the JVM stays up long enough. + // A graceful shutdown: interrupting a POST mid-flight would lose its batch. requestProcessor.close(); } @@ -344,17 +327,10 @@ private void bufferEvent(String event, String featureName, String identifier, Ob eventMetadata.put(SDK_VERSION_KEY, Versions.getVersion()); final Object experimentId = eventMetadata.get(EXPERIMENT_ID_KEY); - // Traits and metadata are caller objects of any shape. Serialising them here, rather than - // at flush time, means a value Jackson cannot serialise drops this one event (logged - // below) instead of failing the whole batch it would later be sent in. The JSON text is - // also a deep copy, so a caller mutating a nested map afterwards cannot change a buffered - // event, and is more compact to hold than the objects or a JSON tree. - // - // It must be writeValueAsString, not valueToTree: on a map or list that contains itself, - // valueToTree lets a raw StackOverflowError escape, where writeValueAsString reports it as - // a JsonMappingException that the catch below handles. Map eventTraits = eventTraits(traits); + // Serialised now, not at flush: an unserialisable value drops only this event, not its + // batch, and the JSON text is a deep copy that later caller mutation cannot reach. Map eventPayload = new LinkedHashMap<>(); eventPayload.put("event", event); eventPayload.put("feature_name", featureName); @@ -367,9 +343,8 @@ private void bufferEvent(String event, String featureName, String identifier, Ob boolean isFull; synchronized (lock) { - // Checked again under the lock: close() sets the flag before its final flush takes the - // lock, so an event either lands in the buffer ahead of that flush or is refused here. - // Checking only above would let an event slip in after the final flush and be lost. + // Re-checked under the lock: close() sets the flag before its final flush takes the + // lock, so an event either makes that flush or is refused here, never stranded. if (closed.get()) { logClosed(event); return; @@ -390,7 +365,10 @@ private void bufferEvent(String event, String featureName, String identifier, Ob } } - /** JSON text Jackson writes out verbatim when the batch is serialised. */ + /** + * JSON text written verbatim into the batch. Uses writeValueAsString, not valueToTree: on a + * self-containing map valueToTree throws a raw StackOverflowError instead of an exception. + */ private static RawValue toJson(Object value) throws JsonProcessingException { return new RawValue(MapperFactory.getMapper().writeValueAsString(value)); } @@ -400,10 +378,8 @@ private void logClosed(String event) { } /** - * Flatten a caller trait map into the flat map of trait values the events API expects. Values - * wrapped in a {@link TraitConfig} are unwrapped, and traits the caller marked transient are - * dropped: transient means "do not persist this against the identity", and an event store keeps - * what it is sent. + * Unwrap {@link TraitConfig} values and drop transient traits: transient means "do not + * persist", and an event store keeps what it is sent. */ private static Map eventTraits(Map traits) { if (traits == null) { @@ -437,13 +413,11 @@ private static String nullToEmpty(String value) { } /** - * Hand a batch to the request processor. {@code tracked} is settled exactly once, on every path - * out of here: leaving it pending would wedge every later {@link #flush()}, since those wait on - * it. + * Hand a batch to the request processor. {@code tracked} is settled on every path out: left + * pending, it would wedge every later {@link #flush()}. */ private void send(List> batch, CompletableFuture tracked) { - // The completion callback outlives this call by as long as the POST takes, so it captures - // the size rather than the list: the serialised request already holds the batch's content. + // The callback lives as long as the POST, so it holds the size and lets the list be freed. final int batchSize = batch.size(); boolean submitted = false; @@ -505,10 +479,8 @@ private void settle(CompletableFuture tracked, int batchSize) { } /** - * Count dropped events, and say whether it is time to report them. The first drop is reported - * at once; later ones accumulate and are reported at most once per - * {@link #DROP_LOG_INTERVAL_NANOS}, so a caller saturating the processor during an outage - * cannot turn every flush into an error line. Called under the lock. + * Count dropped events. The first drop is reported at once, later ones at most once per + * {@link #DROP_LOG_INTERVAL_NANOS}, so an outage does not log per flush. Called under the lock. * * @return the number of drops to report now, or 0 to stay quiet */ @@ -528,10 +500,7 @@ private CompletableFuture awaitInFlight() { return CompletableFuture.allOf(inFlight.toArray(new CompletableFuture[0])); } - /** - * A snapshot of the buffer, for tests. Taken under the lock, so a test never iterates the live - * list while another thread is appending to it. - */ + /** A snapshot of the buffer for tests, taken under the lock. */ List> bufferedEvents() { synchronized (lock) { return new ArrayList<>(buffer); diff --git a/src/main/java/com/flagsmith/threads/RequestProcessor.java b/src/main/java/com/flagsmith/threads/RequestProcessor.java index 035c2a06..36bc4bce 100644 --- a/src/main/java/com/flagsmith/threads/RequestProcessor.java +++ b/src/main/java/com/flagsmith/threads/RequestProcessor.java @@ -84,14 +84,13 @@ public Future executeAsync( } /** - * Execute the request in async mode, returning a CompletableFuture so callers can compose on - * its completion. + * Execute the request in async mode, returning a future callers can compose on. * - * @param request Request object - * @param clazz class type of response - * @param doThrow should throw Exception - * @param retries no of retries before failing - * @param Type inference for the response + * @param request request to send + * @param clazz type to unmarshal the response body into + * @param doThrow whether a failed call completes the future exceptionally + * @param retries retry policy, copied for this call + * @param response type * @return a future completed with the unmarshalled response, or null when the call failed and * doThrow is false */ diff --git a/src/test/java/com/flagsmith/FlagsmithClientTest.java b/src/test/java/com/flagsmith/FlagsmithClientTest.java index aa54a85a..0a571c1a 100644 --- a/src/test/java/com/flagsmith/FlagsmithClientTest.java +++ b/src/test/java/com/flagsmith/FlagsmithClientTest.java @@ -1028,7 +1028,6 @@ public void testEventsConfigWithoutEnableEventsThrows() { @Test public void testEventsUriWithoutEnableEventsIsHarmless() { - // A shared configuration may carry the events URL for the services that do enable events. FlagsmithConfig config = FlagsmithConfig.newBuilder() .eventsUri("http://events-uri") .build(); diff --git a/src/test/java/com/flagsmith/FlagsmithRetryTest.java b/src/test/java/com/flagsmith/FlagsmithRetryTest.java index b2d21ec5..89e0306c 100644 --- a/src/test/java/com/flagsmith/FlagsmithRetryTest.java +++ b/src/test/java/com/flagsmith/FlagsmithRetryTest.java @@ -89,8 +89,7 @@ public void FlagsmithRetry_statusForcelistOnly_stopsAtTheAttemptsBudget() { retry.retryAttempted(); retry.retryAttempted(); - // This is the branch the one-retry-then-drop guarantee rests on: without it, a permanently - // failing endpoint loops forever. + // Without this bound a permanently failing endpoint retries forever. assertFalse(retry.isRetry(503), "a force-listed status must not retry past the budget"); assertFalse(retry.isRetry(null), "a connection failure must not retry past the budget"); } diff --git a/src/test/java/com/flagsmith/threads/EventProcessorTest.java b/src/test/java/com/flagsmith/threads/EventProcessorTest.java index 9cb8480b..58dc4fb1 100644 --- a/src/test/java/com/flagsmith/threads/EventProcessorTest.java +++ b/src/test/java/com/flagsmith/threads/EventProcessorTest.java @@ -478,7 +478,6 @@ public String getValue() throws InterruptedException { proceed.countDown(); tracker.join(TimeUnit.SECONDS.toMillis(WAIT_SECONDS)); - // Without the check under the lock, the event lands in a buffer nothing will flush again. assertTrue(processor.bufferedEvents().isEmpty(), "an event was stranded after close()"); assertEquals(0, recorder.count()); } @@ -557,8 +556,7 @@ public void trackEvent_dropsAnEventWhoseTraitsOrMetadataContainThemselves() { List cyclicList = new ArrayList<>(); cyclicList.add(cyclicList); - // A caller bug, but it must stay out of caller code: serialising these recurses without - // end, and it has to surface as a dropped event, not a StackOverflowError. + // Serialising these recurses without end; it must surface as a dropped event, not an Error. processor.trackEvent("purchase", "user-1", "1", cyclic, null); processor.trackEvent("purchase", "user-2", "2", null, Collections.singletonMap("list", cyclicList)); @@ -745,8 +743,7 @@ public void flush_dropsEventsBeyondTheInFlightLimitInsteadOfQueueingThem() { @Test @SneakyThrows public void flush_neverDropsEventsOnAHealthyApiWithASmallBuffer() { - // A one-event buffer sends a batch per event. Capping batches rather than events throttled - // exactly this configuration, dropping most events even against an instant API. + // A one-event buffer sends a batch per event: the case a cap on batches would throttle. EventProcessor processor = newProcessor(1, 0, AcceptingInterceptor.open()); FlagsmithLogger logger = mock(FlagsmithLogger.class); processor.setLogger(logger); @@ -762,9 +759,8 @@ public void flush_neverDropsEventsOnAHealthyApiWithASmallBuffer() { } /** - * Every error-level call made on a mocked logger, whatever its arguments. Matching on the - * invocations rather than with verify(...) avoids Mockito's varargs matching, under which a - * matcher list silently misses calls with a different number of arguments. + * Every error-level call on a mocked logger. Reads invocations rather than verify(...), whose + * varargs matching silently misses calls with a different argument count. */ private static List errorCalls(FlagsmithLogger logger) { List calls = new ArrayList<>(); @@ -821,9 +817,8 @@ public void flush_logsNothingWhenEveryEventIsAccepted() { } /** - * Accepts every request, optionally holding each one until released, like an events API that - * has stopped answering. It answers itself rather than deferring to the MockInterceptor, whose - * canned response bodies share one buffer and break under concurrent calls. + * Accepts every request, optionally holding each until released. Answers itself because + * MockInterceptor's canned bodies share one buffer and break under concurrent calls. */ private static class AcceptingInterceptor implements Interceptor { From 05d2fc2d962873b64f041025b446337611cce09b Mon Sep 17 00:00:00 2001 From: wadii Date: Thu, 24 Sep 2026 17:14:37 +0200 Subject: [PATCH 15/16] fix: give each client its own event processor FlagsmithConfig built and held the EventProcessor, so clients sharing one config shared a processor: the last build rebound its API key, closing either client stopped events for both, and a custom API wrapper with its own config left the processor in use unstarted. The config now carries only the event settings; FlagsmithClient.build() creates, binds and starts a processor per client from the builder's configuration. An injected processor is still used as given. --- .../java/com/flagsmith/FlagsmithClient.java | 42 +++---- .../com/flagsmith/config/FlagsmithConfig.java | 23 +++- .../com/flagsmith/FlagsmithClientTest.java | 104 +++++++++++++++++- .../flagsmith/threads/EventProcessorTest.java | 7 +- 4 files changed, 145 insertions(+), 31 deletions(-) diff --git a/src/main/java/com/flagsmith/FlagsmithClient.java b/src/main/java/com/flagsmith/FlagsmithClient.java index 1d3a133f..1c58dcfe 100644 --- a/src/main/java/com/flagsmith/FlagsmithClient.java +++ b/src/main/java/com/flagsmith/FlagsmithClient.java @@ -28,8 +28,11 @@ import java.util.concurrent.CompletableFuture; import java.util.function.Function; import java.util.stream.Collectors; +import lombok.AccessLevel; import lombok.Data; +import lombok.Getter; import lombok.NonNull; +import lombok.Setter; import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -44,6 +47,9 @@ public class FlagsmithClient { private FlagsmithSdk flagsmithSdk; private EvaluationContext evaluationContext; private PollingManager pollingManager; + @Getter(AccessLevel.PACKAGE) + @Setter(AccessLevel.NONE) + private EventProcessor eventProcessor; private FlagsmithClient() { } @@ -375,13 +381,11 @@ public void trackExposureEvent(String featureName, String identifier, Object val * are not enabled */ public CompletableFuture flushEvents() { - EventProcessor processor = getEventProcessor(); - - if (processor == null) { + if (eventProcessor == null) { return CompletableFuture.completedFuture(null); } - return processor.flush(); + return eventProcessor.flush(); } /** @@ -393,7 +397,6 @@ public void close() { pollingManager.stopPolling(); } - EventProcessor eventProcessor = getEventProcessor(); if (eventProcessor != null) { eventProcessor.close(); } @@ -401,20 +404,13 @@ public void close() { flagsmithSdk.close(); } - private EventProcessor getEventProcessor() { - FlagsmithConfig config = getConfig(); - return config != null ? config.getEventProcessor() : null; - } - private EventProcessor requireEventProcessor(String action) { - EventProcessor processor = getEventProcessor(); - - if (processor == null) { + if (eventProcessor == null) { throw new FlagsmithRuntimeError( "Events must be enabled to " + action + ". Use withEnableEvents(true)."); } - return processor; + return eventProcessor; } private Flags getEnvironmentFlagsFromEvaluationContext() throws FlagsmithClientError { @@ -710,7 +706,7 @@ public FlagsmithClient build() { if (configuration.getOfflineHandler() == null) { throw new FlagsmithRuntimeError("Offline handler must be provided to use offline mode."); } - if (configuration.getEventProcessor() != null) { + if (configuration.getEnableEvents()) { throw new FlagsmithRuntimeError("Events cannot be enabled in offline mode."); } } @@ -771,10 +767,18 @@ public FlagsmithClient build() { // Last, once nothing else can throw: starting the processor starts its flush timer, which a // failed build would otherwise leave running with no client to close it. - if (configuration.getEventProcessor() != null) { - configuration.getEventProcessor().setApi(client.flagsmithSdk); - configuration.getEventProcessor().setLogger(client.logger); - configuration.getEventProcessor().start(); + if (configuration.getEnableEvents()) { + EventProcessor processor = configuration.getEventProcessor() != null + ? configuration.getEventProcessor() + : new EventProcessor( + configuration.getHttpClient(), + configuration.getEventsUri(), + configuration.getEventsMaxBufferItems(), + configuration.getEventsFlushIntervalMillis()); + processor.setApi(client.flagsmithSdk); + processor.setLogger(client.logger); + processor.start(); + client.eventProcessor = processor; } return this.client; diff --git a/src/main/java/com/flagsmith/config/FlagsmithConfig.java b/src/main/java/com/flagsmith/config/FlagsmithConfig.java index ad15769d..72758446 100644 --- a/src/main/java/com/flagsmith/config/FlagsmithConfig.java +++ b/src/main/java/com/flagsmith/config/FlagsmithConfig.java @@ -42,11 +42,15 @@ public final class FlagsmithConfig { private final OkHttpClient httpClient; private final HttpUrl baseUri; private final HttpUrl eventsUri; + private final Boolean enableEvents; + private final int eventsMaxBufferItems; + private final int eventsFlushIntervalMillis; private final Retry retries; private Boolean enableLocalEvaluation; private Integer environmentRefreshIntervalSeconds; private AnalyticsProcessor analyticsProcessor; + /** The processor from withEventProcessor, or null; each client otherwise builds its own. */ private EventProcessor eventProcessor; private FlagsmithFlagDefaults flagsmithFlagDefaults = null; private Boolean raiseUpdateEnvironmentErrorsOnStartup = true; @@ -96,12 +100,18 @@ protected FlagsmithConfig(Builder builder) { } this.eventsUri = builder.eventsUri; + this.enableEvents = Boolean.TRUE.equals(builder.enableEvents); + this.eventsMaxBufferItems = builder.eventsMaxBufferItems; + this.eventsFlushIntervalMillis = builder.eventsFlushIntervalMillis; - if (Boolean.TRUE.equals(builder.enableEvents)) { - eventProcessor = builder.eventProcessor != null - ? builder.eventProcessor - : new EventProcessor(httpClient, eventsUri, builder.eventsMaxBufferItems, - builder.eventsFlushIntervalMillis); + if (enableEvents) { + if (eventsMaxBufferItems < 1) { + throw new IllegalArgumentException("maxBufferItems must be at least 1."); + } + if (eventsFlushIntervalMillis < 0) { + throw new IllegalArgumentException("flushIntervalMillis must not be negative."); + } + eventProcessor = builder.eventProcessor; } else if (builder.eventsConfigured) { throw new IllegalArgumentException( "Events must be enabled with withEnableEvents(true) to configure the event processor."); @@ -324,7 +334,8 @@ public Builder withEnableEvents(Boolean enable) { } /** - * Use a custom event processor. Also enables events. + * Use a custom event processor. Also enables events. Unlike the default, the processor is + * shared by every client built from this configuration. * * @param processor the processor that buffers and sends events * @return the Builder diff --git a/src/test/java/com/flagsmith/FlagsmithClientTest.java b/src/test/java/com/flagsmith/FlagsmithClientTest.java index 0a571c1a..415bc205 100644 --- a/src/test/java/com/flagsmith/FlagsmithClientTest.java +++ b/src/test/java/com/flagsmith/FlagsmithClientTest.java @@ -5,6 +5,7 @@ import static org.mockito.Mockito.*; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -40,9 +41,11 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.TimeUnit; import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -1032,7 +1035,7 @@ public void testEventsUriWithoutEnableEventsIsHarmless() { .eventsUri("http://events-uri") .build(); - assertNull(config.getEventProcessor()); + assertFalse(config.getEnableEvents()); assertEquals("http://events-uri/", config.getEventsUri().toString()); } @@ -1226,18 +1229,23 @@ public void testEventsSettingsReachTheProcessor() { .withEventsMaxBufferItems(5) .withEventsFlushIntervalMillis(0) .build(); + FlagsmithClient client = FlagsmithClient.newBuilder() + .withConfiguration(config) + .setApiKey("api-key") + .build(); - EventProcessor processor = config.getEventProcessor(); + EventProcessor processor = client.getEventProcessor(); assertEquals("http://events-uri/v1/events", processor.getEventsEndpoint().toString()); assertEquals(5, processor.getMaxBufferItems()); assertEquals(0, processor.getFlushIntervalMillis()); + client.close(); } @Test public void testNullEnableEventsLeavesEventsDisabled() { FlagsmithConfig config = FlagsmithConfig.newBuilder().withEnableEvents(null).build(); - assertNull(config.getEventProcessor()); + assertFalse(config.getEnableEvents()); } @Test @@ -1447,4 +1455,94 @@ public void testCloseFlushesBufferedEvents() throws FlagsmithClientError, IOExce assertEquals("treatment", events.get(0).get("value").asText()); assertEquals(42, events.get(0).get("metadata").get("experiment_id").asInt()); } + + /** An events-enabled config that records each events batch as "environment key|body". */ + private static FlagsmithConfig recordingEventsConfig(List batches) { + MockInterceptor interceptor = new MockInterceptor(); + interceptor.addRule() + .post("http://events-uri/v1/events") + .anyTimes() + .respond("{\"accepted\": 1, \"rejected\": []}", MEDIATYPE_JSON); + return FlagsmithConfig.newBuilder() + .baseUri("http://bad-url") + .addHttpInterceptor((chain) -> { + Request request = chain.request(); + if (request.url().toString().endsWith("/v1/events")) { + Buffer buffer = new Buffer(); + request.body().writeTo(buffer); + batches.add(request.header("X-Environment-Key") + "|" + buffer.readUtf8()); + } + return chain.proceed(request); + }) + .addHttpInterceptor(interceptor) + .eventsUri("http://events-uri") + .withEnableEvents(Boolean.TRUE) + .withEventsFlushIntervalMillis(0) + .build(); + } + + @Test + public void testClientsSharingAConfigSendEventsUnderTheirOwnKeys() throws Exception { + List batches = Collections.synchronizedList(new ArrayList<>()); + FlagsmithConfig config = recordingEventsConfig(batches); + FlagsmithClient clientA = FlagsmithClient.newBuilder() + .withConfiguration(config).setApiKey("key-a").build(); + FlagsmithClient clientB = FlagsmithClient.newBuilder() + .withConfiguration(config).setApiKey("key-b").build(); + + assertNotSame(clientA.getEventProcessor(), clientB.getEventProcessor()); + + clientA.trackEvent("purchase", "user-a"); + clientB.trackEvent("purchase", "user-b"); + clientA.flushEvents().get(5, TimeUnit.SECONDS); + clientB.flushEvents().get(5, TimeUnit.SECONDS); + + assertEquals(2, batches.size()); + assertTrue(batches.get(0).startsWith("key-a|")); + assertTrue(batches.get(0).contains("user-a") && !batches.get(0).contains("user-b")); + assertTrue(batches.get(1).startsWith("key-b|")); + assertTrue(batches.get(1).contains("user-b") && !batches.get(1).contains("user-a")); + clientA.close(); + clientB.close(); + } + + @Test + public void testClosingOneClientLeavesAnotherOnTheSameConfigTracking() throws Exception { + List batches = Collections.synchronizedList(new ArrayList<>()); + FlagsmithConfig config = recordingEventsConfig(batches); + FlagsmithClient clientA = FlagsmithClient.newBuilder() + .withConfiguration(config).setApiKey("key-a").build(); + FlagsmithClient clientB = FlagsmithClient.newBuilder() + .withConfiguration(config).setApiKey("key-b").build(); + + clientA.close(); + clientB.trackEvent("purchase", "user-b"); + clientB.flushEvents().get(5, TimeUnit.SECONDS); + + assertEquals(1, batches.size()); + assertTrue(batches.get(0).startsWith("key-b|")); + assertTrue(batches.get(0).contains("user-b")); + clientB.close(); + } + + @Test + public void testCustomApiWrapperWithAnEventsConfigDeliversEvents() throws Exception { + List batches = Collections.synchronizedList(new ArrayList<>()); + FlagsmithApiWrapper wrapper = new FlagsmithApiWrapper( + FlagsmithConfig.newBuilder().baseUri("http://bad-url").build(), + null, new FlagsmithLogger(), "wrapper-key"); + FlagsmithClient client = FlagsmithClient.newBuilder() + .withFlagsmithApiWrapper(wrapper) + .withConfiguration(recordingEventsConfig(batches)) + .setApiKey("wrapper-key") + .build(); + + client.trackEvent("purchase", "user-1"); + client.flushEvents().get(5, TimeUnit.SECONDS); + + assertEquals(1, batches.size()); + assertTrue(batches.get(0).startsWith("wrapper-key|")); + assertTrue(batches.get(0).contains("user-1")); + client.close(); + } } diff --git a/src/test/java/com/flagsmith/threads/EventProcessorTest.java b/src/test/java/com/flagsmith/threads/EventProcessorTest.java index 58dc4fb1..3f98efbb 100644 --- a/src/test/java/com/flagsmith/threads/EventProcessorTest.java +++ b/src/test/java/com/flagsmith/threads/EventProcessorTest.java @@ -679,12 +679,13 @@ public void closeTimeout_followsTheConfiguredTimeouts() { .connectTimeout(1000) .writeTimeout(2000) .readTimeout(30000) - .withEnableEvents(Boolean.TRUE) .build(); + EventProcessor processor = + new EventProcessor(config.getHttpClient(), config.getEventsUri(), 1, 0); // The read timeout the caller configured, not the SDK default. - assertEquals(2 * (1000 + 2000 + 30000) + 200, - config.getEventProcessor().getCloseTimeoutMillis()); + assertEquals(2 * (1000 + 2000 + 30000) + 200, processor.getCloseTimeoutMillis()); + processor.close(); } @Test From 892e05c92d5624c77ceb200056df8d9781498600 Mon Sep 17 00:00:00 2001 From: wadii Date: Fri, 25 Sep 2026 14:18:33 +0200 Subject: [PATCH 16/16] fix: let an injected event processor back only one client A processor passed to withEventProcessor was rebound by every build(), so two clients from one config shared it: events went under the last client's key and closing either stopped it for both. build() now claims the processor once, atomically, and a second build throws. Also name the exact retried statuses (500, 502, 503, 504) in the retry policy's Javadoc instead of "a 5xx". --- .../java/com/flagsmith/FlagsmithClient.java | 1 + .../com/flagsmith/config/FlagsmithConfig.java | 4 +-- .../com/flagsmith/threads/EventProcessor.java | 21 ++++++++++--- .../com/flagsmith/FlagsmithClientTest.java | 31 +++++++++++++++++-- 4 files changed, 49 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/flagsmith/FlagsmithClient.java b/src/main/java/com/flagsmith/FlagsmithClient.java index 1c58dcfe..644f9f46 100644 --- a/src/main/java/com/flagsmith/FlagsmithClient.java +++ b/src/main/java/com/flagsmith/FlagsmithClient.java @@ -775,6 +775,7 @@ public FlagsmithClient build() { configuration.getEventsUri(), configuration.getEventsMaxBufferItems(), configuration.getEventsFlushIntervalMillis()); + processor.claim(); processor.setApi(client.flagsmithSdk); processor.setLogger(client.logger); processor.start(); diff --git a/src/main/java/com/flagsmith/config/FlagsmithConfig.java b/src/main/java/com/flagsmith/config/FlagsmithConfig.java index 72758446..c65b0ddd 100644 --- a/src/main/java/com/flagsmith/config/FlagsmithConfig.java +++ b/src/main/java/com/flagsmith/config/FlagsmithConfig.java @@ -334,8 +334,8 @@ public Builder withEnableEvents(Boolean enable) { } /** - * Use a custom event processor. Also enables events. Unlike the default, the processor is - * shared by every client built from this configuration. + * Use a custom event processor. Also enables events. The processor can back only one client: + * building a second client from this configuration throws. * * @param processor the processor that buffers and sends events * @return the Builder diff --git a/src/main/java/com/flagsmith/threads/EventProcessor.java b/src/main/java/com/flagsmith/threads/EventProcessor.java index b5cfa5a0..86bdc680 100644 --- a/src/main/java/com/flagsmith/threads/EventProcessor.java +++ b/src/main/java/com/flagsmith/threads/EventProcessor.java @@ -8,6 +8,7 @@ import com.flagsmith.MapperFactory; import com.flagsmith.Versions; import com.flagsmith.config.Retry; +import com.flagsmith.exceptions.FlagsmithRuntimeError; import com.flagsmith.interfaces.FlagsmithSdk; import com.flagsmith.models.TraitConfig; import java.util.ArrayList; @@ -99,6 +100,7 @@ public class EventProcessor { private FlagsmithSdk api; private FlagsmithLogger logger = new FlagsmithLogger(); private final AtomicBoolean closed = new AtomicBoolean(false); + private final AtomicBoolean claimed = new AtomicBoolean(false); private ScheduledFuture scheduledFlush; /** @@ -141,8 +143,8 @@ public EventProcessor(OkHttpClient client, HttpUrl eventsUri, int maxBufferItems } /** - * The retry policy for an event batch: at most one retry, on a connection failure or a 5xx, - * and never on a 4xx. + * The retry policy for an event batch: at most one retry, on a connection failure or a 500, + * 502, 503 or 504, and never on any other status. */ private static Retry buildRetry() { Retry retry = new Retry(2); @@ -180,6 +182,17 @@ static long worstCaseBatchMillis(OkHttpClient client, Retry retry) { return total; } + /** + * Reserve this processor for one client; called by {@code FlagsmithClient.Builder}. + * + * @throws FlagsmithRuntimeError when another client already holds it + */ + public void claim() { + if (!claimed.compareAndSet(false, true)) { + throw new FlagsmithRuntimeError("This event processor already backs another client."); + } + } + /** * Set the logger, for this processor and its request processor. * @@ -269,8 +282,8 @@ public synchronized void start() { } if (closed.get()) { - // Scheduling on the shut-down scheduler would throw. Reached when a FlagsmithConfig is - // reused after a client built from it was closed. + // Scheduling on the shut-down scheduler would throw. Reached when an injected processor + // was closed before its client was built. logger.error("Not starting the event processor: it has been closed."); return; } diff --git a/src/test/java/com/flagsmith/FlagsmithClientTest.java b/src/test/java/com/flagsmith/FlagsmithClientTest.java index 415bc205..04188cf0 100644 --- a/src/test/java/com/flagsmith/FlagsmithClientTest.java +++ b/src/test/java/com/flagsmith/FlagsmithClientTest.java @@ -1148,6 +1148,7 @@ public void testFailedBuildDoesNotStartTheEventProcessor() { .setApiKey("api-key"); assertThrows(FlagsmithRuntimeError.class, clientBuilder::build); + verify(processor, never()).claim(); verify(processor, never()).start(); } @@ -1458,6 +1459,10 @@ public void testCloseFlushesBufferedEvents() throws FlagsmithClientError, IOExce /** An events-enabled config that records each events batch as "environment key|body". */ private static FlagsmithConfig recordingEventsConfig(List batches) { + return recordingEventsConfigBuilder(batches).build(); + } + + private static FlagsmithConfig.Builder recordingEventsConfigBuilder(List batches) { MockInterceptor interceptor = new MockInterceptor(); interceptor.addRule() .post("http://events-uri/v1/events") @@ -1477,8 +1482,7 @@ private static FlagsmithConfig recordingEventsConfig(List batches) { .addHttpInterceptor(interceptor) .eventsUri("http://events-uri") .withEnableEvents(Boolean.TRUE) - .withEventsFlushIntervalMillis(0) - .build(); + .withEventsFlushIntervalMillis(0); } @Test @@ -1545,4 +1549,27 @@ public void testCustomApiWrapperWithAnEventsConfigDeliversEvents() throws Except assertTrue(batches.get(0).contains("user-1")); client.close(); } + + @Test + public void testAnInjectedEventProcessorBacksOnlyOneClient() throws Exception { + List batches = Collections.synchronizedList(new ArrayList<>()); + FlagsmithConfig.Builder configBuilder = recordingEventsConfigBuilder(batches); + FlagsmithConfig probe = configBuilder.build(); + FlagsmithConfig config = configBuilder + .withEventProcessor(new EventProcessor( + probe.getHttpClient(), probe.getEventsUri(), 1000, 0)) + .build(); + FlagsmithClient clientA = FlagsmithClient.newBuilder() + .withConfiguration(config).setApiKey("key-a").build(); + FlagsmithClient.Builder clientBBuilder = FlagsmithClient.newBuilder() + .withConfiguration(config).setApiKey("key-b"); + + assertThrows(FlagsmithRuntimeError.class, clientBBuilder::build); + + clientA.trackEvent("purchase", "user-a"); + clientA.flushEvents().get(5, TimeUnit.SECONDS); + assertEquals(1, batches.size()); + assertTrue(batches.get(0).startsWith("key-a|")); + clientA.close(); + } }