-
Notifications
You must be signed in to change notification settings - Fork 23
chore: Handle Fallback From FDv2 to FDv1 #339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b38910f
e2bbb3d
425906c
6ba1874
c259c4a
a6860d2
86d7645
dddc21f
9af8920
3fd905c
b66ab9e
e0eea5d
3b88fd6
c89661a
dde3f8b
4af3c19
64f3144
27ce5a5
61d8dca
7f49606
e65f133
3afd09a
c6beb5c
16b9f8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,11 +8,18 @@ | |
| import com.launchdarkly.sdk.android.integrations.StreamingSynchronizerBuilder; | ||
| import com.launchdarkly.sdk.android.interfaces.ServiceEndpoints; | ||
| import com.launchdarkly.sdk.android.subsystems.DataSourceBuildInputs; | ||
| import com.launchdarkly.sdk.android.subsystems.DataSourceBuilder; | ||
| import com.launchdarkly.sdk.android.subsystems.Initializer; | ||
| import com.launchdarkly.sdk.android.subsystems.Synchronizer; | ||
| import com.launchdarkly.sdk.internal.http.HttpProperties; | ||
|
|
||
| import androidx.annotation.NonNull; | ||
|
|
||
| import java.net.URI; | ||
| import java.util.Arrays; | ||
| import java.util.Collections; | ||
| import java.util.LinkedHashMap; | ||
| import java.util.Map; | ||
|
|
||
| /** | ||
| * Factory methods for FDv2 data source components used with the | ||
|
|
@@ -102,6 +109,35 @@ public Synchronizer build(DataSourceBuildInputs inputs) { | |
| } | ||
| } | ||
|
|
||
| static final class FDv1PollingSynchronizerBuilderImpl implements DataSourceBuilder<Synchronizer> { | ||
|
|
||
| protected int pollIntervalMillis = LDConfig.DEFAULT_POLL_INTERVAL_MILLIS; | ||
|
|
||
| public FDv1PollingSynchronizerBuilderImpl pollIntervalMillis(int pollIntervalMillis) { | ||
| this.pollIntervalMillis = pollIntervalMillis <= LDConfig.DEFAULT_POLL_INTERVAL_MILLIS ? | ||
| LDConfig.DEFAULT_POLL_INTERVAL_MILLIS : pollIntervalMillis; | ||
| return this; | ||
| } | ||
|
|
||
| @Override | ||
| public Synchronizer build(DataSourceBuildInputs inputs) { | ||
| FeatureFetcher fetcher = new HttpFeatureFlagFetcher( | ||
| inputs.getServiceEndpoints().getPollingBaseUri(), | ||
| inputs.isEvaluationReasons(), | ||
| inputs.getHttp().isUseReport(), | ||
| LDUtil.makeHttpProperties(inputs.getHttp()), | ||
| inputs.getCacheDir(), | ||
| inputs.getBaseLogger() | ||
| ); | ||
| return new FDv1PollingSynchronizer( | ||
| inputs.getEvaluationContext(), fetcher, | ||
| inputs.getSharedExecutor(), 0, | ||
| pollIntervalMillis, | ||
| inputs.getBaseLogger() | ||
| ); | ||
| } | ||
| } | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /** | ||
| * Returns a builder for a polling initializer. | ||
| * <p> | ||
|
|
@@ -141,6 +177,63 @@ public static StreamingSynchronizerBuilder streamingSynchronizer() { | |
| return new StreamingSynchronizerBuilderImpl(); | ||
| } | ||
|
|
||
| /** | ||
| * Produces the default mode table used by {@link DataSystemBuilder#buildModeTable}. | ||
| * Defined here (rather than in {@code DataSystemBuilder}) because the FDv1 fallback | ||
| * synchronizer references package-private types that are not visible from the | ||
| * {@code integrations} package. | ||
| * <p> | ||
| * This method is public only for cross-package access within the SDK; it is not | ||
| * intended for use by application code. | ||
| */ | ||
| @NonNull | ||
| public static Map<ConnectionMode, ModeDefinition> makeDefaultModeTable() { | ||
| DataSourceBuilder<Initializer> pollingInitializer = pollingInitializer(); | ||
| DataSourceBuilder<Synchronizer> pollingSynchronizer = pollingSynchronizer(); | ||
| DataSourceBuilder<Synchronizer> streamingSynchronizer = streamingSynchronizer(); | ||
| DataSourceBuilder<Synchronizer> backgroundPollingSynchronizer = | ||
| pollingSynchronizer() | ||
| .pollIntervalMillis(LDConfig.DEFAULT_BACKGROUND_POLL_INTERVAL_MILLIS); | ||
| DataSourceBuilder<Synchronizer> fdv1FallbackPollingSynchronizerForeground = | ||
| new FDv1PollingSynchronizerBuilderImpl(); | ||
|
|
||
| DataSourceBuilder<Synchronizer> fdv1FallbackPollingSynchronizerBackground = | ||
| new FDv1PollingSynchronizerBuilderImpl().pollIntervalMillis(LDConfig.DEFAULT_BACKGROUND_POLL_INTERVAL_MILLIS); | ||
|
|
||
| Map<ConnectionMode, ModeDefinition> table = new LinkedHashMap<>(); | ||
| table.put(ConnectionMode.STREAMING, new ModeDefinition( | ||
| // TODO: cacheInitializer — add once implemented | ||
| Arrays.asList(/* cacheInitializer, */ pollingInitializer), | ||
| Arrays.asList(streamingSynchronizer, pollingSynchronizer), | ||
| fdv1FallbackPollingSynchronizerForeground | ||
| )); | ||
| table.put(ConnectionMode.POLLING, new ModeDefinition( | ||
| // TODO: Arrays.asList(cacheInitializer) — add once implemented | ||
| Collections.<DataSourceBuilder<Initializer>>emptyList(), | ||
| Collections.singletonList(pollingSynchronizer), | ||
| fdv1FallbackPollingSynchronizerForeground | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shared FDv1 fallback builder instance across STREAMING and POLLING modesLow Severity The same Reviewed by Cursor Bugbot for commit 3afd09a. Configure here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is also by design. I think the alternative is to create two separate FDv1PollingSycnhronizer builders (and i believe only their polling durations would be different). |
||
| )); | ||
| table.put(ConnectionMode.OFFLINE, new ModeDefinition( | ||
| // TODO: Arrays.asList(cacheInitializer) — add once implemented | ||
| Collections.<DataSourceBuilder<Initializer>>emptyList(), | ||
| Collections.<DataSourceBuilder<Synchronizer>>emptyList(), | ||
| null | ||
| )); | ||
| table.put(ConnectionMode.ONE_SHOT, new ModeDefinition( | ||
| // TODO: cacheInitializer and streamingInitializer — add once implemented | ||
| Arrays.asList(/* cacheInitializer, */ pollingInitializer /*, streamingInitializer, */), | ||
| Collections.<DataSourceBuilder<Synchronizer>>emptyList(), | ||
| null | ||
| )); | ||
| table.put(ConnectionMode.BACKGROUND, new ModeDefinition( | ||
| // TODO: Arrays.asList(cacheInitializer) — add once implemented | ||
| Collections.<DataSourceBuilder<Initializer>>emptyList(), | ||
| Collections.singletonList(backgroundPollingSynchronizer), | ||
| fdv1FallbackPollingSynchronizerBackground | ||
| )); | ||
| return table; | ||
| } | ||
|
|
||
| /** | ||
| * Returns a builder for configuring automatic connection mode switching in response to | ||
| * platform events (foreground/background and network availability). | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,191 @@ | ||
| package com.launchdarkly.sdk.android; | ||
|
|
||
| import androidx.annotation.NonNull; | ||
|
|
||
| import com.launchdarkly.logging.LDLogger; | ||
| import com.launchdarkly.sdk.LDContext; | ||
| import com.launchdarkly.sdk.android.DataModel.Flag; | ||
| import com.launchdarkly.sdk.android.subsystems.Callback; | ||
| import com.launchdarkly.sdk.android.subsystems.FDv2SourceResult; | ||
| import com.launchdarkly.sdk.android.subsystems.Synchronizer; | ||
| import com.launchdarkly.sdk.fdv2.ChangeSet; | ||
| import com.launchdarkly.sdk.fdv2.ChangeSetType; | ||
| import com.launchdarkly.sdk.fdv2.Selector; | ||
| import com.launchdarkly.sdk.fdv2.SourceResultType; | ||
| import com.launchdarkly.sdk.fdv2.SourceSignal; | ||
| import com.launchdarkly.sdk.json.SerializationException; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.Map; | ||
| import java.util.concurrent.Future; | ||
| import java.util.concurrent.ScheduledExecutorService; | ||
| import java.util.concurrent.ScheduledFuture; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| /** | ||
| * FDv1 polling synchronizer used as a fallback when the server signals that FDv2 endpoints | ||
| * are unavailable via the {@code x-ld-fd-fallback} response header. | ||
| * <p> | ||
| * Delegates the actual HTTP fetch to a {@link FeatureFetcher} (the same transport used by the | ||
| * production FDv1 polling data source) and converts the response into {@link FDv2SourceResult} | ||
| * objects so it can be used as a drop-in synchronizer within the FDv2 data source pipeline. | ||
| */ | ||
| final class FDv1PollingSynchronizer implements Synchronizer { | ||
|
|
||
| private final LDContext evaluationContext; | ||
| private final FeatureFetcher fetcher; | ||
| private final LDLogger logger; | ||
|
|
||
| private final LDAsyncQueue<FDv2SourceResult> resultQueue = new LDAsyncQueue<>(); | ||
| private final LDAwaitFuture<FDv2SourceResult> shutdownFuture = new LDAwaitFuture<>(); | ||
|
|
||
| private volatile ScheduledFuture<?> scheduledTask; | ||
| private final Object taskLock = new Object(); | ||
|
|
||
| /** | ||
| * @param evaluationContext the context to evaluate flags for | ||
| * @param fetcher the HTTP transport for FDv1 polling requests | ||
| * @param executor scheduler for recurring poll tasks | ||
| * @param initialDelayMillis delay before the first poll in milliseconds | ||
| * @param pollIntervalMillis delay between the end of one poll and the start of the next | ||
| * @param logger logger | ||
| */ | ||
| FDv1PollingSynchronizer( | ||
| @NonNull LDContext evaluationContext, | ||
| @NonNull FeatureFetcher fetcher, | ||
| @NonNull ScheduledExecutorService executor, | ||
| long initialDelayMillis, | ||
| long pollIntervalMillis, | ||
| @NonNull LDLogger logger) { | ||
| this.evaluationContext = evaluationContext; | ||
| this.fetcher = fetcher; | ||
| this.logger = logger; | ||
|
|
||
| synchronized (taskLock) { | ||
| scheduledTask = executor.scheduleWithFixedDelay( | ||
| this::pollAndEnqueue, | ||
| initialDelayMillis, | ||
| pollIntervalMillis, | ||
| TimeUnit.MILLISECONDS); | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shared single-thread executor risks deadlock with blocking pollMedium Severity
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this problem is theoretical only. In practice, when the fallback synchronizer is in use, all the other initializers and synchronizers will be closed. In other words, only one synchronizer runs at a time. Regarding the statement "... especially during the transition period when both FDv2 and FDv1 components are active": The old synchronizer is fully closed before the new one is even built. There's no window where both are running simultaneously. I'm not sure what "transition period" is referring to.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe see if you can figure out why Cursor bugbot thinks
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I asked Cursor to analyze this statement a few different ways. In each case, it seems like Cursor generally knew how the shared executor worked because of the comments in that class's definition. But when it made this comment on my PR, it hadn't taken one step further to understand exactly how many threads were being used, and how the threads are being juggled. After writing this, it seems like adding a little bit more to the comments may help in the future. |
||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FDv1 synchronizer starts polling immediately in constructorMedium Severity
Reviewed by Cursor Bugbot for commit 16b9f8f. Configure here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was our design choice. The FDv2 polling synchronizer behaves the same way. |
||
|
|
||
| private void pollAndEnqueue() { | ||
| try { | ||
| FDv2SourceResult result = doPoll(); | ||
|
|
||
| if (result.getResultType() == SourceResultType.STATUS) { | ||
| FDv2SourceResult.Status status = result.getStatus(); | ||
| if (status != null && status.getState() == SourceSignal.TERMINAL_ERROR) { | ||
| synchronized (taskLock) { | ||
| if (scheduledTask != null) { | ||
| scheduledTask.cancel(false); | ||
| scheduledTask = null; | ||
| } | ||
| } | ||
| shutdownFuture.set(result); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| resultQueue.put(result); | ||
| } catch (RuntimeException e) { | ||
| LDUtil.logExceptionAtErrorLevel(logger, e, "Unexpected exception in FDv1 polling synchronizer task"); | ||
| resultQueue.put(FDv2SourceResult.status(FDv2SourceResult.Status.interrupted(e), false)); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Fetches flags via FDv1 polling and converts the result to an {@link FDv2SourceResult}. | ||
| * <p> | ||
| * All result/error processing happens inside the {@link Callback} so the future carries a | ||
| * fully-formed {@link FDv2SourceResult}. This keeps application-level error classification | ||
| * (e.g. {@link LDInvalidResponseCodeFailure}) at the callback layer rather than unwrapping | ||
| * it from a {@link java.util.concurrent.Future#get()} exception. | ||
| */ | ||
| private FDv2SourceResult doPoll() { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code review notes:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't addressed this yet
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my latest push, I've refactored this new FDv1PollingSynchronizer by injecting a FeatureFetcher object. That's the same helper class that the existing PollingDataSource uses 👍 |
||
| LDAwaitFuture<FDv2SourceResult> resultFuture = new LDAwaitFuture<>(); | ||
|
|
||
| fetcher.fetch(evaluationContext, new Callback<String>() { | ||
| @Override | ||
| public void onSuccess(String json) { | ||
| try { | ||
| logger.debug("FDv1 fallback polling response received"); | ||
| EnvironmentData envData = EnvironmentData.fromJson(json); | ||
| Map<String, Flag> flags = envData.getAll(); | ||
|
|
||
| ChangeSet<Map<String, Flag>> changeSet = new ChangeSet<>( | ||
| ChangeSetType.Full, | ||
| Selector.EMPTY, | ||
| flags, | ||
| null, | ||
| true); | ||
| resultFuture.set(FDv2SourceResult.changeSet(changeSet, false)); | ||
| } catch (SerializationException e) { | ||
| LDUtil.logExceptionAtErrorLevel(logger, e, "FDv1 fallback polling failed to parse response"); | ||
| LDFailure failure = new LDFailure( | ||
| "FDv1 fallback: invalid JSON response", e, LDFailure.FailureType.INVALID_RESPONSE_BODY); | ||
| resultFuture.set(FDv2SourceResult.status(FDv2SourceResult.Status.interrupted(failure), false)); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void onError(Throwable e) { | ||
| if (e instanceof LDInvalidResponseCodeFailure) { | ||
| int code = ((LDInvalidResponseCodeFailure) e).getResponseCode(); | ||
| if (code == 400) { | ||
| logger.error("Received 400 response when fetching flag values. " + | ||
| "Please check recommended R8 and/or ProGuard settings"); | ||
| } | ||
| boolean recoverable = LDUtil.isHttpErrorRecoverable(code); | ||
| logger.warn("FDv1 fallback polling failed with HTTP {}", code); | ||
| LDFailure failure = new LDInvalidResponseCodeFailure( | ||
| "FDv1 fallback polling request failed", e, code, recoverable); | ||
| if (!recoverable) { | ||
| resultFuture.set(FDv2SourceResult.status(FDv2SourceResult.Status.terminalError(failure), false)); | ||
| } else { | ||
| resultFuture.set(FDv2SourceResult.status(FDv2SourceResult.Status.interrupted(failure), false)); | ||
| } | ||
| } else if (e instanceof IOException) { | ||
| LDUtil.logExceptionAtErrorLevel(logger, e, "FDv1 fallback polling failed with network error"); | ||
| resultFuture.set(FDv2SourceResult.status(FDv2SourceResult.Status.interrupted(e), false)); | ||
| } else { | ||
| LDUtil.logExceptionAtErrorLevel(logger, e, "FDv1 fallback polling failed"); | ||
| resultFuture.set(FDv2SourceResult.status(FDv2SourceResult.Status.interrupted(e), false)); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| try { | ||
| return resultFuture.get(); | ||
| } catch (InterruptedException e) { | ||
| return FDv2SourceResult.status(FDv2SourceResult.Status.interrupted(e), false); | ||
| } catch (Exception e) { | ||
| // Should not happen — all callback paths call set(), never setException() | ||
| Throwable cause = e.getCause() != null ? e.getCause() : e; | ||
tanderson-ld marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| LDUtil.logExceptionAtErrorLevel(logger, cause, "FDv1 fallback polling failed unexpectedly"); | ||
| return FDv2SourceResult.status(FDv2SourceResult.Status.interrupted(cause), false); | ||
| } | ||
aaron-zeisler marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| @Override | ||
| @NonNull | ||
| public Future<FDv2SourceResult> next() { | ||
| return LDFutures.anyOf(shutdownFuture, resultQueue.take()); | ||
| } | ||
|
|
||
| @Override | ||
| public void close() { | ||
| synchronized (taskLock) { | ||
| if (scheduledTask != null) { | ||
| scheduledTask.cancel(false); | ||
| scheduledTask = null; | ||
| } | ||
| } | ||
| shutdownFuture.set(FDv2SourceResult.status(FDv2SourceResult.Status.shutdown(), false)); | ||
| try { | ||
| fetcher.close(); | ||
| } catch (IOException ignored) { | ||
| } | ||
| } | ||
| } | ||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FDv1 fallback polling uses wrong endpoint path
High Severity
The
FDv1PollingSynchronizerBuilderImplcreates anHttpFeatureFlagFetcherusinginputs.getServiceEndpoints().getPollingBaseUri()as the polling URI. However,getPollingBaseUri()returns the FDv2 polling base URI. The FDv1 fallback synchronizer needs to reach the legacy FDv1/sdk/evalxendpoints, butHttpFeatureFlagFetcherappends FDv1 paths (StandardEndpoints.POLLING_REQUEST_GET_BASE_PATH) to whatever base URI it receives. If the FDv2 polling base URI differs from the FDv1 polling base URI (which is the case when custom service endpoints are configured separately for FDv2), the requests will be sent to the wrong host or path.Reviewed by Cursor Bugbot for commit 16b9f8f. Configure here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bugbot is confused between the base URI and the full path. The
pollingBaseUriis really just the base url, and it's the same between FDv1 and FDv2. Then the correct path is added to the baseUri when thefetch()function is called.