-
Notifications
You must be signed in to change notification settings - Fork 569
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.
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
Disconnect from RC real-time server when app is backgrounded #5865
base: main
Are you sure you want to change the base?
Changes from all commits
7f34ce3
a62df82
d347ec4
6011c94
e616ede
70ac1fa
448ba88
d58223e
ac2351a
f47a400
2a4b6ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,8 +98,10 @@ public class ConfigRealtimeHttpClient { | |
private boolean isRealtimeDisabled; | ||
|
||
/** Flag to indicate whether or not the app is in the background or not. */ | ||
private boolean isInBackground; | ||
|
||
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; | ||
|
@@ -391,14 +393,42 @@ public void run() { | |
} | ||
} | ||
|
||
void setRealtimeBackgroundState(boolean backgroundState) { | ||
isInBackground = backgroundState; | ||
public void setRealtimeBackgroundState(boolean backgroundState) { | ||
// Make changes in synchronized block so only one thread sets the background state and calls | ||
// disconnect. | ||
synchronized (isInBackground) { | ||
isInBackground = backgroundState; | ||
if (configAutoFetch != null) { | ||
configAutoFetch.setBackgroundState(backgroundState); | ||
} | ||
// Close the connection if the app is in the background and their is an active | ||
// HttpUrlConnection. | ||
if (isInBackground) { | ||
if (httpURLConnection != null) { | ||
httpURLConnection.disconnect(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this instance of HTTPUrlConnection would be (re)used after this point? The docs indicate it shouldn't be reused: https://developer.android.com/reference/java/net/HttpURLConnection#disconnect(). Wondering if there could be side-effects or errors from another thread still reading the connection There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No it won't be reused. We'll disconnect here and the thread that was listening on the connection (blocking at http://shortn/_q8UFRszL0G) will stop listening and and will exit beginRealtimeHttpStream resetting all the flags so that the next time beginRealtimeHttpStream is called a new connection will be used. |
||
} | ||
} | ||
} | ||
} | ||
|
||
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 +499,7 @@ private String parseForbiddenErrorResponseMessage(InputStream inputStream) { | |
*/ | ||
@SuppressLint({"VisibleForTests", "DefaultLocale"}) | ||
public void beginRealtimeHttpStream() { | ||
if (!canMakeHttpStreamConnection()) { | ||
if (!checkAndSetHttpConnectionFlagIfNotRunning()) { | ||
return; | ||
} | ||
|
||
|
@@ -489,17 +519,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 +543,31 @@ public void beginRealtimeHttpStream() { | |
metadataClient.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); | ||
|
||
boolean connectionFailed = | ||
responseCode == null || isStatusCodeRetryable(responseCode); | ||
!isInBackground | ||
&& (responseCode == null || isStatusCodeRetryable(responseCode)); | ||
if (connectionFailed) { | ||
updateBackoffMetadataWithLastFailedStreamConnectionTime( | ||
new Date(clock.currentTimeMillis())); | ||
|
@@ -542,8 +584,7 @@ public void beginRealtimeHttpStream() { | |
"Unable to connect to the server. Try again in a few minutes. HTTP status code: %d", | ||
responseCode); | ||
// Return server message for when the Firebase Remote Config Realtime API is | ||
// disabled and | ||
// the server returns a 403 | ||
// disabled and the server returns a 403 | ||
if (responseCode == 403) { | ||
errorMessage = | ||
parseForbiddenErrorResponseMessage(httpURLConnection.getErrorStream()); | ||
|
@@ -556,24 +597,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(); | ||
private void closeHttpConnectionInputStream(InputStream inputStream) { | ||
if (inputStream == null) { | ||
return; | ||
} | ||
|
||
// Explicitly close the input stream due to a bug in the Android okhttp implementation. | ||
// See github.com/firebase/firebase-android-sdk/pull/808. | ||
try { | ||
httpURLConnection.getInputStream().close(); | ||
if (httpURLConnection.getErrorStream() != null) { | ||
httpURLConnection.getErrorStream().close(); | ||
} | ||
} catch (IOException e) { | ||
} | ||
try { | ||
inputStream.close(); | ||
} catch (IOException ex) { | ||
Log.d(TAG, "Exception thrown when closing connection stream. Retrying connection...", ex); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we actually retry the connection in this case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah if it we made it to this point where the stream is not null and we're trying to close it, we reset (http://shortn/_XOhXyPPLvd) the retry count which should guarantee a retry |
||
} | ||
} | ||
|
||
// Pauses Http stream listening by disconnecting the HttpUrlConnection and underlying InputStream | ||
// and ErrorStream if they exist. | ||
@VisibleForTesting | ||
public void closeRealtimeHttpConnection(InputStream inputStream, InputStream errorStream) { | ||
if (httpURLConnection != null && !isInBackground) { | ||
httpURLConnection.disconnect(); | ||
} | ||
closeHttpConnectionInputStream(inputStream); | ||
closeHttpConnectionInputStream(errorStream); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As much as it's possible we should keep all the network calls required to connect/disconnect in the same place to avoid situations where one method is called but not the other. That's a bit challenging here since it's already split up some, but if it's possible I'd aim for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree it'd be better to keep in one area. I think we could do this in a separate PR where instead of passing HttpUrlConnection to ConfigAutoFetch, we pass the InputStream we pull out in ConfigHttpRealtimeClient.beginRealtimeHttpStream. I'll add this as a followup in the doc