-
Notifications
You must be signed in to change notification settings - Fork 624
Rc realtime dev #3743
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
Rc realtime dev #3743
Changes from all commits
488e8e7
fc01819
41898e7
a00ce0c
31af37a
187864b
a9d3a97
3642424
19956a8
93512af
fa56069
50da87e
99c166c
567aa14
4c16e92
e9e28b2
10fefb5
31777f5
51bf244
b5c4a96
7394492
c35e1bc
a4e4d1a
3630787
83c6d8f
4eca1dc
32e980a
527ae0c
a6f69c2
dd9d76f
9ae0be3
4ba117e
a22e5d3
ca5ce12
8334dfb
0bd4c2b
2bdb734
07488b4
2f9a558
8854eec
59c6ac8
d634bc8
bbb0d68
feed939
7d8e4a8
ba2867f
d9bc0cd
f8fdb8e
b4290aa
df35b7a
7522ddc
622db09
7cb6e69
5b230d7
a20e5ef
0f583a6
33ed2b2
41d69da
8d4335f
dd2c9d5
e1ec000
3865f23
4236508
f01c894
3d8519a
cdbd833
cd03a2e
7d5f792
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 |
---|---|---|
@@ -0,0 +1,37 @@ | ||
// Copyright 2022 Google LLC | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// | ||
// You may obtain a copy of the License at | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package com.google.firebase.remoteconfig; | ||
|
||
import androidx.annotation.NonNull; | ||
import androidx.annotation.Nullable; | ||
|
||
/** | ||
* A Firebase Remote Config exception caused by the auto-fetch component of Realtime config updates. | ||
* | ||
* @author Quan Pham | ||
*/ | ||
public class FirebaseRemoteConfigRealtimeUpdateFetchException | ||
extends FirebaseRemoteConfigException { | ||
/** Creates a Firebase Remote Config Realtime fetch exception with the given message. */ | ||
public FirebaseRemoteConfigRealtimeUpdateFetchException(@NonNull String detailMessage) { | ||
super(detailMessage); | ||
} | ||
|
||
/** Creates a Firebase Remote Config Realtime fetch exception with the given message and cause. */ | ||
public FirebaseRemoteConfigRealtimeUpdateFetchException( | ||
@NonNull String detailMessage, @Nullable Throwable cause) { | ||
super(detailMessage, cause); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
// Copyright 2022 Google LLC | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// | ||
// You may obtain a copy of the License at | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package com.google.firebase.remoteconfig; | ||
|
||
import androidx.annotation.NonNull; | ||
import androidx.annotation.Nullable; | ||
|
||
/** | ||
* A Firebase Remote Config exception caused by the stream component of Realtime config updates. | ||
* | ||
* @author Quan Pham | ||
*/ | ||
public class FirebaseRemoteConfigRealtimeUpdateStreamException | ||
extends FirebaseRemoteConfigException { | ||
/** Creates a Firebase Remote Config Realtime stream exception with the given message. */ | ||
public FirebaseRemoteConfigRealtimeUpdateStreamException(@NonNull String detailMessage) { | ||
super(detailMessage); | ||
} | ||
|
||
/** | ||
* Creates a Firebase Remote Config Realtime stream exception with the given message and cause. | ||
*/ | ||
public FirebaseRemoteConfigRealtimeUpdateStreamException( | ||
@NonNull String detailMessage, @Nullable Throwable cause) { | ||
super(detailMessage, cause); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,182 @@ | ||
// Copyright 2022 Google LLC | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// | ||
// You may obtain a copy of the License at | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package com.google.firebase.remoteconfig.internal; | ||
|
||
import static com.google.firebase.remoteconfig.FirebaseRemoteConfig.TAG; | ||
|
||
import android.util.Log; | ||
import androidx.annotation.GuardedBy; | ||
import androidx.annotation.VisibleForTesting; | ||
import com.google.android.gms.tasks.Task; | ||
import com.google.android.gms.tasks.Tasks; | ||
import com.google.firebase.remoteconfig.ConfigUpdateListener; | ||
import com.google.firebase.remoteconfig.FirebaseRemoteConfigException; | ||
import com.google.firebase.remoteconfig.FirebaseRemoteConfigRealtimeUpdateFetchException; | ||
import com.google.firebase.remoteconfig.FirebaseRemoteConfigRealtimeUpdateStreamException; | ||
import java.io.BufferedReader; | ||
import java.io.IOException; | ||
import java.io.InputStream; | ||
import java.io.InputStreamReader; | ||
import java.net.HttpURLConnection; | ||
import java.util.Random; | ||
import java.util.Set; | ||
import java.util.concurrent.Executor; | ||
import java.util.concurrent.ScheduledExecutorService; | ||
import java.util.concurrent.TimeUnit; | ||
import org.json.JSONException; | ||
import org.json.JSONObject; | ||
|
||
public class ConfigAutoFetch { | ||
|
||
private static final int FETCH_RETRY = 3; | ||
|
||
@GuardedBy("this") | ||
private final Set<ConfigUpdateListener> eventListeners; | ||
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. Shouldn't this be thread-safe? 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. Updated. And yeah, with the next PR that includes the retry methods we would do this for them already. This exception would be more of an indication to the user that Realtime won't be able to work at all due to the stream issue. This may enable them to revert to base RC as a fallback or some other mitigation where they are not reliant on realtime. |
||
|
||
@GuardedBy("this") | ||
private final HttpURLConnection httpURLConnection; | ||
|
||
private final ConfigFetchHandler configFetchHandler; | ||
private final ConfigUpdateListener retryCallback; | ||
private final ScheduledExecutorService scheduledExecutorService; | ||
private final Random random; | ||
private final Executor executor; | ||
|
||
public ConfigAutoFetch( | ||
HttpURLConnection httpURLConnection, | ||
ConfigFetchHandler configFetchHandler, | ||
Set<ConfigUpdateListener> eventListeners, | ||
ConfigUpdateListener retryCallback, | ||
Executor executor, | ||
ScheduledExecutorService scheduledExecutorService) { | ||
this.httpURLConnection = httpURLConnection; | ||
this.configFetchHandler = configFetchHandler; | ||
this.eventListeners = eventListeners; | ||
this.retryCallback = retryCallback; | ||
this.scheduledExecutorService = scheduledExecutorService; | ||
this.random = new Random(); | ||
this.executor = executor; | ||
} | ||
|
||
public void beginAutoFetch() { | ||
executor.execute( | ||
new Runnable() { | ||
@Override | ||
public void run() { | ||
listenForNotifications(); | ||
} | ||
}); | ||
} | ||
|
||
private synchronized void propagateErrors(FirebaseRemoteConfigException exception) { | ||
for (ConfigUpdateListener listener : eventListeners) { | ||
listener.onError(exception); | ||
} | ||
} | ||
|
||
private synchronized void executeAllListenerCallbacks() { | ||
for (ConfigUpdateListener listener : eventListeners) { | ||
listener.onEvent(); | ||
} | ||
} | ||
|
||
// Check connection and establish InputStream | ||
@VisibleForTesting | ||
public synchronized void listenForNotifications() { | ||
if (httpURLConnection != null) { | ||
try { | ||
int responseCode = httpURLConnection.getResponseCode(); | ||
if (responseCode == 200) { | ||
InputStream inputStream = httpURLConnection.getInputStream(); | ||
handleNotifications(inputStream); | ||
inputStream.close(); | ||
} else { | ||
propagateErrors( | ||
new FirebaseRemoteConfigRealtimeUpdateStreamException( | ||
"Http connection responded with error: " + responseCode)); | ||
} | ||
} catch (IOException ex) { | ||
propagateErrors( | ||
new FirebaseRemoteConfigRealtimeUpdateFetchException( | ||
"Error handling stream messages while fetching.", ex.getCause())); | ||
} | ||
} | ||
retryCallback.onEvent(); | ||
} | ||
|
||
// Auto-fetch new config and execute callbacks on each new message | ||
private void handleNotifications(InputStream inputStream) throws IOException { | ||
BufferedReader reader = new BufferedReader((new InputStreamReader(inputStream, "utf-8"))); | ||
String message; | ||
while ((message = reader.readLine()) != null) { | ||
long targetTemplateVersion = configFetchHandler.getTemplateVersionNumber(); | ||
try { | ||
JSONObject jsonObject = new JSONObject(message); | ||
if (jsonObject.has("latestTemplateVersionNumber")) { | ||
targetTemplateVersion = jsonObject.getLong("latestTemplateVersionNumber"); | ||
} | ||
} catch (JSONException ex) { | ||
Log.i(TAG, "Unable to parse latest config update message."); | ||
} | ||
|
||
autoFetch(FETCH_RETRY, targetTemplateVersion); | ||
} | ||
reader.close(); | ||
} | ||
|
||
private void autoFetch(int remainingAttempts, long targetVersion) { | ||
if (remainingAttempts == 0) { | ||
propagateErrors( | ||
new FirebaseRemoteConfigRealtimeUpdateFetchException("Unable to fetch latest version.")); | ||
return; | ||
} | ||
|
||
// Needs fetch to occur between 2 - 12 seconds. Randomize to not cause ddos alerts in backend | ||
int timeTillFetch = random.nextInt(6) + 1; | ||
scheduledExecutorService.schedule( | ||
new Runnable() { | ||
@Override | ||
public void run() { | ||
fetchLatestConfig(remainingAttempts, targetVersion); | ||
} | ||
}, | ||
timeTillFetch, | ||
TimeUnit.SECONDS); | ||
} | ||
|
||
@VisibleForTesting | ||
public synchronized void fetchLatestConfig(int remainingAttempts, long targetVersion) { | ||
Task<ConfigFetchHandler.FetchResponse> fetchTask = configFetchHandler.fetch(0L); | ||
fetchTask.onSuccessTask( | ||
(fetchResponse) -> { | ||
long newTemplateVersion = 0; | ||
if (fetchResponse.getFetchedConfigs() != null) { | ||
newTemplateVersion = fetchResponse.getFetchedConfigs().getTemplateVersionNumber(); | ||
} | ||
|
||
if (newTemplateVersion >= targetVersion) { | ||
executeAllListenerCallbacks(); | ||
} else { | ||
Log.i( | ||
TAG, | ||
"Fetched template version is the same as SDK's current version." | ||
+ " Retrying fetch."); | ||
// Continue fetching until template version number if greater then current. | ||
autoFetch(remainingAttempts - 1, targetVersion); | ||
} | ||
return Tasks.forResult(null); | ||
}); | ||
} | ||
} |
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.
What is the semantic difference between these 2 exceptions? Do we expect devs to understand this difference or is it part of the implementation detail?
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.
The difference is that the fetch exception is meant to indicate to the user that we got a new config update signal but we were unable to retrieve the latest config version that we were expecting. And the second one indicated that the stream was unable to open. We'll let users know about these exceptions so that they can expect them when writing their onError callback. We'll include these exceptions as part of our docs.
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.
Just for reference, do you have an example of different actions that would make sense to take depending on which exception is thrown?
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.
For the stream exception some things they could do is remove their listener, fetch again, or add a new listener to trigger another round of http stream retries.
And for the fetch exception users could try and fetch again or activate their default configs.
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.
This sounds like it should be done internally by the SDK as opposed to having devs do it manually, wdyt?
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.
And yeah, with the next PR that includes the retry methods we would do this for them already. This exception would be more of an indication to the user that Realtime won't be able to work at all due to the stream issue. This may enable them to revert to base RC as a fallback or some other mitigation where they are not reliant on realtime.
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.
What's still unclear to me is why would we expose this as an exception to the developer as opposed to retrying internally until we succeed?
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.
Mainly so user's can fall back onto something else and know when to not rely on Realtime. When we retry we're only going to have limited attempts b/c we don't want to use up the devices bandwidth/battery making an unlimited amount of retries. Especially in scenarios when our endpoint is down or the device has no network connection and so it won't successfully connect for a while. Do you think we should just fail quietly and not pass anything back if we can't establish a stream?
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.
Updated. PTAL @vkryachko