Skip to content

Commit

Permalink
fix: registered LDStatusListeners are now sent updates that result fr…
Browse files Browse the repository at this point in the history 
…om a call to LDClient.identify(...) (#274)

**Requirements**

- [x] I have added test coverage for new or changed functionality

- [x] I have followed the repository's [pull request submission
guidelines](../blob/main/CONTRIBUTING.md#submitting-pull-requests)

- [x] I have validated my changes against all supported platform
versions

**Related issues**


https://app.shortcut.com/launchdarkly/story/211876/android-sdk-connectioninformation-and-listeners-not-updated-correctly

**Describe the solution you've provided**

The issue was introduced in version 4.X and is has affected 2 major
versions. The root of the issue is that from 3.x to 4.x, the code that
sets the connection mode and kicks the last successful connection
timestamp was moved out of the connectivity manager, so reconnects
(which are handled inside the connectivity manager), were not handled.
The fix is to adjust the code so that cconnection information /
connection timestamps are kicked when the reconnect logic succeeds or
fails.

**Describe alternatives you've considered**

Refactoring to match iOS's logic of emitting an offline event when
reconnecting. This would more closely match 3.x. This was rejected as
not having a good ratio of work required and risk involved. It also
could introduce unexpected behavior for customers that implemented
LDStatusListeners in version 4.x and 5.x.
  • Loading branch information
@tanderson-ld
tanderson-ld authored Jul 10, 2024
1 parent 5a0fd0e commit a648213
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 47 deletions.
94 changes: 57 additions & 37 deletions ...ly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ConnectivityManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,6 @@ class ConnectivityManager {
// source we're using, like "if there was an error, update the ConnectionInformation."
private class DataSourceUpdateSinkImpl implements DataSourceUpdateSink {
private final ContextDataManager contextDataManager;
private final AtomicReference<ConnectionMode> connectionMode = new AtomicReference<>(null);
private final AtomicReference<LDFailure> lastFailure = new AtomicReference<>(null);

DataSourceUpdateSinkImpl(ContextDataManager contextDataManager) {
this.contextDataManager = contextDataManager;
Expand All @@ -97,40 +95,10 @@ public void upsert(LDContext context, DataModel.Flag item) {

@Override
public void setStatus(ConnectionMode newConnectionMode, Throwable error) {
ConnectionMode oldConnectionMode = newConnectionMode == null ? null :
connectionMode.getAndSet(newConnectionMode);
LDFailure failure = null;
if (error != null) {
if (error instanceof LDFailure) {
failure = (LDFailure)error;
} else {
failure = new LDFailure("Unknown failure", error, LDFailure.FailureType.UNKNOWN_ERROR);
}
lastFailure.set(failure);
}
boolean updated = false;
if (newConnectionMode != null && oldConnectionMode != newConnectionMode) {
if (failure == null && newConnectionMode.isConnectionActive()) {
connectionInformation.setLastSuccessfulConnection(System.currentTimeMillis());
}
connectionInformation.setConnectionMode(newConnectionMode);
updated = true;
}
if (failure != null) {
connectionInformation.setLastFailedConnection(System.currentTimeMillis());
connectionInformation.setLastFailure(failure);
updated = true;
}
if (updated) {
try {
saveConnectionInformation();
} catch (Exception ex) {
LDUtil.logExceptionAtErrorLevel(logger, ex, "Error saving connection information");
}
updateStatusListeners(connectionInformation);
if (failure != null) {
updateListenersOnFailure(failure);
}
if (error == null) {
updateConnectionInfoForSuccess(newConnectionMode);
} else {
updateConnectionInfoForError(newConnectionMode, error);
}
}

Expand Down Expand Up @@ -263,11 +231,17 @@ private synchronized boolean updateDataSource(
@Override
public void onSuccess(Boolean result) {
initialized = true;
// passing the current connection mode since we don't want to change the mode, just trigger
// the logic to update the last connection success.
updateConnectionInfoForSuccess(connectionInformation.getConnectionMode());
onCompletion.onSuccess(null);
}

@Override
public void onError(Throwable error) {
// passing the current connection mode since we don't want to change the mode, just trigger
// the logic to update the last connection failure.
updateConnectionInfoForError(connectionInformation.getConnectionMode(), error);
onCompletion.onSuccess(null);
}
});
Expand Down Expand Up @@ -303,6 +277,52 @@ void unregisterStatusListener(LDStatusListener LDStatusListener) {
}
}

private void updateConnectionInfoForSuccess(ConnectionMode connectionMode) {
boolean updated = false;
if (connectionInformation.getConnectionMode() != connectionMode) {
connectionInformation.setConnectionMode(connectionMode);
updated = true;
}

// even if connection mode doesn't change, it may be the case that the data source re-established its connection
// and so we should update the last successful connection time (e.g. connection drops and we reconnect,
// an identify occurs)
if (connectionMode.isConnectionActive()) {
connectionInformation.setLastSuccessfulConnection(System.currentTimeMillis());
updated = true;
}

if (updated) {
try {
saveConnectionInformation(connectionInformation);
} catch (Exception ex) {
LDUtil.logExceptionAtErrorLevel(logger, ex, "Error saving connection information");
}
updateStatusListeners(connectionInformation);
}
}

private void updateConnectionInfoForError(ConnectionMode connectionMode, Throwable error) {
LDFailure failure = null;
if (error != null) {
if (error instanceof LDFailure) {
failure = (LDFailure)error;
} else {
failure = new LDFailure("Unknown failure", error, LDFailure.FailureType.UNKNOWN_ERROR);
}
}

connectionInformation.setConnectionMode(connectionMode);
connectionInformation.setLastFailedConnection(System.currentTimeMillis());
connectionInformation.setLastFailure(failure);
try {
saveConnectionInformation(connectionInformation);
} catch (Exception ex) {
LDUtil.logExceptionAtErrorLevel(logger, ex, "Error saving connection information");
}
updateStatusListeners(connectionInformation);
}

private void readStoredConnectionState() {
PersistentDataStoreWrapper.SavedConnectionInfo savedConnectionInfo =
environmentStore.getConnectionInfo();
Expand All @@ -315,7 +335,7 @@ private void readStoredConnectionState() {
connectionInformation.setLastFailure(savedConnectionInfo.lastFailure);
}

private synchronized void saveConnectionInformation() {
private synchronized void saveConnectionInformation(ConnectionInformation connectionInformation) {
PersistentDataStoreWrapper.SavedConnectionInfo savedConnectionInfo =
new PersistentDataStoreWrapper.SavedConnectionInfo(
connectionInformation.getLastSuccessfulConnection(),
Expand Down
63 changes: 53 additions & 10 deletions ...ndroid-client-sdk/src/test/java/com/launchdarkly/sdk/android/ConnectivityManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,12 @@

import static com.launchdarkly.sdk.android.TestUtil.requireNoMoreValues;
import static com.launchdarkly.sdk.android.TestUtil.requireValue;
import static org.easymock.EasyMock.anyObject;
import static org.easymock.EasyMock.expectLastCall;
import static org.easymock.EasyMock.replay;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
Expand All @@ -28,6 +32,7 @@
import org.easymock.Mock;
import org.easymock.MockType;
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
Expand All @@ -54,8 +59,6 @@ public class ConnectivityManagerTest extends EasyMockSupport {
private static final EnvironmentData DATA = new DataSetBuilder()
.add("flag1", 1, LDValue.of(true), 0)
.build();
private static final ConnectionMode EXPECTED_FOREGROUND_MODE = ConnectionMode.POLLING;
private static final ConnectionMode EXPECTED_BACKGROUND_MODE = ConnectionMode.BACKGROUND_POLLING;

@Rule
public LogCaptureRule logging = new LogCaptureRule();
Expand Down Expand Up @@ -241,7 +244,7 @@ public void initForegroundDataSource() throws Exception {
assertTrue(connectivityManager.isInitialized());
assertFalse(connectivityManager.isForcedOffline());

assertEquals(EXPECTED_FOREGROUND_MODE, connectivityManager.getConnectionInformation().getConnectionMode());
assertEquals(ConnectionMode.POLLING, connectivityManager.getConnectionInformation().getConnectionMode());
assertNull(connectivityManager.getConnectionInformation().getLastFailure());
assertNull(connectivityManager.getConnectionInformation().getLastFailedConnection());
assertNotNull(connectivityManager.getConnectionInformation().getLastSuccessfulConnection());
Expand All @@ -266,7 +269,7 @@ public void initBackgroundDataSource() throws Exception {
assertTrue(connectivityManager.isInitialized());
assertFalse(connectivityManager.isForcedOffline());

assertEquals(EXPECTED_BACKGROUND_MODE, connectivityManager.getConnectionInformation().getConnectionMode());
assertEquals(ConnectionMode.BACKGROUND_POLLING, connectivityManager.getConnectionInformation().getConnectionMode());
assertNull(connectivityManager.getConnectionInformation().getLastFailure());
assertNull(connectivityManager.getConnectionInformation().getLastFailedConnection());
assertNotNull(connectivityManager.getConnectionInformation().getLastSuccessfulConnection());
Expand All @@ -284,7 +287,7 @@ public void initDataSourceWithKnownError() throws ExecutionException {
final Throwable testError = new Throwable();
final LDFailure testFailure = new LDFailure("failure", testError, LDFailure.FailureType.NETWORK_FAILURE);
ComponentConfigurer<DataSource> dataSourceConfigurer = clientContext ->
MockComponents.failingDataSource(clientContext, EXPECTED_FOREGROUND_MODE, testFailure);
MockComponents.failingDataSource(clientContext, ConnectionMode.POLLING, testFailure);

createTestManager(false, false, dataSourceConfigurer);

Expand All @@ -293,7 +296,7 @@ public void initDataSourceWithKnownError() throws ExecutionException {

assertFalse(connectivityManager.isInitialized());
assertFalse(connectivityManager.isForcedOffline());
assertEquals(EXPECTED_FOREGROUND_MODE, connectivityManager.getConnectionInformation().getConnectionMode());
assertEquals(ConnectionMode.POLLING, connectivityManager.getConnectionInformation().getConnectionMode());
LDFailure failure = connectivityManager.getConnectionInformation().getLastFailure();
assertNotNull(failure);
assertEquals("failure", failure.getMessage());
Expand All @@ -311,7 +314,7 @@ public void initDataSourceWithUnknownError() throws ExecutionException {

final Throwable testError = new Throwable();
ComponentConfigurer<DataSource> dataSourceConfigurer = clientContext ->
MockComponents.failingDataSource(clientContext, EXPECTED_FOREGROUND_MODE, testError);
MockComponents.failingDataSource(clientContext, ConnectionMode.POLLING, testError);

createTestManager(false, false, dataSourceConfigurer);

Expand Down Expand Up @@ -342,7 +345,7 @@ public void setOfflineAfterInit() throws Exception {

assertTrue(connectivityManager.isInitialized());
assertFalse(connectivityManager.isForcedOffline());
assertEquals(EXPECTED_FOREGROUND_MODE, connectivityManager.getConnectionInformation().getConnectionMode());
assertEquals(ConnectionMode.POLLING, connectivityManager.getConnectionInformation().getConnectionMode());

verifyForegroundDataSourceWasCreatedAndStarted(CONTEXT);
verifyNoMoreDataSourcesWereCreated();
Expand All @@ -356,7 +359,7 @@ public void setOfflineAfterInit() throws Exception {

// We don't currently have a good way to wait for this state change to take effect, so we'll
// poll for it.
ConnectionMode newConnectionMode = awaitConnectionModeChangedFrom(EXPECTED_FOREGROUND_MODE);
ConnectionMode newConnectionMode = awaitConnectionModeChangedFrom(ConnectionMode.POLLING);
assertEquals(ConnectionMode.SET_OFFLINE, newConnectionMode);
assertTrue(connectivityManager.isForcedOffline());

Expand Down Expand Up @@ -493,16 +496,19 @@ public void refreshDataSourceForNewContext() throws Exception {
eventProcessor.setInBackground(false); // we expect this call
replayAll();

long connectionTimeBeforeSwitch = connectivityManager.getConnectionInformation().getLastSuccessfulConnection();
LDContext context2 = LDContext.create("context2");
contextDataManager.switchToContext(context2);
AwaitableCallback<Void> done = new AwaitableCallback<>();
connectivityManager.switchToContext(context2, done);
done.await();
long connectionTimeAfterSwitch = connectivityManager.getConnectionInformation().getLastSuccessfulConnection();

verifyAll(); // verifies eventProcessor calls
verifyDataSourceWasStopped();
verifyForegroundDataSourceWasCreatedAndStarted(context2);
verifyNoMoreDataSourcesWereCreated();
assertNotEquals(connectionTimeBeforeSwitch, connectionTimeAfterSwitch);
}

@Test
Expand Down Expand Up @@ -559,14 +565,51 @@ public void refreshDataSourceWhileInBackgroundWithBackgroundPollingDisabled() {
verifyNoMoreDataSourcesWereCreated();
}

@Test
public void notifyListenersWhenStatusChanges() throws Exception {
createTestManager(false, false, makeSuccessfulDataSourceFactory());

awaitStartUp();

LDStatusListener mockListener = mock(LDStatusListener.class);
// expected initial connection
mockListener.onConnectionModeChanged(anyObject(ConnectionInformation.class));
// expected second connection after identify
mockListener.onConnectionModeChanged(anyObject(ConnectionInformation.class));
expectLastCall();
replayAll();

AwaitableCallback<Void> identifyListenersCalled = new AwaitableCallback<>();
connectivityManager.registerStatusListener(mockListener);
connectivityManager.registerStatusListener(new LDStatusListener() {
@Override
public void onConnectionModeChanged(ConnectionInformation connectionInformation) {
// since the callback system is on another thread, need to use awaitable callback
identifyListenersCalled.onSuccess(null);
}

@Override
public void onInternalFailure(LDFailure ldFailure) {
Assert.fail(); // unexpected
}
});

LDContext context2 = LDContext.create("context2");
contextDataManager.switchToContext(context2);
connectivityManager.switchToContext(context2, new AwaitableCallback<>());
identifyListenersCalled.await();

verifyAll();
}

private ComponentConfigurer<DataSource> makeSuccessfulDataSourceFactory() {
return clientContext -> makeSuccessfulDataSource(clientContext);
}

private DataSource makeSuccessfulDataSource(ClientContext clientContext) {
receivedClientContexts.add(clientContext);
return MockComponents.successfulDataSource(clientContext, DATA,
clientContext.isInBackground() ? EXPECTED_BACKGROUND_MODE : EXPECTED_FOREGROUND_MODE,
clientContext.isInBackground() ? ConnectionMode.BACKGROUND_POLLING : ConnectionMode.POLLING,
startedDataSources,
stoppedDataSources);
}
Expand Down

0 comments on commit a648213

Please sign in to comment.