Disconnect from RC real-time server when app is backgrounded #5865
Conversation
Release note changesNo release note changes were detected. If you made changes that should be |
Generated by 🚫 Danger |
Coverage Report 1Affected Products
Test Logs |
Size Report 1Affected ProductsTest Logs |
Startup Time Report 1Note: Layout is sometimes suboptimal due to limited formatting support on GitHub. Please check this report on GCS. Notes
Startup Times
|
| try { | ||
| stream.close(); | ||
| } catch (IOException ex) { | ||
| Log.d(TAG, "Exception thrown when closing connection stream. Retrying connection...", ex); |
There was a problem hiding this comment.
Do we actually retry the connection in this case?
There was a problem hiding this comment.
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
...config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHttpClient.java
Outdated
Show resolved
Hide resolved
| void setRealtimeBackgroundState(boolean backgroundState) { | ||
| isInBackground = backgroundState; | ||
| public void setRealtimeBackgroundState(boolean backgroundState) { | ||
| // Make changes in synchronized block so that everything is updated in a single atomic |
There was a problem hiding this comment.
I don't think synchronized guarantees atomicity or transactionality (i.e. everything succeeds or fails together), only that no two threads modify the same variables at the same time. Is it important these operations happen in a transaction?
There was a problem hiding this comment.
Oh my bad, I wrote the description wrong. These operations don't need to happen in a transaction but within a synchronized block where only one thread should operate at a time. Updated the comment
| try { | ||
| // Only need to close the InputStream, ConfigRealtimeHttpClient will disconnect | ||
| // HttpUrlConnection | ||
| inputStream.close(); |
There was a problem hiding this comment.
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.
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
...config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigRealtimeHttpClient.java
Outdated
Show resolved
Hide resolved
| // HttpUrlConnection. | ||
| if (isInBackground) { | ||
| if (httpURLConnection != null) { | ||
| httpURLConnection.disconnect(); |
There was a problem hiding this comment.
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.
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.
Changes required to strongly close the Http stream connection when app is backgrounded:
HttpUrlConnectionandConfigAutoFetchinConfigRealtimeHttpClientso that they can be used to disconnect the current connection and propagate this background state changeisInBackgroundflag toConfigAutoFetchInputStreamwithinConfigRealtimeHttpClientandConfigAutoFetchso that we don't need to get the stream fromHttpUrlConnectionafter the connection has already been disconnectedgo/rc-realtime-close-background-connections