Realtime RC - Check HTTP status before listening and retrying #4121
Conversation
Coverage Report 1Affected Products
Test Logs |
Size Report 1Affected Products
Test Logs |
| if (!canMakeHttpStreamConnection()) { | ||
| return; | ||
| } | ||
|
|
||
| int responseCode = 200; |
There was a problem hiding this comment.
Would just leave this unset here (i.e int responseCode;) to ensure HTTP_OK means we really did get a response from the server
There was a problem hiding this comment.
Did this originally because of the edge case where the client isn't able to make a connection with the server(due to an exception or no connection) and doesn't get a status code back but initialized it to 0 instead and added a check for 0 too.
There was a problem hiding this comment.
How about checking responseCode == null or just !responseCode to indicate no response code was set (the connection wasn't made)? 0 is confusing since it's not a valid value
There was a problem hiding this comment.
Updated it to check for responseCode == null
| || statusCode == HttpURLConnection.HTTP_BAD_GATEWAY | ||
| || statusCode == HttpURLConnection.HTTP_UNAVAILABLE | ||
| || statusCode == HttpURLConnection.HTTP_GATEWAY_TIMEOUT | ||
| || statusCode == HttpURLConnection.HTTP_OK; |
There was a problem hiding this comment.
Can you add a comment here why we're retrying when we got an OK response since "retry" sounds like error handling logic. I'd even consider changing the condition below to split the success case out to avoid confusion: if (isStatusCodeRetryable(responseCode) || responseCode == HTTP_OK) { ... }
There was a problem hiding this comment.
Split the http_ok code out of isStatusCodeRetryable method
Check http response status code before retrying. Http status codes being used to retry are:
Ok 200
Client Timeout 408
Bad Gateway 502
Unavailable 503
Gateway timeout 504
Similar to what Fetch uses to throttle requests:
firebase-android-sdk/firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigFetchHandler.java
Close error stream