Skip to content
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.

Sign up for GitHub

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

fix: registered LDStatusListeners are now sent updates that result from a call to LDClient.identify(...) #274

Merged
merged 2 commits into from
Jul 10, 2024
Merged

fix: registered LDStatusListeners are now sent updates that result from a call to LDClient.identify(...) #274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf9ece6
fix: registered LDStatusListeners are now sent updates that result fr…
tanderson-ld Jul 8, 2024
0a02ef6
chore: adding unit tests for LDStatusListener bugfix verification
tanderson-ld Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 57 additions & 37 deletions ...ly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ConnectivityManager.java
View file
Open in desktop
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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers: These pieces of data duplicated what was held in the ConnectionInformationState member of the ConnectivityManager, so removed duplication to reduce chance of being out of sync.


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);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers: Majority of the removed code has been moved into updateConnectionInfoForSuccess and updateConnectionInfoForError.

}

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
Open in desktop
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