Skip to content

Commit b0b1e2f

Browse files
committed
[SDK-1956] Address PR review feedback
- Short-circuit forceOffline in resolveMode() so ModeState reflects actual platform state - Match ConnectionMode string values to cross-SDK spec (lowercase, hyphenated) - Add Javadoc to ConnectionMode, ClientContextImpl overloads, and FDv2DataSource internals - Inline FDv2DataSourceBuilder casts in ConnectivityManager - Restore try/finally and explanatory comments in runSynchronizers Made-with: Cursor
1 parent 0b070e6 commit b0b1e2f

4 files changed

Lines changed: 118 additions & 90 deletions

File tree

launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ClientContextImpl.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ final class ClientContextImpl extends ClientContext {
3939
@Nullable
4040
private final TransactionalDataStore transactionalDataStore;
4141

42+
/** Used by FDv1 code paths that do not need a {@link TransactionalDataStore}. */
4243
ClientContextImpl(
4344
ClientContext base,
4445
DiagnosticStore diagnosticStore,
@@ -50,6 +51,11 @@ final class ClientContextImpl extends ClientContext {
5051
this(base, diagnosticStore, fetcher, platformState, taskExecutor, perEnvironmentData, null);
5152
}
5253

54+
/**
55+
* Used by FDv2 code paths. The {@code transactionalDataStore} is needed by
56+
* {@link FDv2DataSourceBuilder} to create {@link SelectorSourceFacade} instances
57+
* that provide selector state to initializers and synchronizers.
58+
*/
5359
ClientContextImpl(
5460
ClientContext base,
5561
DiagnosticStore diagnosticStore,
@@ -113,6 +119,7 @@ public static ClientContextImpl get(ClientContext context) {
113119
return new ClientContextImpl(context, null, null, null, null, null);
114120
}
115121

122+
/** Creates a context for FDv1 data sources that do not need a {@link TransactionalDataStore}. */
116123
public static ClientContextImpl forDataSource(
117124
ClientContext baseClientContext,
118125
DataSourceUpdateSink dataSourceUpdateSink,
@@ -124,6 +131,11 @@ public static ClientContextImpl forDataSource(
124131
newInBackground, previouslyInBackground, null);
125132
}
126133

134+
/**
135+
* Creates a context for data sources, optionally including a {@link TransactionalDataStore}.
136+
* FDv2 data sources require the store so that {@link FDv2DataSourceBuilder} can provide
137+
* selector state to initializers and synchronizers via {@link SelectorSourceFacade}.
138+
*/
127139
public static ClientContextImpl forDataSource(
128140
ClientContext baseClientContext,
129141
DataSourceUpdateSink dataSourceUpdateSink,

launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ConnectionMode.java

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,11 @@
55
* {@link ModeDefinition} that specifies which initializers and synchronizers
66
* are active when the SDK is operating in that mode.
77
* <p>
8+
* Not to be confused with {@link ConnectionInformation.ConnectionMode}, which
9+
* is the public FDv1 enum representing the SDK's current connection state
10+
* (e.g. POLLING, STREAMING, SET_OFFLINE). This class is an internal FDv2
11+
* concept describing the <em>desired</em> data-acquisition pipeline.
12+
* <p>
813
* This is a closed enum — custom connection modes (spec 5.3.5 TBD) are not
914
* supported in this release.
1015
* <p>
@@ -15,11 +20,11 @@
1520
*/
1621
final class ConnectionMode {
1722

18-
static final ConnectionMode STREAMING = new ConnectionMode("STREAMING");
19-
static final ConnectionMode POLLING = new ConnectionMode("POLLING");
20-
static final ConnectionMode OFFLINE = new ConnectionMode("OFFLINE");
21-
static final ConnectionMode ONE_SHOT = new ConnectionMode("ONE_SHOT");
22-
static final ConnectionMode BACKGROUND = new ConnectionMode("BACKGROUND");
23+
static final ConnectionMode STREAMING = new ConnectionMode("streaming");
24+
static final ConnectionMode POLLING = new ConnectionMode("polling");
25+
static final ConnectionMode OFFLINE = new ConnectionMode("offline");
26+
static final ConnectionMode ONE_SHOT = new ConnectionMode("one-shot");
27+
static final ConnectionMode BACKGROUND = new ConnectionMode("background");
2328

2429
private final String name;
2530

launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ConnectivityManager.java

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -271,9 +271,8 @@ private synchronized boolean updateDataSource(
271271
);
272272

273273
if (useFDv2ModeResolution) {
274-
FDv2DataSourceBuilder fdv2Builder = (FDv2DataSourceBuilder) dataSourceFactory;
275274
// CONNMODE 2.0.1: mode switches only transition synchronizers, not initializers.
276-
fdv2Builder.setActiveMode(currentFDv2Mode, !isFDv2ModeSwitch);
275+
((FDv2DataSourceBuilder) dataSourceFactory).setActiveMode(currentFDv2Mode, !isFDv2ModeSwitch);
277276
}
278277

279278
DataSource dataSource = dataSourceFactory.build(clientContext);
@@ -439,8 +438,7 @@ synchronized boolean startUp(@NonNull Callback<Void> onCompletion) {
439438

440439
if (useFDv2ModeResolution) {
441440
currentFDv2Mode = resolveMode();
442-
FDv2DataSourceBuilder fdv2Builder = (FDv2DataSourceBuilder) dataSourceFactory;
443-
fdv2Builder.setActiveMode(currentFDv2Mode, true);
441+
((FDv2DataSourceBuilder) dataSourceFactory).setActiveMode(currentFDv2Mode, true);
444442
}
445443

446444
updateEventProcessor(forcedOffline.get(), platformState.isNetworkAvailable(), platformState.isForeground());
@@ -495,15 +493,16 @@ private void handleModeStateChange() {
495493

496494
/**
497495
* Resolves the current platform state to a {@link ConnectionMode} via the
498-
* {@link ModeResolutionTable}.
496+
* {@link ModeResolutionTable}. Force-offline is handled as a short-circuit
497+
* so that {@link ModeState} faithfully represents actual platform state.
499498
*/
500499
private ConnectionMode resolveMode() {
501-
boolean forceOffline = forcedOffline.get();
502-
boolean networkAvailable = platformState.isNetworkAvailable();
503-
boolean foreground = platformState.isForeground();
500+
if (forcedOffline.get()) {
501+
return ConnectionMode.OFFLINE;
502+
}
504503
ModeState state = new ModeState(
505-
foreground && !forceOffline,
506-
networkAvailable && !forceOffline
504+
platformState.isForeground(),
505+
platformState.isNetworkAvailable()
507506
);
508507
return ModeResolutionTable.MOBILE.resolve(state);
509508
}

launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/FDv2DataSource.java

Lines changed: 87 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -304,91 +304,103 @@ private void runSynchronizers(
304304
@NonNull LDContext context,
305305
@NonNull DataSourceUpdateSinkV2 sink
306306
) {
307-
Synchronizer synchronizer = sourceManager.getNextAvailableSynchronizerAndSetActive();
308-
while (synchronizer != null) {
309-
if (stopped.get()) {
310-
return;
311-
}
312-
int synchronizerCount = sourceManager.getAvailableSynchronizerCount();
313-
boolean isPrime = sourceManager.isPrimeSynchronizer();
314-
try {
315-
boolean running = true;
316-
try (FDv2DataSourceConditions.Conditions conditions =
317-
new FDv2DataSourceConditions.Conditions(getConditions(synchronizerCount, isPrime))) {
318-
while (running) {
319-
Future<FDv2SourceResult> nextFuture = synchronizer.next();
320-
Object res = LDFutures.anyOf(conditions.getFuture(), nextFuture).get();
307+
try {
308+
Synchronizer synchronizer = sourceManager.getNextAvailableSynchronizerAndSetActive();
309+
while (synchronizer != null) {
310+
if (stopped.get()) {
311+
return;
312+
}
313+
int synchronizerCount = sourceManager.getAvailableSynchronizerCount();
314+
boolean isPrime = sourceManager.isPrimeSynchronizer();
315+
try {
316+
boolean running = true;
317+
try (FDv2DataSourceConditions.Conditions conditions =
318+
new FDv2DataSourceConditions.Conditions(getConditions(synchronizerCount, isPrime))) {
319+
while (running) {
320+
Future<FDv2SourceResult> nextFuture = synchronizer.next();
321+
// Race the next synchronizer result against any active conditions
322+
// (fallback/recovery timers). Whichever resolves first wins.
323+
Object res = LDFutures.anyOf(conditions.getFuture(), nextFuture).get();
321324

322-
if (res instanceof FDv2DataSourceConditions.ConditionType) {
323-
FDv2DataSourceConditions.ConditionType ct = (FDv2DataSourceConditions.ConditionType) res;
324-
switch (ct) {
325-
case FALLBACK:
326-
logger.debug("Synchronizer {} experienced an interruption; falling back to next synchronizer.",
327-
synchronizer.getClass().getSimpleName());
328-
break;
329-
case RECOVERY:
330-
logger.debug("The data source is attempting to recover to a higher priority synchronizer.");
331-
sourceManager.resetSourceIndex();
332-
break;
325+
if (res instanceof FDv2DataSourceConditions.ConditionType) {
326+
FDv2DataSourceConditions.ConditionType ct = (FDv2DataSourceConditions.ConditionType) res;
327+
switch (ct) {
328+
case FALLBACK:
329+
logger.debug("Synchronizer {} experienced an interruption; falling back to next synchronizer.",
330+
synchronizer.getClass().getSimpleName());
331+
break;
332+
case RECOVERY:
333+
logger.debug("The data source is attempting to recover to a higher priority synchronizer.");
334+
sourceManager.resetSourceIndex();
335+
break;
336+
}
337+
running = false;
338+
break;
333339
}
334-
running = false;
335-
break;
336-
}
337340

338-
if (!(res instanceof FDv2SourceResult)) {
339-
logger.error("Unexpected result type from synchronizer: {}", res != null ? res.getClass().getName() : "null");
340-
continue;
341-
}
341+
if (!(res instanceof FDv2SourceResult)) {
342+
logger.error("Unexpected result type from synchronizer: {}", res != null ? res.getClass().getName() : "null");
343+
continue;
344+
}
342345

343-
FDv2SourceResult result = (FDv2SourceResult) res;
344-
conditions.inform(result);
346+
FDv2SourceResult result = (FDv2SourceResult) res;
347+
// Let conditions observe the result before we act on it so
348+
// they can update their internal state (e.g. reset interruption timers).
349+
conditions.inform(result);
345350

346-
switch (result.getResultType()) {
347-
case CHANGE_SET:
348-
ChangeSet<Map<String, DataModel.Flag>> changeSet = result.getChangeSet();
349-
if (changeSet != null) {
350-
sink.apply(context, changeSet);
351-
sink.setStatus(DataSourceState.VALID, null);
352-
tryCompleteStart(true, null);
353-
}
354-
break;
355-
case STATUS:
356-
FDv2SourceResult.Status status = result.getStatus();
357-
if (status != null) {
358-
switch (status.getState()) {
359-
case INTERRUPTED:
360-
sink.setStatus(DataSourceState.INTERRUPTED, status.getError());
361-
break;
362-
case SHUTDOWN:
363-
running = false;
364-
break;
365-
case TERMINAL_ERROR:
366-
sourceManager.blockCurrentSynchronizer();
367-
running = false;
368-
sink.setStatus(DataSourceState.INTERRUPTED, status.getError());
369-
break;
370-
case GOODBYE:
371-
break;
372-
default:
373-
break;
351+
switch (result.getResultType()) {
352+
case CHANGE_SET:
353+
ChangeSet<Map<String, DataModel.Flag>> changeSet = result.getChangeSet();
354+
if (changeSet != null) {
355+
sink.apply(context, changeSet);
356+
sink.setStatus(DataSourceState.VALID, null);
357+
tryCompleteStart(true, null);
374358
}
375-
}
376-
break;
359+
break;
360+
case STATUS:
361+
FDv2SourceResult.Status status = result.getStatus();
362+
if (status != null) {
363+
switch (status.getState()) {
364+
case INTERRUPTED:
365+
sink.setStatus(DataSourceState.INTERRUPTED, status.getError());
366+
break;
367+
case SHUTDOWN:
368+
// This synchronizer is shutting down cleanly/intentionally
369+
running = false;
370+
break;
371+
case TERMINAL_ERROR:
372+
// This synchronizer cannot recover; block it so the outer
373+
// loop advances to the next available synchronizer.
374+
sourceManager.blockCurrentSynchronizer();
375+
running = false;
376+
sink.setStatus(DataSourceState.INTERRUPTED, status.getError());
377+
break;
378+
case GOODBYE:
379+
// We let the synchronizer handle this internally.
380+
break;
381+
default:
382+
break;
383+
}
384+
}
385+
break;
386+
}
377387
}
378388
}
389+
} catch (ExecutionException e) {
390+
logger.warn("Synchronizer error: {}", e.getCause() != null ? e.getCause().toString() : e.toString());
391+
sink.setStatus(DataSourceState.INTERRUPTED, e.getCause() != null ? e.getCause() : e);
392+
} catch (CancellationException e) {
393+
logger.warn("Synchronizer cancelled: {}", e.toString());
394+
sink.setStatus(DataSourceState.INTERRUPTED, e);
395+
} catch (InterruptedException e) {
396+
logger.warn("Synchronizer interrupted: {}", e.toString());
397+
sink.setStatus(DataSourceState.INTERRUPTED, e);
398+
return;
379399
}
380-
} catch (ExecutionException e) {
381-
logger.warn("Synchronizer error: {}", e.getCause() != null ? e.getCause().toString() : e.toString());
382-
sink.setStatus(DataSourceState.INTERRUPTED, e.getCause() != null ? e.getCause() : e);
383-
} catch (CancellationException e) {
384-
logger.warn("Synchronizer cancelled: {}", e.toString());
385-
sink.setStatus(DataSourceState.INTERRUPTED, e);
386-
} catch (InterruptedException e) {
387-
logger.warn("Synchronizer interrupted: {}", e.toString());
388-
sink.setStatus(DataSourceState.INTERRUPTED, e);
389-
return;
400+
synchronizer = sourceManager.getNextAvailableSynchronizerAndSetActive();
390401
}
391-
synchronizer = sourceManager.getNextAvailableSynchronizerAndSetActive();
402+
} finally {
403+
sourceManager.close();
392404
}
393405
}
394406
}

0 commit comments

Comments
 (0)