From a648213dfd6b7f50a76f7a8160f07e62628aedbb Mon Sep 17 00:00:00 2001 From: Todd Anderson <127344469+tanderson-ld@users.noreply.github.com> Date: Wed, 10 Jul 2024 13:07:31 -0500 Subject: [PATCH] fix: registered LDStatusListeners are now sent updates that result from 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. --- .../sdk/android/ConnectivityManager.java | 94 +++++++++++-------- .../sdk/android/ConnectivityManagerTest.java | 63 +++++++++++-- 2 files changed, 110 insertions(+), 47 deletions(-) diff --git a/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ConnectivityManager.java b/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ConnectivityManager.java index f91e824d..f99cb042 100644 --- a/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ConnectivityManager.java +++ b/launchdarkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/ConnectivityManager.java @@ -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 = new AtomicReference<>(null); - private final AtomicReference lastFailure = new AtomicReference<>(null); DataSourceUpdateSinkImpl(ContextDataManager contextDataManager) { this.contextDataManager = contextDataManager; @@ -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); } } @@ -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); } }); @@ -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(); @@ -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(), diff --git a/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/ConnectivityManagerTest.java b/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/ConnectivityManagerTest.java index 903f7751..0ca39dca 100644 --- a/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/ConnectivityManagerTest.java +++ b/launchdarkly-android-client-sdk/src/test/java/com/launchdarkly/sdk/android/ConnectivityManagerTest.java @@ -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; @@ -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; @@ -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(); @@ -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()); @@ -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()); @@ -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 dataSourceConfigurer = clientContext -> - MockComponents.failingDataSource(clientContext, EXPECTED_FOREGROUND_MODE, testFailure); + MockComponents.failingDataSource(clientContext, ConnectionMode.POLLING, testFailure); createTestManager(false, false, dataSourceConfigurer); @@ -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()); @@ -311,7 +314,7 @@ public void initDataSourceWithUnknownError() throws ExecutionException { final Throwable testError = new Throwable(); ComponentConfigurer dataSourceConfigurer = clientContext -> - MockComponents.failingDataSource(clientContext, EXPECTED_FOREGROUND_MODE, testError); + MockComponents.failingDataSource(clientContext, ConnectionMode.POLLING, testError); createTestManager(false, false, dataSourceConfigurer); @@ -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(); @@ -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()); @@ -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 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 @@ -559,6 +565,43 @@ 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 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 makeSuccessfulDataSourceFactory() { return clientContext -> makeSuccessfulDataSource(clientContext); } @@ -566,7 +609,7 @@ private ComponentConfigurer makeSuccessfulDataSourceFactory() { 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); }