diff --git a/firebase-config/CHANGELOG.md b/firebase-config/CHANGELOG.md index 93a7da1867e..ed0c9f93da6 100644 --- a/firebase-config/CHANGELOG.md +++ b/firebase-config/CHANGELOG.md @@ -1,5 +1,6 @@ # Unreleased - +[fixed] Fixed an issue where the connection to the real-time Remote Config backend could remain +open in the background. # 22.1.0 * [feature] Added support for custom signal targeting in Remote Config. Use `setCustomSignals` API for setting custom signals and use them to build custom targeting conditions in Remote Config. diff --git a/firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigAutoFetch.java b/firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigAutoFetch.java index 81016602532..a93b1dc5784 100644 --- a/firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigAutoFetch.java +++ b/firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigAutoFetch.java @@ -54,6 +54,7 @@ public class ConfigAutoFetch { private final ConfigUpdateListener retryCallback; private final ScheduledExecutorService scheduledExecutorService; private final Random random; + private boolean isInBackground; public ConfigAutoFetch( HttpURLConnection httpURLConnection, @@ -69,6 +70,7 @@ public ConfigAutoFetch( this.retryCallback = retryCallback; this.scheduledExecutorService = scheduledExecutorService; this.random = new Random(); + this.isInBackground = false; } private synchronized void propagateErrors(FirebaseRemoteConfigException exception) { @@ -87,6 +89,10 @@ private synchronized boolean isEventListenersEmpty() { return this.eventListeners.isEmpty(); } + public void setIsInBackground(boolean isInBackground) { + this.isInBackground = isInBackground; + } + private String parseAndValidateConfigUpdateMessage(String message) { int left = message.indexOf('{'); int right = message.lastIndexOf('}'); @@ -105,15 +111,29 @@ public void listenForNotifications() { return; } + // Maintain a reference to the InputStream to guarantee its closure upon completion or in case + // of an exception. + InputStream inputStream = null; try { - InputStream inputStream = httpURLConnection.getInputStream(); + inputStream = httpURLConnection.getInputStream(); handleNotifications(inputStream); - inputStream.close(); } catch (IOException ex) { - // Stream was interrupted due to a transient issue and the system will retry the connection. - Log.d(TAG, "Stream was cancelled due to an exception. Retrying the connection...", ex); + // If the real-time connection is at an unexpected lifecycle state when the app is + // backgrounded, it's expected closing the httpURLConnection will throw an exception. + if (!isInBackground) { + // Otherwise, the real-time server connection was closed due to a transient issue. + Log.d(TAG, "Real-time connection was closed due to an exception.", ex); + } } finally { - httpURLConnection.disconnect(); + if (inputStream != null) { + try { + // Only need to close the InputStream, ConfigRealtimeHttpClient will disconnect + // HttpUrlConnection + inputStream.close(); + } catch (IOException ex) { + Log.d(TAG, "Exception thrown when closing connection stream. Retrying connection...", ex); + } + } } } @@ -186,7 +206,6 @@ private void handleNotifications(InputStream inputStream) throws IOException { } reader.close(); - inputStream.close(); } private void autoFetch(int remainingAttempts, long targetVersion) { diff --git a/firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHandler.java b/firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHandler.java index 5ed1135dfc7..e340ef0b8c0 100644 --- a/firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHandler.java +++ b/firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHandler.java @@ -91,7 +91,7 @@ public synchronized ConfigUpdateListenerRegistration addRealtimeConfigUpdateList } public synchronized void setBackgroundState(boolean isInBackground) { - configRealtimeHttpClient.setRealtimeBackgroundState(isInBackground); + configRealtimeHttpClient.setIsInBackground(isInBackground); if (!isInBackground) { beginRealtime(); } diff --git a/firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHttpClient.java b/firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHttpClient.java index 2c1c44480e2..ef082d21086 100644 --- a/firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHttpClient.java +++ b/firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHttpClient.java @@ -100,6 +100,10 @@ public class ConfigRealtimeHttpClient { /** Flag to indicate whether or not the app is in the background or not. */ private boolean isInBackground; + // The HttpUrlConnection and auto-fetcher for this client. Only one of each exist at a time. + private HttpURLConnection httpURLConnection; + private ConfigAutoFetch configAutoFetch; + private final int ORIGINAL_RETRIES = 8; private final ScheduledExecutorService scheduledExecutorService; private final ConfigFetchHandler configFetchHandler; @@ -111,6 +115,7 @@ public class ConfigRealtimeHttpClient { private final Random random; private final Clock clock; private final ConfigSharedPrefsClient sharedPrefsClient; + private final Object backgroundLock; public ConfigRealtimeHttpClient( FirebaseApp firebaseApp, @@ -145,6 +150,7 @@ public ConfigRealtimeHttpClient( this.sharedPrefsClient = sharedPrefsClient; this.isRealtimeDisabled = false; this.isInBackground = false; + this.backgroundLock = new Object(); } private static String extractProjectNumberFromAppId(String gmpAppId) { @@ -391,14 +397,42 @@ public void run() { } } - void setRealtimeBackgroundState(boolean backgroundState) { - isInBackground = backgroundState; + public void setIsInBackground(boolean isInBackground) { + // Make changes in synchronized block so only one thread sets the background state and calls + // disconnect. + synchronized (backgroundLock) { + this.isInBackground = isInBackground; + + // Propagate to ConfigAutoFetch as well. + if (configAutoFetch != null) { + configAutoFetch.setIsInBackground(isInBackground); + } + // Close the connection if the app is in the background and there is an active + // HttpUrlConnection. + if (isInBackground && httpURLConnection != null) { + httpURLConnection.disconnect(); + } + } } private synchronized void resetRetryCount() { httpRetriesRemaining = ORIGINAL_RETRIES; } + /** + * The check and set http connection method are combined so that when canMakeHttpStreamConnection + * returns true, the same thread can mark isHttpConnectionIsRunning as true to prevent a race + * condition with another thread. + */ + private synchronized boolean checkAndSetHttpConnectionFlagIfNotRunning() { + boolean canMakeConnection = canMakeHttpStreamConnection(); + if (canMakeConnection) { + setIsHttpConnectionRunning(true); + } + + return canMakeConnection; + } + private synchronized void setIsHttpConnectionRunning(boolean connectionRunning) { isHttpConnectionRunning = connectionRunning; } @@ -469,7 +503,7 @@ private String parseForbiddenErrorResponseMessage(InputStream inputStream) { */ @SuppressLint({"VisibleForTests", "DefaultLocale"}) public void beginRealtimeHttpStream() { - if (!canMakeHttpStreamConnection()) { + if (!checkAndSetHttpConnectionFlagIfNotRunning()) { return; } @@ -489,17 +523,21 @@ public void beginRealtimeHttpStream() { this.scheduledExecutorService, (completedHttpUrlConnectionTask) -> { Integer responseCode = null; - HttpURLConnection httpURLConnection = null; + // Get references to InputStream and ErrorStream before listening on the stream so + // that they can be closed without getting them from HttpUrlConnection. + InputStream inputStream = null; + InputStream errorStream = null; try { // If HTTP connection task failed throw exception to move to the catch block. if (!httpURLConnectionTask.isSuccessful()) { throw new IOException(httpURLConnectionTask.getException()); } - setIsHttpConnectionRunning(true); // Get HTTP connection and check response code. httpURLConnection = httpURLConnectionTask.getResult(); + inputStream = httpURLConnection.getInputStream(); + errorStream = httpURLConnection.getErrorStream(); responseCode = httpURLConnection.getResponseCode(); // If the connection returned a 200 response code, start listening for messages. @@ -509,23 +547,32 @@ public void beginRealtimeHttpStream() { sharedPrefsClient.resetRealtimeBackoff(); // Start listening for realtime notifications. - ConfigAutoFetch configAutoFetch = startAutoFetch(httpURLConnection); + configAutoFetch = startAutoFetch(httpURLConnection); configAutoFetch.listenForNotifications(); } } catch (IOException e) { - // Stream could not be open due to a transient issue and the system will retry the - // connection - // without user intervention. - Log.d( - TAG, - "Exception connecting to real-time RC backend. Retrying the connection...", - e); + if (isInBackground) { + // It's possible the app was backgrounded while the connection was open, which + // threw an exception trying to read the response. No real error here, so treat + // this as a success, even if we haven't read a 200 response code yet. + resetRetryCount(); + } else { + // If it's not in the background, there might have been a transient error so the + // client will retry the connection. + Log.d( + TAG, + "Exception connecting to real-time RC backend. Retrying the connection...", + e); + } } finally { - closeRealtimeHttpStream(httpURLConnection); + // Close HTTP connection and associated streams. + closeRealtimeHttpConnection(inputStream, errorStream); setIsHttpConnectionRunning(false); + // Update backoff metadata if the connection failed in the foreground. boolean connectionFailed = - responseCode == null || isStatusCodeRetryable(responseCode); + !isInBackground + && (responseCode == null || isStatusCodeRetryable(responseCode)); if (connectionFailed) { updateBackoffMetadataWithLastFailedStreamConnectionTime( new Date(clock.currentTimeMillis())); @@ -556,24 +603,34 @@ public void beginRealtimeHttpStream() { } } + // Reset parameters. + httpURLConnection = null; + configAutoFetch = null; + return Tasks.forResult(null); }); } - // Pauses Http stream listening - public void closeRealtimeHttpStream(HttpURLConnection httpURLConnection) { - if (httpURLConnection != null) { - httpURLConnection.disconnect(); - - // Explicitly close the input stream due to a bug in the Android okhttp implementation. - // See github.com/firebase/firebase-android-sdk/pull/808. + private void closeHttpConnectionInputStream(InputStream inputStream) { + if (inputStream != null) { try { - httpURLConnection.getInputStream().close(); - if (httpURLConnection.getErrorStream() != null) { - httpURLConnection.getErrorStream().close(); - } - } catch (IOException e) { + inputStream.close(); + } catch (IOException ex) { + Log.d(TAG, "Error closing connection stream.", ex); } } } + + // Pauses Http stream listening by disconnecting the HttpUrlConnection and underlying InputStream + // and ErrorStream if they exist. + @VisibleForTesting + public void closeRealtimeHttpConnection(InputStream inputStream, InputStream errorStream) { + // Disconnect only if the connection is not null and in the foreground. + if (httpURLConnection != null && !isInBackground) { + httpURLConnection.disconnect(); + } + + closeHttpConnectionInputStream(inputStream); + closeHttpConnectionInputStream(errorStream); + } } diff --git a/firebase-config/src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigTest.java b/firebase-config/src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigTest.java index fffc439dc2b..9e8f65c767e 100644 --- a/firebase-config/src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigTest.java +++ b/firebase-config/src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigTest.java @@ -37,6 +37,7 @@ import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; @@ -80,6 +81,7 @@ import com.google.firebase.remoteconfig.internal.rollouts.RolloutsStateSubscriptionsHandler; import java.io.ByteArrayInputStream; import java.io.IOException; +import java.io.InputStream; import java.net.HttpURLConnection; import java.net.URL; import java.nio.charset.StandardCharsets; @@ -350,6 +352,7 @@ public void onError(@NonNull FirebaseRemoteConfigException error) { listeners, mockRetryListener, scheduledExecutorService); + configAutoFetch.setIsInBackground(false); realtimeSharedPrefsClient = new ConfigSharedPrefsClient( context.getSharedPreferences("test_file", Context.MODE_PRIVATE)); @@ -1274,17 +1277,18 @@ public void realtime_client_removeListener_success() { @Test public void realtime_stream_listen_and_end_connection() throws Exception { - when(mockHttpURLConnection.getInputStream()) - .thenReturn( - new ByteArrayInputStream( - "{ \"latestTemplateVersionNumber\": 1 }".getBytes(StandardCharsets.UTF_8))); + InputStream inputStream = + new ByteArrayInputStream( + "{ \"latestTemplateVersionNumber\": 1 }".getBytes(StandardCharsets.UTF_8)); + InputStream inputStreamSpy = spy(inputStream); + when(mockHttpURLConnection.getInputStream()).thenReturn(inputStreamSpy); when(mockFetchHandler.getTemplateVersionNumber()).thenReturn(1L); when(mockFetchHandler.fetchNowWithTypeAndAttemptNumber( ConfigFetchHandler.FetchType.REALTIME, 1)) .thenReturn(Tasks.forResult(realtimeFetchedContainerResponse)); configAutoFetch.listenForNotifications(); - verify(mockHttpURLConnection).disconnect(); + verify(inputStreamSpy, times(2)).close(); } @Test @@ -1308,7 +1312,7 @@ public void realtime_redirectStatusCode_noRetries() throws Exception { .createRealtimeConnection(); doNothing() .when(configRealtimeHttpClientSpy) - .closeRealtimeHttpStream(any(HttpURLConnection.class)); + .closeRealtimeHttpConnection(any(InputStream.class), any(InputStream.class)); when(mockHttpURLConnection.getResponseCode()).thenReturn(301); configRealtimeHttpClientSpy.beginRealtimeHttpStream(); @@ -1329,7 +1333,7 @@ public void realtime_okStatusCode_startAutofetchAndRetries() throws Exception { doNothing().when(configRealtimeHttpClientSpy).retryHttpConnectionWhenBackoffEnds(); doNothing() .when(configRealtimeHttpClientSpy) - .closeRealtimeHttpStream(any(HttpURLConnection.class)); + .closeRealtimeHttpConnection(any(InputStream.class), any(InputStream.class)); when(mockHttpURLConnection.getResponseCode()).thenReturn(200); configRealtimeHttpClientSpy.beginRealtimeHttpStream(); @@ -1348,7 +1352,7 @@ public void realtime_badGatewayStatusCode_noAutofetchButRetries() throws Excepti doNothing().when(configRealtimeHttpClientSpy).retryHttpConnectionWhenBackoffEnds(); doNothing() .when(configRealtimeHttpClientSpy) - .closeRealtimeHttpStream(any(HttpURLConnection.class)); + .closeRealtimeHttpConnection(any(InputStream.class), any(InputStream.class)); when(mockHttpURLConnection.getResponseCode()).thenReturn(502); configRealtimeHttpClientSpy.beginRealtimeHttpStream(); @@ -1367,7 +1371,7 @@ public void realtime_retryableStatusCode_increasesConfigMetadataFailedStreams() doNothing().when(configRealtimeHttpClientSpy).retryHttpConnectionWhenBackoffEnds(); doNothing() .when(configRealtimeHttpClientSpy) - .closeRealtimeHttpStream(any(HttpURLConnection.class)); + .closeRealtimeHttpConnection(any(InputStream.class), any(InputStream.class)); when(mockHttpURLConnection.getResponseCode()).thenReturn(502); int failedStreams = configRealtimeHttpClientSpy.getNumberOfFailedStreams(); @@ -1386,7 +1390,7 @@ public void realtime_retryableStatusCode_increasesConfigMetadataBackoffDate() th doNothing().when(configRealtimeHttpClientSpy).retryHttpConnectionWhenBackoffEnds(); doNothing() .when(configRealtimeHttpClientSpy) - .closeRealtimeHttpStream(any(HttpURLConnection.class)); + .closeRealtimeHttpConnection(any(InputStream.class), any(InputStream.class)); when(mockHttpURLConnection.getResponseCode()).thenReturn(502); Date backoffDate = configRealtimeHttpClientSpy.getBackoffEndTime(); @@ -1407,7 +1411,7 @@ public void realtime_successfulStatusCode_doesNotIncreaseConfigMetadataFailedStr doNothing().when(configRealtimeHttpClientSpy).retryHttpConnectionWhenBackoffEnds(); doNothing() .when(configRealtimeHttpClientSpy) - .closeRealtimeHttpStream(any(HttpURLConnection.class)); + .closeRealtimeHttpConnection(any(InputStream.class), any(InputStream.class)); when(mockHttpURLConnection.getResponseCode()).thenReturn(200); int failedStreams = configRealtimeHttpClientSpy.getNumberOfFailedStreams(); @@ -1428,7 +1432,7 @@ public void realtime_successfulStatusCode_doesNotIncreaseConfigMetadataBackoffDa doNothing().when(configRealtimeHttpClientSpy).retryHttpConnectionWhenBackoffEnds(); doNothing() .when(configRealtimeHttpClientSpy) - .closeRealtimeHttpStream(any(HttpURLConnection.class)); + .closeRealtimeHttpConnection(any(InputStream.class), any(InputStream.class)); when(mockHttpURLConnection.getResponseCode()).thenReturn(200); Date backoffDate = configRealtimeHttpClientSpy.getBackoffEndTime(); @@ -1446,7 +1450,7 @@ public void realtime_forbiddenStatusCode_returnsStreamError() throws Exception { .createRealtimeConnection(); doNothing() .when(configRealtimeHttpClientSpy) - .closeRealtimeHttpStream(any(HttpURLConnection.class)); + .closeRealtimeHttpConnection(any(InputStream.class), any(InputStream.class)); when(mockHttpURLConnection.getErrorStream()) .thenReturn( new ByteArrayInputStream(FORBIDDEN_ERROR_MESSAGE.getBytes(StandardCharsets.UTF_8))); @@ -1469,7 +1473,7 @@ public void realtime_exceptionThrown_noAutofetchButRetries() throws Exception { doNothing().when(configRealtimeHttpClientSpy).retryHttpConnectionWhenBackoffEnds(); doNothing() .when(configRealtimeHttpClientSpy) - .closeRealtimeHttpStream(any(HttpURLConnection.class)); + .closeRealtimeHttpConnection(any(InputStream.class), any(InputStream.class)); configRealtimeHttpClientSpy.beginRealtimeHttpStream(); flushScheduledTasks(); @@ -1511,6 +1515,25 @@ public void realtime_stream_listen_and_failsafe_disabled() throws Exception { verify(mockFetchHandler).getTemplateVersionNumber(); } + @Test + public void realtime_stream_listen_backgrounded_disconnects() throws Exception { + ConfigRealtimeHttpClient configRealtimeHttpClientSpy = spy(configRealtimeHttpClient); + doReturn(Tasks.forResult(mockHttpURLConnection)) + .when(configRealtimeHttpClientSpy) + .createRealtimeConnection(); + doReturn(mockConfigAutoFetch).when(configRealtimeHttpClientSpy).startAutoFetch(any()); + doNothing().when(configRealtimeHttpClientSpy).retryHttpConnectionWhenBackoffEnds(); + doNothing() + .when(configRealtimeHttpClientSpy) + .closeRealtimeHttpConnection(any(InputStream.class), any(InputStream.class)); + when(mockHttpURLConnection.getResponseCode()).thenReturn(200); + configRealtimeHttpClientSpy.beginRealtimeHttpStream(); + configRealtimeHttpClientSpy.setIsInBackground(true); + flushScheduledTasks(); + + verify(mockHttpURLConnection, times(1)).disconnect(); + } + @Test public void realtimeStreamListen_andUnableToParseMessage() throws Exception { when(mockHttpURLConnection.getResponseCode()).thenReturn(200); @@ -1530,15 +1553,28 @@ public void realtimeStreamListen_andUnableToParseMessage() throws Exception { @Test public void realtime_stream_listen_get_inputstream_fail() throws Exception { + InputStream inputStream = mock(InputStream.class); when(mockHttpURLConnection.getResponseCode()).thenReturn(200); - when(mockHttpURLConnection.getInputStream()).thenThrow(IOException.class); + when(mockHttpURLConnection.getInputStream()).thenReturn(inputStream); + when(inputStream.read()).thenThrow(IOException.class); when(mockFetchHandler.getTemplateVersionNumber()).thenReturn(1L); when(mockFetchHandler.fetchNowWithTypeAndAttemptNumber( ConfigFetchHandler.FetchType.REALTIME, 1)) .thenReturn(Tasks.forResult(realtimeFetchedContainerResponse)); configAutoFetch.listenForNotifications(); - verify(mockHttpURLConnection).disconnect(); + verify(inputStream).close(); + } + + @Test + public void realtime_stream_listen_get_inputstream_exception_handling() throws Exception { + InputStream inputStream = mock(InputStream.class); + when(mockHttpURLConnection.getResponseCode()).thenReturn(200); + when(mockHttpURLConnection.getInputStream()).thenThrow(IOException.class); + configAutoFetch.listenForNotifications(); + + verify(mockHttpURLConnection, times(1)).getInputStream(); + verify(inputStream, never()).close(); } @Test