Skip to content

Commit 46cccc9

Browse files
danasilverqdpham13
danasilver
and
qdpham13
authored
Disconnect from RC real-time server when app is backgrounded. (#5688)
Store the Realtime client's `HttpUrlConnection` reference on the class instance so it can used by another client instance method (`closeRealtimeHttpStream`) to disconnect when the app is backgrounded. The rest of the changes are follow on work to handle network errors thrown when `HttpUrlConnection#disconnect` is called. Since HttpUrlConnection enforces a [strict lifecycle](https://android.googlesource.com/platform/external/okhttp/+/602d5e4/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpConnection.java#43) and this code (1) interacts with the connection in several methods/classes and (2) holds open the connection for a long time, it's likely randomly disconnecting when backgrounded will occur at an inopportune lifecycle state. This change inserts a few conditionals to ensure we only call once-only network methods once, and error handling to catch expected errors when we disconnect intentionally. Generally, the code could be refactored to consolidate network handling into one class, which would simplify the state that needs to be passed around and places errors must be handled. --------- Co-authored-by: qdpham <[email protected]>
1 parent 1140948 commit 46cccc9

File tree

4 files changed

+120
-62
lines changed

4 files changed

+120
-62
lines changed

‎firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigAutoFetch.java

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ public class ConfigAutoFetch {
5454
private final ConfigUpdateListener retryCallback;
5555
private final ScheduledExecutorService scheduledExecutorService;
5656
private final Random random;
57+
private Boolean isInBackground;
5758

5859
public ConfigAutoFetch(
5960
HttpURLConnection httpURLConnection,
@@ -99,6 +100,7 @@ private String parseAndValidateConfigUpdateMessage(String message) {
99100
}
100101

101102
// Check connection and establish InputStream
103+
// TODO: Refactor connection management so it's handled by ConfigRealtimeHttpClient.
102104
@VisibleForTesting
103105
public void listenForNotifications() {
104106
if (httpURLConnection == null) {
@@ -110,13 +112,23 @@ public void listenForNotifications() {
110112
handleNotifications(inputStream);
111113
inputStream.close();
112114
} catch (IOException ex) {
113-
// Stream was interrupted due to a transient issue and the system will retry the connection.
114-
Log.d(TAG, "Stream was cancelled due to an exception. Retrying the connection...", ex);
115+
// If the real-time connection is at an unexpected lifecycle state when the app is
116+
// backgrounded, it's expected closing the InputStream will throw an exception.
117+
// Moving network management to a single place should remove the need for this check.
118+
// See above todo.
119+
if (!isInBackground) {
120+
// Otherwise, the real-time server connection was closed due to a transient issue.
121+
Log.d(TAG, "Real-time connection was closed due to an exception.", ex);
122+
}
115123
} finally {
116124
httpURLConnection.disconnect();
117125
}
118126
}
119127

128+
public void setInBackground(Boolean inBackground) {
129+
isInBackground = inBackground;
130+
}
131+
120132
// Auto-fetch new config and execute callbacks on each new message
121133
private void handleNotifications(InputStream inputStream) throws IOException {
122134
BufferedReader reader = new BufferedReader((new InputStreamReader(inputStream, "utf-8")));

‎firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHandler.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,9 @@ public synchronized void setBackgroundState(boolean isInBackground) {
9494
configRealtimeHttpClient.setRealtimeBackgroundState(isInBackground);
9595
if (!isInBackground) {
9696
beginRealtime();
97+
} else {
98+
// If we're in the background, close the client's real-time server connection.
99+
configRealtimeHttpClient.closeRealtimeHttpStream();
97100
}
98101
}
99102

‎firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHttpClient.java

Lines changed: 73 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,10 @@ public class ConfigRealtimeHttpClient {
9797
@GuardedBy("this")
9898
private boolean isRealtimeDisabled;
9999

100+
// The HttpUrlConnection and auto-fetcher for this client. Only one of each exist at a time.
101+
private HttpURLConnection httpURLConnection;
102+
private ConfigAutoFetch configAutoFetch;
103+
100104
/** Flag to indicate whether or not the app is in the background or not. */
101105
private boolean isInBackground;
102106

@@ -289,6 +293,20 @@ private synchronized boolean canMakeHttpStreamConnection() {
289293
&& !isInBackground;
290294
}
291295

296+
/**
297+
* The check and set http connection method are combined so that when canMakeHttpStreamConnection
298+
* returns true, the same thread can mark isHttpConnectionIsRunning as true to prevent a race
299+
* condition with another thread.
300+
*/
301+
private synchronized boolean checkAndSetHttpConnectionFlagIfNotRunning() {
302+
boolean canMakeConnection = canMakeHttpStreamConnection();
303+
if (canMakeConnection) {
304+
setIsHttpConnectionRunning(true);
305+
}
306+
307+
return canMakeConnection;
308+
}
309+
292310
private String getRealtimeURL(String namespace) {
293311
return String.format(
294312
REALTIME_REGEX_URL,
@@ -368,10 +386,6 @@ public synchronized void retryHttpConnectionWhenBackoffEnds() {
368386
}
369387

370388
private synchronized void makeRealtimeHttpConnection(long retryMilliseconds) {
371-
if (!canMakeHttpStreamConnection()) {
372-
return;
373-
}
374-
375389
if (httpRetriesRemaining > 0) {
376390
httpRetriesRemaining--;
377391
scheduledExecutorService.schedule(
@@ -393,6 +407,11 @@ public void run() {
393407

394408
void setRealtimeBackgroundState(boolean backgroundState) {
395409
isInBackground = backgroundState;
410+
411+
// Propagate to ConfigAutoFetch, too.
412+
if (configAutoFetch != null) {
413+
this.configAutoFetch.setInBackground(backgroundState);
414+
}
396415
}
397416

398417
private synchronized void resetRetryCount() {
@@ -469,7 +488,7 @@ private String parseForbiddenErrorResponseMessage(InputStream inputStream) {
469488
*/
470489
@SuppressLint({"VisibleForTests", "DefaultLocale"})
471490
public void beginRealtimeHttpStream() {
472-
if (!canMakeHttpStreamConnection()) {
491+
if (!checkAndSetHttpConnectionFlagIfNotRunning()) {
473492
return;
474493
}
475494

@@ -489,7 +508,7 @@ public void beginRealtimeHttpStream() {
489508
this.scheduledExecutorService,
490509
(completedHttpUrlConnectionTask) -> {
491510
Integer responseCode = null;
492-
HttpURLConnection httpURLConnection = null;
511+
httpURLConnection = null;
493512

494513
try {
495514
// If HTTP connection task failed throw exception to move to the catch block.
@@ -509,31 +528,44 @@ public void beginRealtimeHttpStream() {
509528
metadataClient.resetRealtimeBackoff();
510529

511530
// Start listening for realtime notifications.
512-
ConfigAutoFetch configAutoFetch = startAutoFetch(httpURLConnection);
531+
configAutoFetch = startAutoFetch(httpURLConnection);
513532
configAutoFetch.listenForNotifications();
514533
}
515534
} catch (IOException e) {
516-
// Stream could not be open due to a transient issue and the system will retry the
517-
// connection
518-
// without user intervention.
519-
Log.d(
520-
TAG,
521-
"Exception connecting to real-time RC backend. Retrying the connection...",
522-
e);
535+
if (isInBackground) {
536+
// It's possible the app was backgrounded while the connection was open, which
537+
// threw an exception trying to read the response. No real error here, so treat
538+
// this as a success, even if we haven't read a 200 response code yet.
539+
resetRetryCount();
540+
} else {
541+
// If it's not in the background, there might have been a transient error so the
542+
// client will retry the connection.
543+
Log.d(
544+
TAG,
545+
"Exception connecting to real-time RC backend. Retrying the connection...",
546+
e);
547+
}
523548
} finally {
524-
closeRealtimeHttpStream(httpURLConnection);
549+
// The connection is closed explicitly when the app is backgrounded. Many methods
550+
// on HttpUrlConnection cannot be called twice.
551+
if (!isInBackground) {
552+
this.closeRealtimeHttpStream();
553+
}
554+
555+
// Either way indicate the HTTP connection is closed.
525556
setIsHttpConnectionRunning(false);
526557

558+
// Update backoff metadata if the connection failed in the foreground.
527559
boolean connectionFailed =
528-
responseCode == null || isStatusCodeRetryable(responseCode);
560+
!isInBackground
561+
&& (responseCode == null || isStatusCodeRetryable(responseCode));
529562
if (connectionFailed) {
530563
updateBackoffMetadataWithLastFailedStreamConnectionTime(
531564
new Date(clock.currentTimeMillis()));
532565
}
533566

534567
// If responseCode is null then no connection was made to server and the SDK should
535-
// still
536-
// retry.
568+
// still retry.
537569
if (connectionFailed || responseCode == HttpURLConnection.HTTP_OK) {
538570
retryHttpConnectionWhenBackoffEnds();
539571
} else {
@@ -542,8 +574,7 @@ public void beginRealtimeHttpStream() {
542574
"Unable to connect to the server. Try again in a few minutes. HTTP status code: %d",
543575
responseCode);
544576
// Return server message for when the Firebase Remote Config Realtime API is
545-
// disabled and
546-
// the server returns a 403
577+
// disabled and the server returns a 403
547578
if (responseCode == 403) {
548579
errorMessage =
549580
parseForbiddenErrorResponseMessage(httpURLConnection.getErrorStream());
@@ -560,20 +591,29 @@ public void beginRealtimeHttpStream() {
560591
});
561592
}
562593

563-
// Pauses Http stream listening
564-
public void closeRealtimeHttpStream(HttpURLConnection httpURLConnection) {
594+
/** Close the {@link HttpURLConnection} maintained by this client. */
595+
public void closeRealtimeHttpStream() {
565596
if (httpURLConnection != null) {
566-
httpURLConnection.disconnect();
567-
568-
// Explicitly close the input stream due to a bug in the Android okhttp implementation.
569-
// See github.com/firebase/firebase-android-sdk/pull/808.
570-
try {
571-
httpURLConnection.getInputStream().close();
572-
if (httpURLConnection.getErrorStream() != null) {
573-
httpURLConnection.getErrorStream().close();
574-
}
575-
} catch (IOException e) {
576-
}
597+
// Network operations must be off the main thread.
598+
this.scheduledExecutorService.execute(
599+
() -> {
600+
httpURLConnection.disconnect();
601+
602+
// Explicitly close the input stream due to a bug in the Android okhttp implementation.
603+
// See github.com/firebase/firebase-android-sdk/pull/808.
604+
try {
605+
httpURLConnection.getInputStream().close();
606+
if (httpURLConnection.getErrorStream() != null) {
607+
httpURLConnection.getErrorStream().close();
608+
}
609+
} catch (IOException | IllegalStateException e) {
610+
// HttpUrlConnection enforces a strict lifecycle. If the connection is closed before
611+
// the response is read, it'll throw an IllegalStateException. See docs:
612+
// https://android.googlesource.com/platform/external/okhttp/+/602d5e4/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpConnection.java#43
613+
}
614+
});
615+
616+
setIsHttpConnectionRunning(false);
577617
}
578618
}
579619
}

‎firebase-config/src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigTest.java

Lines changed: 30 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,7 @@ public void onError(@NonNull FirebaseRemoteConfigException error) {
349349
listeners,
350350
mockRetryListener,
351351
scheduledExecutorService);
352+
configAutoFetch.setInBackground(false);
352353
realtimeMetadataClient =
353354
new ConfigMetadataClient(context.getSharedPreferences("test_file", Context.MODE_PRIVATE));
354355
configRealtimeHttpClient =
@@ -1304,9 +1305,7 @@ public void realtime_redirectStatusCode_noRetries() throws Exception {
13041305
doReturn(Tasks.forResult(mockHttpURLConnection))
13051306
.when(configRealtimeHttpClientSpy)
13061307
.createRealtimeConnection();
1307-
doNothing()
1308-
.when(configRealtimeHttpClientSpy)
1309-
.closeRealtimeHttpStream(any(HttpURLConnection.class));
1308+
doNothing().when(configRealtimeHttpClientSpy).closeRealtimeHttpStream();
13101309
when(mockHttpURLConnection.getResponseCode()).thenReturn(301);
13111310

13121311
configRealtimeHttpClientSpy.beginRealtimeHttpStream();
@@ -1325,9 +1324,7 @@ public void realtime_okStatusCode_startAutofetchAndRetries() throws Exception {
13251324
.createRealtimeConnection();
13261325
doReturn(mockConfigAutoFetch).when(configRealtimeHttpClientSpy).startAutoFetch(any());
13271326
doNothing().when(configRealtimeHttpClientSpy).retryHttpConnectionWhenBackoffEnds();
1328-
doNothing()
1329-
.when(configRealtimeHttpClientSpy)
1330-
.closeRealtimeHttpStream(any(HttpURLConnection.class));
1327+
doNothing().when(configRealtimeHttpClientSpy).closeRealtimeHttpStream();
13311328
when(mockHttpURLConnection.getResponseCode()).thenReturn(200);
13321329

13331330
configRealtimeHttpClientSpy.beginRealtimeHttpStream();
@@ -1344,9 +1341,7 @@ public void realtime_badGatewayStatusCode_noAutofetchButRetries() throws Excepti
13441341
.when(configRealtimeHttpClientSpy)
13451342
.createRealtimeConnection();
13461343
doNothing().when(configRealtimeHttpClientSpy).retryHttpConnectionWhenBackoffEnds();
1347-
doNothing()
1348-
.when(configRealtimeHttpClientSpy)
1349-
.closeRealtimeHttpStream(any(HttpURLConnection.class));
1344+
doNothing().when(configRealtimeHttpClientSpy).closeRealtimeHttpStream();
13501345
when(mockHttpURLConnection.getResponseCode()).thenReturn(502);
13511346

13521347
configRealtimeHttpClientSpy.beginRealtimeHttpStream();
@@ -1363,9 +1358,7 @@ public void realtime_retryableStatusCode_increasesConfigMetadataFailedStreams()
13631358
.when(configRealtimeHttpClientSpy)
13641359
.createRealtimeConnection();
13651360
doNothing().when(configRealtimeHttpClientSpy).retryHttpConnectionWhenBackoffEnds();
1366-
doNothing()
1367-
.when(configRealtimeHttpClientSpy)
1368-
.closeRealtimeHttpStream(any(HttpURLConnection.class));
1361+
doNothing().when(configRealtimeHttpClientSpy).closeRealtimeHttpStream();
13691362
when(mockHttpURLConnection.getResponseCode()).thenReturn(502);
13701363
int failedStreams = configRealtimeHttpClientSpy.getNumberOfFailedStreams();
13711364

@@ -1382,9 +1375,7 @@ public void realtime_retryableStatusCode_increasesConfigMetadataBackoffDate() th
13821375
.when(configRealtimeHttpClientSpy)
13831376
.createRealtimeConnection();
13841377
doNothing().when(configRealtimeHttpClientSpy).retryHttpConnectionWhenBackoffEnds();
1385-
doNothing()
1386-
.when(configRealtimeHttpClientSpy)
1387-
.closeRealtimeHttpStream(any(HttpURLConnection.class));
1378+
doNothing().when(configRealtimeHttpClientSpy).closeRealtimeHttpStream();
13881379
when(mockHttpURLConnection.getResponseCode()).thenReturn(502);
13891380
Date backoffDate = configRealtimeHttpClientSpy.getBackoffEndTime();
13901381

@@ -1403,9 +1394,7 @@ public void realtime_successfulStatusCode_doesNotIncreaseConfigMetadataFailedStr
14031394
.createRealtimeConnection();
14041395
doReturn(mockConfigAutoFetch).when(configRealtimeHttpClientSpy).startAutoFetch(any());
14051396
doNothing().when(configRealtimeHttpClientSpy).retryHttpConnectionWhenBackoffEnds();
1406-
doNothing()
1407-
.when(configRealtimeHttpClientSpy)
1408-
.closeRealtimeHttpStream(any(HttpURLConnection.class));
1397+
doNothing().when(configRealtimeHttpClientSpy).closeRealtimeHttpStream();
14091398
when(mockHttpURLConnection.getResponseCode()).thenReturn(200);
14101399
int failedStreams = configRealtimeHttpClientSpy.getNumberOfFailedStreams();
14111400

@@ -1424,9 +1413,7 @@ public void realtime_successfulStatusCode_doesNotIncreaseConfigMetadataBackoffDa
14241413
.createRealtimeConnection();
14251414
doReturn(mockConfigAutoFetch).when(configRealtimeHttpClientSpy).startAutoFetch(any());
14261415
doNothing().when(configRealtimeHttpClientSpy).retryHttpConnectionWhenBackoffEnds();
1427-
doNothing()
1428-
.when(configRealtimeHttpClientSpy)
1429-
.closeRealtimeHttpStream(any(HttpURLConnection.class));
1416+
doNothing().when(configRealtimeHttpClientSpy).closeRealtimeHttpStream();
14301417
when(mockHttpURLConnection.getResponseCode()).thenReturn(200);
14311418
Date backoffDate = configRealtimeHttpClientSpy.getBackoffEndTime();
14321419

@@ -1442,9 +1429,7 @@ public void realtime_forbiddenStatusCode_returnsStreamError() throws Exception {
14421429
doReturn(Tasks.forResult(mockHttpURLConnection))
14431430
.when(configRealtimeHttpClientSpy)
14441431
.createRealtimeConnection();
1445-
doNothing()
1446-
.when(configRealtimeHttpClientSpy)
1447-
.closeRealtimeHttpStream(any(HttpURLConnection.class));
1432+
doNothing().when(configRealtimeHttpClientSpy).closeRealtimeHttpStream();
14481433
when(mockHttpURLConnection.getErrorStream())
14491434
.thenReturn(
14501435
new ByteArrayInputStream(FORBIDDEN_ERROR_MESSAGE.getBytes(StandardCharsets.UTF_8)));
@@ -1465,9 +1450,7 @@ public void realtime_exceptionThrown_noAutofetchButRetries() throws Exception {
14651450
.when(configRealtimeHttpClientSpy)
14661451
.createRealtimeConnection();
14671452
doNothing().when(configRealtimeHttpClientSpy).retryHttpConnectionWhenBackoffEnds();
1468-
doNothing()
1469-
.when(configRealtimeHttpClientSpy)
1470-
.closeRealtimeHttpStream(any(HttpURLConnection.class));
1453+
doNothing().when(configRealtimeHttpClientSpy).closeRealtimeHttpStream();
14711454

14721455
configRealtimeHttpClientSpy.beginRealtimeHttpStream();
14731456
flushScheduledTasks();
@@ -1534,11 +1517,31 @@ public void realtime_stream_listen_get_inputstream_fail() throws Exception {
15341517
when(mockFetchHandler.fetchNowWithTypeAndAttemptNumber(
15351518
ConfigFetchHandler.FetchType.REALTIME, 1))
15361519
.thenReturn(Tasks.forResult(realtimeFetchedContainerResponse));
1520+
15371521
configAutoFetch.listenForNotifications();
15381522

15391523
verify(mockHttpURLConnection).disconnect();
15401524
}
15411525

1526+
@Test
1527+
public void realtime_stream_listen_backgrounded_disconnects() throws Exception {
1528+
when(mockHttpURLConnection.getResponseCode()).thenReturn(200);
1529+
when(mockHttpURLConnection.getInputStream())
1530+
.thenReturn(
1531+
new ByteArrayInputStream(
1532+
"{ \"featureDisabled\": false, \"latestTemplateVersionNumber\": 2 } }"
1533+
.getBytes(StandardCharsets.UTF_8)));
1534+
when(mockFetchHandler.getTemplateVersionNumber()).thenReturn(1L);
1535+
when(mockFetchHandler.fetchNowWithTypeAndAttemptNumber(
1536+
ConfigFetchHandler.FetchType.REALTIME, 1))
1537+
.thenReturn(Tasks.forResult(realtimeFetchedContainerResponse));
1538+
1539+
configAutoFetch.listenForNotifications();
1540+
frc.setConfigUpdateBackgroundState(true);
1541+
1542+
verify(mockHttpURLConnection, times(1)).disconnect();
1543+
}
1544+
15421545
@Test
15431546
public void realtime_stream_autofetch_success() throws Exception {
15441547
// Setup activated configs with keys "string_param", "long_param"

0 commit comments

Comments
 (0)