From f4be1e44a364b39ea0701fb5de0809a950cd9c54 Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Wed, 13 Feb 2019 17:23:24 -0800 Subject: [PATCH 01/19] Basic framework for HTTP retries --- .../internal/FirebaseRequestInitializer.java | 26 +++ .../firebase/internal/HttpRetryConfig.java | 96 +++++++++ .../firebase/internal/HttpRetryHandler.java | 75 +++++++ .../FirebaseRequestInitializerTest.java | 59 ++++- .../internal/HttpRetryConfigTest.java | 77 +++++++ .../internal/HttpRetryHandlerTest.java | 204 ++++++++++++++++++ ...OnlyImplRequestInitializerTrampolines.java | 27 +++ 7 files changed, 563 insertions(+), 1 deletion(-) create mode 100644 src/main/java/com/google/firebase/internal/HttpRetryConfig.java create mode 100644 src/main/java/com/google/firebase/internal/HttpRetryHandler.java create mode 100644 src/test/java/com/google/firebase/internal/HttpRetryConfigTest.java create mode 100644 src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java create mode 100644 src/test/java/com/google/firebase/internal/TestOnlyImplRequestInitializerTrampolines.java diff --git a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java index 0cd4e9393..f244f8b6c 100644 --- a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java +++ b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java @@ -20,9 +20,11 @@ import com.google.api.client.http.HttpRequestInitializer; import com.google.auth.http.HttpCredentialsAdapter; import com.google.auth.oauth2.GoogleCredentials; +import com.google.common.collect.ImmutableList; import com.google.firebase.FirebaseApp; import com.google.firebase.ImplFirebaseTrampolines; import java.io.IOException; +import java.util.concurrent.TimeUnit; /** * {@code HttpRequestInitializer} for configuring outgoing REST calls. Handles OAuth2 authorization @@ -30,13 +32,29 @@ */ public class FirebaseRequestInitializer implements HttpRequestInitializer { + private static final int STATUS_INTERNAL_SERVER_ERROR = 500; + private static final int STATUS_SERVICE_UNAVAILABLE = 503; + private static final HttpRetryConfig DEFAULT_RETRY_CONFIG = HttpRetryConfig.builder() + .setRetryStatusCodes(ImmutableList.of( + STATUS_INTERNAL_SERVER_ERROR, STATUS_SERVICE_UNAVAILABLE)) + .setMaxRetries(4) + .setMultiplier(2.0) + .setMaxIntervalInMillis((int) TimeUnit.MINUTES.toMillis(2)) + .build(); + private final HttpCredentialsAdapter credentialsAdapter; + private final HttpRetryConfig retryConfig; private final int connectTimeout; private final int readTimeout; public FirebaseRequestInitializer(FirebaseApp app) { + this(app, null); + } + + public FirebaseRequestInitializer(FirebaseApp app, @Nullable HttpRetryConfig retryConfig) { GoogleCredentials credentials = ImplFirebaseTrampolines.getCredentials(app); this.credentialsAdapter = new HttpCredentialsAdapter(credentials); + this.retryConfig = retryConfig; this.connectTimeout = app.getOptions().getConnectTimeout(); this.readTimeout = app.getOptions().getReadTimeout(); } @@ -46,5 +64,13 @@ public void initialize(HttpRequest httpRequest) throws IOException { credentialsAdapter.initialize(httpRequest); httpRequest.setConnectTimeout(connectTimeout); httpRequest.setReadTimeout(readTimeout); + if (retryConfig != null) { + httpRequest.setNumberOfRetries(retryConfig.getMaxRetries()); + HttpRetryHandler retryHandler = new HttpRetryHandler(credentialsAdapter, retryConfig); + httpRequest.setUnsuccessfulResponseHandler(retryHandler); + httpRequest.setIOExceptionHandler(retryHandler); + } else { + httpRequest.setNumberOfRetries(0); + } } } diff --git a/src/main/java/com/google/firebase/internal/HttpRetryConfig.java b/src/main/java/com/google/firebase/internal/HttpRetryConfig.java new file mode 100644 index 000000000..ae42062e6 --- /dev/null +++ b/src/main/java/com/google/firebase/internal/HttpRetryConfig.java @@ -0,0 +1,96 @@ +/* + * Copyright 2019 Google Inc. + * + * 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.internal; + +import static com.google.common.base.Preconditions.checkArgument; + +import com.google.api.client.util.BackOff; +import com.google.api.client.util.ExponentialBackOff; +import com.google.common.collect.ImmutableList; +import java.util.List; +import java.util.concurrent.TimeUnit; + +public final class HttpRetryConfig { + + private final List retryStatusCodes; + private final int maxRetries; + private final ExponentialBackOff.Builder backOffBuilder; + + private HttpRetryConfig(Builder builder) { + if (builder.retryStatusCodes != null) { + this.retryStatusCodes = ImmutableList.copyOf(builder.retryStatusCodes); + } else { + this.retryStatusCodes = ImmutableList.of(); + } + checkArgument(builder.maxRetries >= 0, "maxRetries must not be negative"); + this.maxRetries = builder.maxRetries; + this.backOffBuilder = new ExponentialBackOff.Builder() + .setMaxIntervalMillis(builder.maxIntervalInMillis) + .setMultiplier(builder.multiplier) + .setRandomizationFactor(0); + } + + BackOff newBackoff() { + return backOffBuilder.build(); + } + + int getMaxRetries() { + return maxRetries; + } + + List getRetryStatusCodes() { + return retryStatusCodes; + } + + public static Builder builder() { + return new Builder(); + } + + public static final class Builder { + + private List retryStatusCodes; + private int maxRetries; + private int maxIntervalInMillis = (int) TimeUnit.MINUTES.toMillis(2); + private double multiplier = 2.0; + + private Builder() { } + + public Builder setRetryStatusCodes(List retryStatusCodes) { + this.retryStatusCodes = retryStatusCodes; + return this; + } + + public Builder setMaxRetries(int maxRetries) { + this.maxRetries = maxRetries; + return this; + } + + public Builder setMaxIntervalInMillis(int maxIntervalInMillis) { + this.maxIntervalInMillis = maxIntervalInMillis; + return this; + } + + public Builder setMultiplier(double multiplier) { + this.multiplier = multiplier; + return this; + } + + public HttpRetryConfig build() { + return new HttpRetryConfig(this); + } + } +} diff --git a/src/main/java/com/google/firebase/internal/HttpRetryHandler.java b/src/main/java/com/google/firebase/internal/HttpRetryHandler.java new file mode 100644 index 000000000..534472fba --- /dev/null +++ b/src/main/java/com/google/firebase/internal/HttpRetryHandler.java @@ -0,0 +1,75 @@ +/* + * Copyright 2019 Google Inc. + * + * 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.internal; + +import static com.google.common.base.Preconditions.checkNotNull; + +import com.google.api.client.http.HttpBackOffIOExceptionHandler; +import com.google.api.client.http.HttpBackOffUnsuccessfulResponseHandler; +import com.google.api.client.http.HttpIOExceptionHandler; +import com.google.api.client.http.HttpRequest; +import com.google.api.client.http.HttpResponse; +import com.google.api.client.http.HttpUnsuccessfulResponseHandler; +import com.google.api.client.util.Sleeper; +import com.google.auth.http.HttpCredentialsAdapter; +import java.io.IOException; + +final class HttpRetryHandler implements HttpUnsuccessfulResponseHandler, HttpIOExceptionHandler { + + static Sleeper SLEEPER = Sleeper.DEFAULT; + + private final HttpCredentialsAdapter credentials; + private final HttpRetryConfig retryConfig; + private final HttpIOExceptionHandler ioExceptionHandler; + private final HttpUnsuccessfulResponseHandler responseHandler; + + public HttpRetryHandler(HttpCredentialsAdapter credentials, HttpRetryConfig retryConfig) { + this.credentials = checkNotNull(credentials); + this.retryConfig = checkNotNull(retryConfig); + this.ioExceptionHandler = new HttpBackOffIOExceptionHandler(retryConfig.newBackoff()) + .setSleeper(SLEEPER); + this.responseHandler = new HttpBackOffUnsuccessfulResponseHandler(retryConfig.newBackoff()) + .setSleeper(SLEEPER); + } + + @Override + public boolean handleIOException(HttpRequest request, boolean supportsRetry) throws IOException { + return ioExceptionHandler.handleIOException(request, supportsRetry); + } + + @Override + public boolean handleResponse(HttpRequest request, HttpResponse response, boolean supportsRetry) + throws IOException { + boolean retry = credentials.handleResponse(request, response, supportsRetry); + if (!retry) { + int status = response.getStatusCode(); + if (retryConfig.getRetryStatusCodes().contains(status)) { + retry = responseHandler.handleResponse(request, response, supportsRetry); + } + } + request.setUnsuccessfulResponseHandler(this); + return retry; + } + + HttpIOExceptionHandler getIoExceptionHandler() { + return ioExceptionHandler; + } + + HttpUnsuccessfulResponseHandler getResponseHandler() { + return responseHandler; + } +} diff --git a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java index c7919eb97..d6024a54f 100644 --- a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java @@ -17,12 +17,16 @@ package com.google.firebase.internal; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; import com.google.api.client.http.GenericUrl; import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpRequestFactory; import com.google.api.client.http.HttpTransport; import com.google.api.client.testing.http.MockHttpTransport; +import com.google.auth.http.HttpCredentialsAdapter; import com.google.firebase.FirebaseApp; import com.google.firebase.FirebaseOptions; import com.google.firebase.TestOnlyImplFirebaseTrampolines; @@ -38,18 +42,23 @@ public void tearDown() { } @Test - public void testDefaultTimeouts() throws Exception { + public void testDefaultSettings() throws Exception { FirebaseApp app = FirebaseApp.initializeApp(new FirebaseOptions.Builder() .setCredentials(new MockGoogleCredentials("token")) .build()); HttpTransport transport = new MockHttpTransport(); HttpRequestFactory factory = transport.createRequestFactory( new FirebaseRequestInitializer(app)); + HttpRequest request = factory.buildGetRequest( new GenericUrl("https://firebase.google.com")); + assertEquals(0, request.getConnectTimeout()); assertEquals(0, request.getReadTimeout()); assertEquals("Bearer token", request.getHeaders().getAuthorization()); + // assertEquals(4, request.getNumberOfRetries()); + // assertTrue(request.getIOExceptionHandler() instanceof HttpRetryHandler); + // assertTrue(request.getUnsuccessfulResponseHandler() instanceof HttpRetryHandler); } @Test @@ -62,10 +71,58 @@ public void testExplicitTimeouts() throws Exception { HttpTransport transport = new MockHttpTransport(); HttpRequestFactory factory = transport.createRequestFactory( new FirebaseRequestInitializer(app)); + HttpRequest request = factory.buildGetRequest( new GenericUrl("https://firebase.google.com")); + assertEquals(30000, request.getConnectTimeout()); assertEquals(60000, request.getReadTimeout()); assertEquals("Bearer token", request.getHeaders().getAuthorization()); + // assertEquals(4, request.getNumberOfRetries()); + // assertTrue(request.getIOExceptionHandler() instanceof HttpRetryHandler); + // assertTrue(request.getUnsuccessfulResponseHandler() instanceof HttpRetryHandler); + } + + @Test + public void testNullRetryConfig() throws Exception { + FirebaseApp app = FirebaseApp.initializeApp(new FirebaseOptions.Builder() + .setCredentials(new MockGoogleCredentials("token")) + .build()); + HttpTransport transport = new MockHttpTransport(); + HttpRequestFactory factory = transport.createRequestFactory( + new FirebaseRequestInitializer(app, null)); + + HttpRequest request = factory.buildGetRequest( + new GenericUrl("https://firebase.google.com")); + + assertEquals(0, request.getConnectTimeout()); + assertEquals(0, request.getReadTimeout()); + assertEquals("Bearer token", request.getHeaders().getAuthorization()); + assertEquals(0, request.getNumberOfRetries()); + assertNull(request.getIOExceptionHandler()); + assertTrue(request.getUnsuccessfulResponseHandler() instanceof HttpCredentialsAdapter); + } + + @Test + public void testExplicitRetryConfig() throws Exception { + FirebaseApp app = FirebaseApp.initializeApp(new FirebaseOptions.Builder() + .setCredentials(new MockGoogleCredentials("token")) + .build()); + HttpTransport transport = new MockHttpTransport(); + HttpRetryConfig retryConfig = HttpRetryConfig.builder() + .setMaxRetries(5) + .build(); + HttpRequestFactory factory = transport.createRequestFactory( + new FirebaseRequestInitializer(app, retryConfig)); + + HttpRequest request = factory.buildGetRequest( + new GenericUrl("https://firebase.google.com")); + + assertEquals(0, request.getConnectTimeout()); + assertEquals(0, request.getReadTimeout()); + assertEquals("Bearer token", request.getHeaders().getAuthorization()); + assertEquals(5, request.getNumberOfRetries()); + assertTrue(request.getIOExceptionHandler() instanceof HttpRetryHandler); + assertTrue(request.getUnsuccessfulResponseHandler() instanceof HttpRetryHandler); } } diff --git a/src/test/java/com/google/firebase/internal/HttpRetryConfigTest.java b/src/test/java/com/google/firebase/internal/HttpRetryConfigTest.java new file mode 100644 index 000000000..77b317f57 --- /dev/null +++ b/src/test/java/com/google/firebase/internal/HttpRetryConfigTest.java @@ -0,0 +1,77 @@ +/* + * Copyright 2019 Google Inc. + * + * 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.internal; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertTrue; + +import com.google.api.client.util.ExponentialBackOff; +import com.google.common.collect.ImmutableList; +import java.io.IOException; +import org.junit.Test; + +public class HttpRetryConfigTest { + + @Test + public void testEmptyBuilder() { + HttpRetryConfig config = HttpRetryConfig.builder().build(); + + assertTrue(config.getRetryStatusCodes().isEmpty()); + assertEquals(0, config.getMaxRetries()); + ExponentialBackOff backoff = (ExponentialBackOff) config.newBackoff(); + assertEquals(500, backoff.getInitialIntervalMillis()); + assertEquals(2 * 60 * 1000, backoff.getMaxIntervalMillis()); + assertEquals(2.0, backoff.getMultiplier(), 0.01); + assertEquals(0.0, backoff.getRandomizationFactor(), 0.01); + assertNotSame(backoff, config.newBackoff()); + } + + @Test + public void testBuilder() { + ImmutableList statusCodes = ImmutableList.of(500, 503); + HttpRetryConfig config = HttpRetryConfig.builder() + .setMaxRetries(4) + .setRetryStatusCodes(statusCodes) + .setMaxIntervalInMillis(5 * 60 * 1000) + .setMultiplier(1.5) + .build(); + + assertEquals(2, config.getRetryStatusCodes().size()); + assertEquals(statusCodes.get(0), config.getRetryStatusCodes().get(0)); + assertEquals(statusCodes.get(1), config.getRetryStatusCodes().get(1)); + assertEquals(4, config.getMaxRetries()); + ExponentialBackOff backoff = (ExponentialBackOff) config.newBackoff(); + assertEquals(500, backoff.getInitialIntervalMillis()); + assertEquals(5 * 60 * 1000, backoff.getMaxIntervalMillis()); + assertEquals(1.5, backoff.getMultiplier(), 0.01); + assertEquals(0.0, backoff.getRandomizationFactor(), 0.01); + assertNotSame(backoff, config.newBackoff()); + } + + @Test + public void testExponentialBackoff() throws IOException { + HttpRetryConfig config = HttpRetryConfig.builder().build(); + + ExponentialBackOff backoff = (ExponentialBackOff) config.newBackoff(); + + assertEquals(500, backoff.nextBackOffMillis()); + assertEquals(1000, backoff.nextBackOffMillis()); + assertEquals(2000, backoff.nextBackOffMillis()); + assertEquals(4000, backoff.nextBackOffMillis()); + } +} diff --git a/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java b/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java new file mode 100644 index 000000000..8f7355e55 --- /dev/null +++ b/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java @@ -0,0 +1,204 @@ +/* + * Copyright 2019 Google Inc. + * + * 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.internal; + +import static com.google.common.base.Preconditions.checkNotNull; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +import com.google.api.client.http.EmptyContent; +import com.google.api.client.http.GenericUrl; +import com.google.api.client.http.HttpBackOffIOExceptionHandler; +import com.google.api.client.http.HttpBackOffUnsuccessfulResponseHandler; +import com.google.api.client.http.HttpRequest; +import com.google.api.client.http.HttpRequestFactory; +import com.google.api.client.http.HttpResponse; +import com.google.api.client.http.HttpResponseException; +import com.google.api.client.http.HttpTransport; +import com.google.api.client.http.LowLevelHttpResponse; +import com.google.api.client.testing.http.MockHttpTransport; +import com.google.api.client.testing.http.MockLowLevelHttpRequest; +import com.google.api.client.testing.http.MockLowLevelHttpResponse; +import com.google.api.client.testing.util.MockSleeper; +import com.google.auth.http.HttpCredentialsAdapter; +import com.google.auth.oauth2.GoogleCredentials; +import com.google.common.collect.ImmutableList; +import com.google.firebase.auth.MockGoogleCredentials; +import java.io.IOException; +import org.junit.Test; + +public class HttpRetryHandlerTest { + + private static final GenericUrl TEST_URL = new GenericUrl("https://firebase.google.com"); + private static final GoogleCredentials TEST_CREDENTIALS = new MockGoogleCredentials(); + + @Test + public void testRetryOnIOException() throws IOException { + CountingHttpRequest failingRequest = CountingHttpRequest.fromException( + new IOException("test error")); + HttpRequest request = createRequest(failingRequest); + HttpRetryHandler retryHandler = new HttpRetryHandler( + new HttpCredentialsAdapter(TEST_CREDENTIALS), + HttpRetryConfig.builder().build()); + MockSleeper sleeper = new MockSleeper(); + ((HttpBackOffIOExceptionHandler) retryHandler.getIoExceptionHandler()).setSleeper(sleeper); + request.setNumberOfRetries(4); + request.setIOExceptionHandler(retryHandler); + + try { + request.execute(); + fail("No exception thrown for transport error"); + } catch (IOException e) { + assertEquals("test error", e.getMessage()); + } + + assertEquals(4, sleeper.getCount()); + assertEquals(5, failingRequest.getCount()); + } + + @Test + public void testRetryOnHttpError() throws IOException { + CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + new MockLowLevelHttpResponse().setStatusCode(503).setZeroContent()); + HttpRequest request = createRequest(failingRequest); + HttpRetryHandler retryHandler = new HttpRetryHandler( + new HttpCredentialsAdapter(TEST_CREDENTIALS), + HttpRetryConfig.builder().setRetryStatusCodes(ImmutableList.of(503)).build()); + MockSleeper sleeper = new MockSleeper(); + ((HttpBackOffUnsuccessfulResponseHandler) retryHandler.getResponseHandler()) + .setSleeper(sleeper); + request.setNumberOfRetries(4); + request.setUnsuccessfulResponseHandler(retryHandler); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (HttpResponseException e) { + assertEquals(503, e.getStatusCode()); + } + + assertEquals(4, sleeper.getCount()); + assertEquals(5, failingRequest.getCount()); + } + + @Test + public void testDoesNotRetryOnUnspecifiedHttpError() throws IOException { + CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + new MockLowLevelHttpResponse().setStatusCode(404).setZeroContent()); + HttpRequest request = createRequest(failingRequest); + HttpRetryHandler retryHandler = new HttpRetryHandler( + new HttpCredentialsAdapter(TEST_CREDENTIALS), + HttpRetryConfig.builder().setRetryStatusCodes(ImmutableList.of(503)).build()); + MockSleeper sleeper = new MockSleeper(); + ((HttpBackOffUnsuccessfulResponseHandler) retryHandler.getResponseHandler()) + .setSleeper(sleeper); + request.setNumberOfRetries(4); + request.setUnsuccessfulResponseHandler(retryHandler); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (HttpResponseException e) { + assertEquals(404, e.getStatusCode()); + } + + assertEquals(0, sleeper.getCount()); + assertEquals(1, failingRequest.getCount()); + } + + @Test + public void testRetryCredentialsCheck() throws IOException { + CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + new MockLowLevelHttpResponse().setStatusCode(401).setZeroContent()); + HttpRequest request = createRequest(failingRequest); + HttpCredentialsAdapter credentials = new HttpCredentialsAdapter(TEST_CREDENTIALS){ + @Override + public boolean handleResponse( + HttpRequest request, HttpResponse response, boolean supportsRetry) { + String authorization = request.getHeaders().getAuthorization(); + if (!"Bearer retry".equals(authorization)) { + request.getHeaders().setAuthorization("Bearer retry"); + return true; + } + return false; + } + }; + HttpRetryHandler retryHandler = new HttpRetryHandler( + credentials, + HttpRetryConfig.builder().setRetryStatusCodes(ImmutableList.of(503)).build()); + MockSleeper sleeper = new MockSleeper(); + ((HttpBackOffUnsuccessfulResponseHandler) retryHandler.getResponseHandler()) + .setSleeper(sleeper); + request.setNumberOfRetries(4); + request.setUnsuccessfulResponseHandler(retryHandler); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (HttpResponseException e) { + assertEquals(401, e.getStatusCode()); + } + + assertEquals("Bearer retry", request.getHeaders().getAuthorization()); + assertEquals(0, sleeper.getCount()); + assertEquals(2, failingRequest.getCount()); + } + + private HttpRequest createRequest(MockLowLevelHttpRequest request) throws IOException { + HttpTransport transport = new MockHttpTransport.Builder() + .setLowLevelHttpRequest(request) + .build(); + HttpRequestFactory requestFactory = transport.createRequestFactory(); + return requestFactory.buildPostRequest(TEST_URL, new EmptyContent()); + } + + private static class CountingHttpRequest extends MockLowLevelHttpRequest { + + private final LowLevelHttpResponse response; + private final IOException exception; + private int count; + + private CountingHttpRequest(LowLevelHttpResponse response, IOException exception) { + this.response = response; + this.exception = exception; + } + + static CountingHttpRequest fromResponse(LowLevelHttpResponse response) { + return new CountingHttpRequest(checkNotNull(response), null); + } + + static CountingHttpRequest fromException(IOException exception) { + return new CountingHttpRequest(null, checkNotNull(exception)); + } + + @Override + public void addHeader(String name, String value) { } + + @Override + public LowLevelHttpResponse execute() throws IOException { + count++; + if (response != null) { + return response; + } + throw exception; + } + + int getCount() { + return count; + } + } +} diff --git a/src/test/java/com/google/firebase/internal/TestOnlyImplRequestInitializerTrampolines.java b/src/test/java/com/google/firebase/internal/TestOnlyImplRequestInitializerTrampolines.java new file mode 100644 index 000000000..4a03d20d2 --- /dev/null +++ b/src/test/java/com/google/firebase/internal/TestOnlyImplRequestInitializerTrampolines.java @@ -0,0 +1,27 @@ +/* + * Copyright 2019 Google Inc. + * + * 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.internal; + +import com.google.api.client.testing.util.MockSleeper; + +public class TestOnlyImplRequestInitializerTrampolines { + + public static void disableRetryDelays() { + HttpRetryHandler.SLEEPER = new MockSleeper(); + } + +} From a426fe675f07f1016602c2336972fbe298ed2c2a Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Thu, 14 Feb 2019 17:41:15 -0800 Subject: [PATCH 02/19] Implementing support for retry-after header --- .../internal/FirebaseRequestInitializer.java | 4 +- .../firebase/internal/HttpRetryConfig.java | 43 ++-- .../firebase/internal/HttpRetryHandler.java | 17 +- .../RetryAfterAwareHttpResponseHandler.java | 91 +++++++ .../internal/HttpRetryConfigTest.java | 33 ++- .../internal/HttpRetryHandlerTest.java | 7 +- ...etryAfterAwareHttpResponseHandlerTest.java | 223 ++++++++++++++++++ ...OnlyImplRequestInitializerTrampolines.java | 27 --- 8 files changed, 384 insertions(+), 61 deletions(-) create mode 100644 src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java create mode 100644 src/test/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandlerTest.java delete mode 100644 src/test/java/com/google/firebase/internal/TestOnlyImplRequestInitializerTrampolines.java diff --git a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java index f244f8b6c..04a78b532 100644 --- a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java +++ b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java @@ -38,8 +38,8 @@ public class FirebaseRequestInitializer implements HttpRequestInitializer { .setRetryStatusCodes(ImmutableList.of( STATUS_INTERNAL_SERVER_ERROR, STATUS_SERVICE_UNAVAILABLE)) .setMaxRetries(4) - .setMultiplier(2.0) - .setMaxIntervalInMillis((int) TimeUnit.MINUTES.toMillis(2)) + .setBackoffMultiplier(2.0) + .setMaxIntervalMillis((int) TimeUnit.MINUTES.toMillis(2)) .build(); private final HttpCredentialsAdapter credentialsAdapter; diff --git a/src/main/java/com/google/firebase/internal/HttpRetryConfig.java b/src/main/java/com/google/firebase/internal/HttpRetryConfig.java index ae42062e6..7f8d3c1d7 100644 --- a/src/main/java/com/google/firebase/internal/HttpRetryConfig.java +++ b/src/main/java/com/google/firebase/internal/HttpRetryConfig.java @@ -26,9 +26,11 @@ public final class HttpRetryConfig { + private static final int INITIAL_INTERVAL_MILLIS = 500; + private final List retryStatusCodes; private final int maxRetries; - private final ExponentialBackOff.Builder backOffBuilder; + private final ExponentialBackOff.Builder backoffBuilder; private HttpRetryConfig(Builder builder) { if (builder.retryStatusCodes != null) { @@ -36,24 +38,37 @@ private HttpRetryConfig(Builder builder) { } else { this.retryStatusCodes = ImmutableList.of(); } + checkArgument(builder.maxRetries >= 0, "maxRetries must not be negative"); this.maxRetries = builder.maxRetries; - this.backOffBuilder = new ExponentialBackOff.Builder() - .setMaxIntervalMillis(builder.maxIntervalInMillis) - .setMultiplier(builder.multiplier) + this.backoffBuilder = new ExponentialBackOff.Builder() + .setInitialIntervalMillis(INITIAL_INTERVAL_MILLIS) + .setMaxIntervalMillis(builder.maxIntervalMillis) + .setMultiplier(builder.backoffMultiplier) .setRandomizationFactor(0); + + // Force validation of arguments by building the BackOff object + this.backoffBuilder.build(); } - BackOff newBackoff() { - return backOffBuilder.build(); + List getRetryStatusCodes() { + return retryStatusCodes; } int getMaxRetries() { return maxRetries; } - List getRetryStatusCodes() { - return retryStatusCodes; + int getMaxIntervalMillis() { + return backoffBuilder.getMaxIntervalMillis(); + } + + double getBackoffMultiplier() { + return backoffBuilder.getMultiplier(); + } + + BackOff newBackoff() { + return backoffBuilder.build(); } public static Builder builder() { @@ -64,8 +79,8 @@ public static final class Builder { private List retryStatusCodes; private int maxRetries; - private int maxIntervalInMillis = (int) TimeUnit.MINUTES.toMillis(2); - private double multiplier = 2.0; + private int maxIntervalMillis = (int) TimeUnit.MINUTES.toMillis(2); + private double backoffMultiplier = 2.0; private Builder() { } @@ -79,13 +94,13 @@ public Builder setMaxRetries(int maxRetries) { return this; } - public Builder setMaxIntervalInMillis(int maxIntervalInMillis) { - this.maxIntervalInMillis = maxIntervalInMillis; + public Builder setMaxIntervalMillis(int maxIntervalMillis) { + this.maxIntervalMillis = maxIntervalMillis; return this; } - public Builder setMultiplier(double multiplier) { - this.multiplier = multiplier; + public Builder setBackoffMultiplier(double backoffMultiplier) { + this.backoffMultiplier = backoffMultiplier; return this; } diff --git a/src/main/java/com/google/firebase/internal/HttpRetryHandler.java b/src/main/java/com/google/firebase/internal/HttpRetryHandler.java index 534472fba..0f42f0fd9 100644 --- a/src/main/java/com/google/firebase/internal/HttpRetryHandler.java +++ b/src/main/java/com/google/firebase/internal/HttpRetryHandler.java @@ -19,31 +19,25 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.api.client.http.HttpBackOffIOExceptionHandler; -import com.google.api.client.http.HttpBackOffUnsuccessfulResponseHandler; import com.google.api.client.http.HttpIOExceptionHandler; import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpResponse; import com.google.api.client.http.HttpUnsuccessfulResponseHandler; -import com.google.api.client.util.Sleeper; import com.google.auth.http.HttpCredentialsAdapter; import java.io.IOException; final class HttpRetryHandler implements HttpUnsuccessfulResponseHandler, HttpIOExceptionHandler { - static Sleeper SLEEPER = Sleeper.DEFAULT; - private final HttpCredentialsAdapter credentials; private final HttpRetryConfig retryConfig; private final HttpIOExceptionHandler ioExceptionHandler; private final HttpUnsuccessfulResponseHandler responseHandler; - public HttpRetryHandler(HttpCredentialsAdapter credentials, HttpRetryConfig retryConfig) { + HttpRetryHandler(HttpCredentialsAdapter credentials, HttpRetryConfig retryConfig) { this.credentials = checkNotNull(credentials); this.retryConfig = checkNotNull(retryConfig); - this.ioExceptionHandler = new HttpBackOffIOExceptionHandler(retryConfig.newBackoff()) - .setSleeper(SLEEPER); - this.responseHandler = new HttpBackOffUnsuccessfulResponseHandler(retryConfig.newBackoff()) - .setSleeper(SLEEPER); + this.ioExceptionHandler = new HttpBackOffIOExceptionHandler(retryConfig.newBackoff()); + this.responseHandler = new RetryAfterAwareHttpResponseHandler(retryConfig); } @Override @@ -52,8 +46,9 @@ public boolean handleIOException(HttpRequest request, boolean supportsRetry) thr } @Override - public boolean handleResponse(HttpRequest request, HttpResponse response, boolean supportsRetry) - throws IOException { + public boolean handleResponse( + HttpRequest request, HttpResponse response, boolean supportsRetry) throws IOException { + boolean retry = credentials.handleResponse(request, response, supportsRetry); if (!retry) { int status = response.getStatusCode(); diff --git a/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java b/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java new file mode 100644 index 000000000..f688d7aff --- /dev/null +++ b/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java @@ -0,0 +1,91 @@ +/* + * Copyright 2019 Google Inc. + * + * 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.internal; + +import static com.google.common.base.Preconditions.checkNotNull; + +import com.google.api.client.http.HttpBackOffUnsuccessfulResponseHandler; +import com.google.api.client.http.HttpRequest; +import com.google.api.client.http.HttpResponse; +import com.google.api.client.http.HttpUnsuccessfulResponseHandler; +import com.google.api.client.util.Clock; +import com.google.api.client.util.Sleeper; +import com.google.common.base.Strings; +import java.io.IOException; +import java.util.Date; +import org.apache.http.client.utils.DateUtils; + +final class RetryAfterAwareHttpResponseHandler implements HttpUnsuccessfulResponseHandler { + + private final HttpRetryConfig retryConfig; + private final HttpBackOffUnsuccessfulResponseHandler backoffHandler; + private final Clock clock; + + RetryAfterAwareHttpResponseHandler(HttpRetryConfig retryConfig) { + this(retryConfig, Clock.SYSTEM); + } + + RetryAfterAwareHttpResponseHandler(HttpRetryConfig retryConfig, Clock clock) { + this.retryConfig = checkNotNull(retryConfig); + this.backoffHandler = new HttpBackOffUnsuccessfulResponseHandler(retryConfig.newBackoff()); + this.clock = checkNotNull(clock); + } + + void setSleeper(Sleeper sleeper) { + backoffHandler.setSleeper(sleeper); + } + + @Override + public boolean handleResponse( + HttpRequest request, HttpResponse response, boolean supportsRetry) throws IOException { + + if (!supportsRetry) { + return false; + } + + String retryAfter = response.getHeaders().getRetryAfter(); + if (!Strings.isNullOrEmpty(retryAfter)) { + long delayMillis = parseRetryAfter(retryAfter.trim()); + if (delayMillis > retryConfig.getMaxIntervalMillis()) { + return false; + } + + if (delayMillis > 0) { + try { + backoffHandler.getSleeper().sleep(delayMillis); + } catch (InterruptedException e) { + // ignore + } + } + return true; + } + + return backoffHandler.handleResponse(request, response, true); + } + + private long parseRetryAfter(String retryAfter) { + try { + return Long.parseLong(retryAfter.trim()) * 1000; + } catch (NumberFormatException e) { + Date date = DateUtils.parseDate(retryAfter); + if (date != null) { + return date.getTime() - clock.currentTimeMillis(); + } + } + return 0L; + } +} diff --git a/src/test/java/com/google/firebase/internal/HttpRetryConfigTest.java b/src/test/java/com/google/firebase/internal/HttpRetryConfigTest.java index 77b317f57..de4ead19e 100644 --- a/src/test/java/com/google/firebase/internal/HttpRetryConfigTest.java +++ b/src/test/java/com/google/firebase/internal/HttpRetryConfigTest.java @@ -33,10 +33,13 @@ public void testEmptyBuilder() { assertTrue(config.getRetryStatusCodes().isEmpty()); assertEquals(0, config.getMaxRetries()); + assertEquals(2 * 60 * 1000, config.getMaxIntervalMillis()); + assertEquals(2.0, config.getBackoffMultiplier(), 0.01); + ExponentialBackOff backoff = (ExponentialBackOff) config.newBackoff(); - assertEquals(500, backoff.getInitialIntervalMillis()); assertEquals(2 * 60 * 1000, backoff.getMaxIntervalMillis()); assertEquals(2.0, backoff.getMultiplier(), 0.01); + assertEquals(500, backoff.getInitialIntervalMillis()); assertEquals(0.0, backoff.getRandomizationFactor(), 0.01); assertNotSame(backoff, config.newBackoff()); } @@ -47,14 +50,17 @@ public void testBuilder() { HttpRetryConfig config = HttpRetryConfig.builder() .setMaxRetries(4) .setRetryStatusCodes(statusCodes) - .setMaxIntervalInMillis(5 * 60 * 1000) - .setMultiplier(1.5) + .setMaxIntervalMillis(5 * 60 * 1000) + .setBackoffMultiplier(1.5) .build(); assertEquals(2, config.getRetryStatusCodes().size()); assertEquals(statusCodes.get(0), config.getRetryStatusCodes().get(0)); assertEquals(statusCodes.get(1), config.getRetryStatusCodes().get(1)); assertEquals(4, config.getMaxRetries()); + assertEquals(5 * 60 * 1000, config.getMaxIntervalMillis()); + assertEquals(1.5, config.getBackoffMultiplier(), 0.01); + ExponentialBackOff backoff = (ExponentialBackOff) config.newBackoff(); assertEquals(500, backoff.getInitialIntervalMillis()); assertEquals(5 * 60 * 1000, backoff.getMaxIntervalMillis()); @@ -74,4 +80,25 @@ public void testExponentialBackoff() throws IOException { assertEquals(2000, backoff.nextBackOffMillis()); assertEquals(4000, backoff.nextBackOffMillis()); } + + @Test(expected = IllegalArgumentException.class) + public void testNegativeMaxRetriesNotAllowed() { + HttpRetryConfig.builder() + .setMaxRetries(-1) + .build(); + } + + @Test(expected = IllegalArgumentException.class) + public void testMaxIntervalMillisTooSmall() { + HttpRetryConfig.builder() + .setMaxIntervalMillis(499) + .build(); + } + + @Test(expected = IllegalArgumentException.class) + public void testBackoffMultiplierTooSmall() { + HttpRetryConfig.builder() + .setBackoffMultiplier(0.99) + .build(); + } } diff --git a/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java b/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java index 8f7355e55..bf12bba26 100644 --- a/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java +++ b/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java @@ -23,7 +23,6 @@ import com.google.api.client.http.EmptyContent; import com.google.api.client.http.GenericUrl; import com.google.api.client.http.HttpBackOffIOExceptionHandler; -import com.google.api.client.http.HttpBackOffUnsuccessfulResponseHandler; import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpRequestFactory; import com.google.api.client.http.HttpResponse; @@ -79,7 +78,7 @@ public void testRetryOnHttpError() throws IOException { new HttpCredentialsAdapter(TEST_CREDENTIALS), HttpRetryConfig.builder().setRetryStatusCodes(ImmutableList.of(503)).build()); MockSleeper sleeper = new MockSleeper(); - ((HttpBackOffUnsuccessfulResponseHandler) retryHandler.getResponseHandler()) + ((RetryAfterAwareHttpResponseHandler) retryHandler.getResponseHandler()) .setSleeper(sleeper); request.setNumberOfRetries(4); request.setUnsuccessfulResponseHandler(retryHandler); @@ -104,7 +103,7 @@ public void testDoesNotRetryOnUnspecifiedHttpError() throws IOException { new HttpCredentialsAdapter(TEST_CREDENTIALS), HttpRetryConfig.builder().setRetryStatusCodes(ImmutableList.of(503)).build()); MockSleeper sleeper = new MockSleeper(); - ((HttpBackOffUnsuccessfulResponseHandler) retryHandler.getResponseHandler()) + ((RetryAfterAwareHttpResponseHandler) retryHandler.getResponseHandler()) .setSleeper(sleeper); request.setNumberOfRetries(4); request.setUnsuccessfulResponseHandler(retryHandler); @@ -141,7 +140,7 @@ public boolean handleResponse( credentials, HttpRetryConfig.builder().setRetryStatusCodes(ImmutableList.of(503)).build()); MockSleeper sleeper = new MockSleeper(); - ((HttpBackOffUnsuccessfulResponseHandler) retryHandler.getResponseHandler()) + ((RetryAfterAwareHttpResponseHandler) retryHandler.getResponseHandler()) .setSleeper(sleeper); request.setNumberOfRetries(4); request.setUnsuccessfulResponseHandler(retryHandler); diff --git a/src/test/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandlerTest.java b/src/test/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandlerTest.java new file mode 100644 index 000000000..240726841 --- /dev/null +++ b/src/test/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandlerTest.java @@ -0,0 +1,223 @@ +/* + * Copyright 2019 Google Inc. + * + * 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.internal; + +import static com.google.common.base.Preconditions.checkNotNull; +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import com.google.api.client.http.EmptyContent; +import com.google.api.client.http.GenericUrl; +import com.google.api.client.http.HttpRequest; +import com.google.api.client.http.HttpRequestFactory; +import com.google.api.client.http.HttpResponseException; +import com.google.api.client.http.HttpTransport; +import com.google.api.client.http.LowLevelHttpResponse; +import com.google.api.client.testing.http.FixedClock; +import com.google.api.client.testing.http.MockHttpTransport; +import com.google.api.client.testing.http.MockLowLevelHttpRequest; +import com.google.api.client.testing.http.MockLowLevelHttpResponse; +import com.google.api.client.testing.util.MockSleeper; +import com.google.api.client.util.Clock; +import com.google.common.collect.ImmutableList; +import com.google.common.primitives.Longs; +import java.io.IOException; +import java.text.SimpleDateFormat; +import java.util.ArrayList; +import java.util.Date; +import java.util.List; +import java.util.TimeZone; +import org.junit.Test; + +public class RetryAfterAwareHttpResponseHandlerTest { + + private static final GenericUrl TEST_URL = new GenericUrl("https://firebase.google.com"); + private static final HttpRetryConfig TEST_RETRY_CONFIG = HttpRetryConfig.builder() + .setRetryStatusCodes(ImmutableList.of(503)) + .build(); + + @Test + public void testRetryWithBackoffWhenRetryAfterIsAbsent() throws IOException { + RetryAfterAwareHttpResponseHandler handler = new RetryAfterAwareHttpResponseHandler( + TEST_RETRY_CONFIG); + MultipleCallSleeper sleeper = new MultipleCallSleeper(); + handler.setSleeper(sleeper); + CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + new MockLowLevelHttpResponse().setStatusCode(503).setZeroContent()); + HttpRequest request = createRequest(failingRequest); + request.setNumberOfRetries(4); + request.setUnsuccessfulResponseHandler(handler); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (HttpResponseException e) { + assertEquals(503, e.getStatusCode()); + } + + assertEquals(4, sleeper.getCount()); + assertArrayEquals(new long[]{500, 1000, 2000, 4000}, sleeper.getDelays()); + assertEquals(5, failingRequest.getCount()); + } + + @Test + public void testRetryWithRetryAfterGivenAsSeconds() throws IOException { + RetryAfterAwareHttpResponseHandler handler = new RetryAfterAwareHttpResponseHandler( + TEST_RETRY_CONFIG); + MultipleCallSleeper sleeper = new MultipleCallSleeper(); + handler.setSleeper(sleeper); + CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + new MockLowLevelHttpResponse() + .addHeader("retry-after", "2") + .setStatusCode(503) + .setZeroContent()); + HttpRequest request = createRequest(failingRequest); + request.setNumberOfRetries(4); + request.setUnsuccessfulResponseHandler(handler); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (HttpResponseException e) { + assertEquals(503, e.getStatusCode()); + } + + assertEquals(4, sleeper.getCount()); + assertArrayEquals(new long[]{2000, 2000, 2000, 2000}, sleeper.getDelays()); + assertEquals(5, failingRequest.getCount()); + } + + @Test + public void testRetryWithRetryAfterGivenAsDate() throws IOException { + SimpleDateFormat dateFormat = new SimpleDateFormat("EEE, dd MMM yyyy HH:mm:ss zzz"); + dateFormat.setTimeZone(TimeZone.getTimeZone("GMT")); + Date date = new Date(1000); + Clock clock = new FixedClock(date.getTime()); + String retryAfter = dateFormat.format(new Date(date.getTime() + 30000)); + + RetryAfterAwareHttpResponseHandler handler = new RetryAfterAwareHttpResponseHandler( + TEST_RETRY_CONFIG, clock); + MultipleCallSleeper sleeper = new MultipleCallSleeper(); + handler.setSleeper(sleeper); + CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + new MockLowLevelHttpResponse() + .addHeader("retry-after", retryAfter) + .setStatusCode(503) + .setZeroContent()); + HttpRequest request = createRequest(failingRequest); + request.setNumberOfRetries(4); + request.setUnsuccessfulResponseHandler(handler); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (HttpResponseException e) { + assertEquals(503, e.getStatusCode()); + } + + assertEquals(4, sleeper.getCount()); + assertArrayEquals(new long[]{30000, 30000, 30000, 30000}, sleeper.getDelays()); + assertEquals(5, failingRequest.getCount()); + } + + @Test + public void testDoesNotRetryWhenRetryAfterIsTooLong() throws IOException { + RetryAfterAwareHttpResponseHandler handler = new RetryAfterAwareHttpResponseHandler( + TEST_RETRY_CONFIG); + MultipleCallSleeper sleeper = new MultipleCallSleeper(); + handler.setSleeper(sleeper); + CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + new MockLowLevelHttpResponse() + .addHeader("retry-after", "121") + .setStatusCode(503) + .setZeroContent()); + HttpRequest request = createRequest(failingRequest); + request.setNumberOfRetries(4); + request.setUnsuccessfulResponseHandler(handler); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (HttpResponseException e) { + assertEquals(503, e.getStatusCode()); + } + + assertEquals(0, sleeper.getCount()); + assertEquals(1, failingRequest.getCount()); + } + + private HttpRequest createRequest(MockLowLevelHttpRequest request) throws IOException { + HttpTransport transport = new MockHttpTransport.Builder() + .setLowLevelHttpRequest(request) + .build(); + HttpRequestFactory requestFactory = transport.createRequestFactory(); + return requestFactory.buildPostRequest(TEST_URL, new EmptyContent()); + } + + private static class CountingHttpRequest extends MockLowLevelHttpRequest { + + private final LowLevelHttpResponse response; + private final IOException exception; + private int count; + + private CountingHttpRequest(LowLevelHttpResponse response, IOException exception) { + this.response = response; + this.exception = exception; + } + + static CountingHttpRequest fromResponse(LowLevelHttpResponse response) { + return new CountingHttpRequest(checkNotNull(response), null); + } + + static CountingHttpRequest fromException(IOException exception) { + return new CountingHttpRequest(null, checkNotNull(exception)); + } + + @Override + public void addHeader(String name, String value) { } + + @Override + public LowLevelHttpResponse execute() throws IOException { + count++; + if (response != null) { + return response; + } + throw exception; + } + + int getCount() { + return count; + } + } + + private static class MultipleCallSleeper extends MockSleeper { + + private final List delays = new ArrayList<>(); + + @Override + public void sleep(long millis) throws InterruptedException { + super.sleep(millis); + delays.add(millis); + } + + long[] getDelays() { + return Longs.toArray(delays); + } + } +} diff --git a/src/test/java/com/google/firebase/internal/TestOnlyImplRequestInitializerTrampolines.java b/src/test/java/com/google/firebase/internal/TestOnlyImplRequestInitializerTrampolines.java deleted file mode 100644 index 4a03d20d2..000000000 --- a/src/test/java/com/google/firebase/internal/TestOnlyImplRequestInitializerTrampolines.java +++ /dev/null @@ -1,27 +0,0 @@ -/* - * Copyright 2019 Google Inc. - * - * 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.internal; - -import com.google.api.client.testing.util.MockSleeper; - -public class TestOnlyImplRequestInitializerTrampolines { - - public static void disableRetryDelays() { - HttpRetryHandler.SLEEPER = new MockSleeper(); - } - -} From a50da389fb52f1cc47b7d20969b892fd30c4c3b4 Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Thu, 14 Feb 2019 22:06:21 -0800 Subject: [PATCH 03/19] Cleaned up the retry-after processing logic --- .../internal/FirebaseRequestInitializer.java | 10 ++- .../firebase/internal/HttpRetryHandler.java | 51 ++++++++----- .../RetryAfterAwareHttpResponseHandler.java | 10 +-- .../FirebaseRequestInitializerTest.java | 4 +- .../internal/HttpRetryHandlerTest.java | 72 ++++++++----------- ...etryAfterAwareHttpResponseHandlerTest.java | 3 +- 6 files changed, 78 insertions(+), 72 deletions(-) diff --git a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java index 04a78b532..89453b4b8 100644 --- a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java +++ b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java @@ -16,6 +16,7 @@ package com.google.firebase.internal; +import com.google.api.client.http.HttpBackOffIOExceptionHandler; import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpRequestInitializer; import com.google.auth.http.HttpCredentialsAdapter; @@ -66,9 +67,14 @@ public void initialize(HttpRequest httpRequest) throws IOException { httpRequest.setReadTimeout(readTimeout); if (retryConfig != null) { httpRequest.setNumberOfRetries(retryConfig.getMaxRetries()); - HttpRetryHandler retryHandler = new HttpRetryHandler(credentialsAdapter, retryConfig); + HttpRetryHandler retryHandler = HttpRetryHandler.builder() + .setCredentials(credentialsAdapter) + .setRetryConfig(retryConfig) + .setResponseHandler(new RetryAfterAwareHttpResponseHandler(retryConfig)) + .build(); httpRequest.setUnsuccessfulResponseHandler(retryHandler); - httpRequest.setIOExceptionHandler(retryHandler); + httpRequest.setIOExceptionHandler( + new HttpBackOffIOExceptionHandler(retryConfig.newBackoff())); } else { httpRequest.setNumberOfRetries(0); } diff --git a/src/main/java/com/google/firebase/internal/HttpRetryHandler.java b/src/main/java/com/google/firebase/internal/HttpRetryHandler.java index 0f42f0fd9..a6a19f839 100644 --- a/src/main/java/com/google/firebase/internal/HttpRetryHandler.java +++ b/src/main/java/com/google/firebase/internal/HttpRetryHandler.java @@ -18,31 +18,22 @@ import static com.google.common.base.Preconditions.checkNotNull; -import com.google.api.client.http.HttpBackOffIOExceptionHandler; -import com.google.api.client.http.HttpIOExceptionHandler; import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpResponse; import com.google.api.client.http.HttpUnsuccessfulResponseHandler; import com.google.auth.http.HttpCredentialsAdapter; import java.io.IOException; -final class HttpRetryHandler implements HttpUnsuccessfulResponseHandler, HttpIOExceptionHandler { +final class HttpRetryHandler implements HttpUnsuccessfulResponseHandler { private final HttpCredentialsAdapter credentials; private final HttpRetryConfig retryConfig; - private final HttpIOExceptionHandler ioExceptionHandler; private final HttpUnsuccessfulResponseHandler responseHandler; - HttpRetryHandler(HttpCredentialsAdapter credentials, HttpRetryConfig retryConfig) { - this.credentials = checkNotNull(credentials); - this.retryConfig = checkNotNull(retryConfig); - this.ioExceptionHandler = new HttpBackOffIOExceptionHandler(retryConfig.newBackoff()); - this.responseHandler = new RetryAfterAwareHttpResponseHandler(retryConfig); - } - - @Override - public boolean handleIOException(HttpRequest request, boolean supportsRetry) throws IOException { - return ioExceptionHandler.handleIOException(request, supportsRetry); + private HttpRetryHandler(Builder builder) { + this.credentials = checkNotNull(builder.credentials); + this.retryConfig = checkNotNull(builder.retryConfig); + this.responseHandler = checkNotNull(builder.responseHandler); } @Override @@ -56,15 +47,39 @@ public boolean handleResponse( retry = responseHandler.handleResponse(request, response, supportsRetry); } } + request.setUnsuccessfulResponseHandler(this); return retry; } - HttpIOExceptionHandler getIoExceptionHandler() { - return ioExceptionHandler; + static Builder builder() { + return new Builder(); } - HttpUnsuccessfulResponseHandler getResponseHandler() { - return responseHandler; + static class Builder { + private HttpCredentialsAdapter credentials; + private HttpRetryConfig retryConfig; + private HttpUnsuccessfulResponseHandler responseHandler; + + private Builder() { } + + Builder setCredentials(HttpCredentialsAdapter credentials) { + this.credentials = credentials; + return this; + } + + Builder setRetryConfig(HttpRetryConfig retryConfig) { + this.retryConfig = retryConfig; + return this; + } + + Builder setResponseHandler(HttpUnsuccessfulResponseHandler responseHandler) { + this.responseHandler = responseHandler; + return this; + } + + HttpRetryHandler build() { + return new HttpRetryHandler(this); + } } } diff --git a/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java b/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java index f688d7aff..ddd5a4137 100644 --- a/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java +++ b/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java @@ -32,7 +32,7 @@ final class RetryAfterAwareHttpResponseHandler implements HttpUnsuccessfulResponseHandler { private final HttpRetryConfig retryConfig; - private final HttpBackOffUnsuccessfulResponseHandler backoffHandler; + private final HttpBackOffUnsuccessfulResponseHandler backOffHandler; private final Clock clock; RetryAfterAwareHttpResponseHandler(HttpRetryConfig retryConfig) { @@ -41,12 +41,12 @@ final class RetryAfterAwareHttpResponseHandler implements HttpUnsuccessfulRespon RetryAfterAwareHttpResponseHandler(HttpRetryConfig retryConfig, Clock clock) { this.retryConfig = checkNotNull(retryConfig); - this.backoffHandler = new HttpBackOffUnsuccessfulResponseHandler(retryConfig.newBackoff()); + this.backOffHandler = new HttpBackOffUnsuccessfulResponseHandler(retryConfig.newBackoff()); this.clock = checkNotNull(clock); } void setSleeper(Sleeper sleeper) { - backoffHandler.setSleeper(sleeper); + backOffHandler.setSleeper(sleeper); } @Override @@ -66,7 +66,7 @@ public boolean handleResponse( if (delayMillis > 0) { try { - backoffHandler.getSleeper().sleep(delayMillis); + backOffHandler.getSleeper().sleep(delayMillis); } catch (InterruptedException e) { // ignore } @@ -74,7 +74,7 @@ public boolean handleResponse( return true; } - return backoffHandler.handleResponse(request, response, true); + return backOffHandler.handleResponse(request, response, true); } private long parseRetryAfter(String retryAfter) { diff --git a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java index d6024a54f..90885adf6 100644 --- a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java @@ -17,11 +17,11 @@ package com.google.firebase.internal; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import com.google.api.client.http.GenericUrl; +import com.google.api.client.http.HttpBackOffIOExceptionHandler; import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpRequestFactory; import com.google.api.client.http.HttpTransport; @@ -122,7 +122,7 @@ public void testExplicitRetryConfig() throws Exception { assertEquals(0, request.getReadTimeout()); assertEquals("Bearer token", request.getHeaders().getAuthorization()); assertEquals(5, request.getNumberOfRetries()); - assertTrue(request.getIOExceptionHandler() instanceof HttpRetryHandler); + assertTrue(request.getIOExceptionHandler() instanceof HttpBackOffIOExceptionHandler); assertTrue(request.getUnsuccessfulResponseHandler() instanceof HttpRetryHandler); } } diff --git a/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java b/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java index bf12bba26..127c20978 100644 --- a/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java +++ b/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java @@ -22,7 +22,7 @@ import com.google.api.client.http.EmptyContent; import com.google.api.client.http.GenericUrl; -import com.google.api.client.http.HttpBackOffIOExceptionHandler; +import com.google.api.client.http.HttpBackOffUnsuccessfulResponseHandler; import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpRequestFactory; import com.google.api.client.http.HttpResponse; @@ -44,42 +44,24 @@ public class HttpRetryHandlerTest { private static final GenericUrl TEST_URL = new GenericUrl("https://firebase.google.com"); private static final GoogleCredentials TEST_CREDENTIALS = new MockGoogleCredentials(); - - @Test - public void testRetryOnIOException() throws IOException { - CountingHttpRequest failingRequest = CountingHttpRequest.fromException( - new IOException("test error")); - HttpRequest request = createRequest(failingRequest); - HttpRetryHandler retryHandler = new HttpRetryHandler( - new HttpCredentialsAdapter(TEST_CREDENTIALS), - HttpRetryConfig.builder().build()); - MockSleeper sleeper = new MockSleeper(); - ((HttpBackOffIOExceptionHandler) retryHandler.getIoExceptionHandler()).setSleeper(sleeper); - request.setNumberOfRetries(4); - request.setIOExceptionHandler(retryHandler); - - try { - request.execute(); - fail("No exception thrown for transport error"); - } catch (IOException e) { - assertEquals("test error", e.getMessage()); - } - - assertEquals(4, sleeper.getCount()); - assertEquals(5, failingRequest.getCount()); - } + private static final HttpRetryConfig RETRY_CONFIG = HttpRetryConfig.builder() + .setRetryStatusCodes(ImmutableList.of(503)) + .build(); @Test public void testRetryOnHttpError() throws IOException { CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( new MockLowLevelHttpResponse().setStatusCode(503).setZeroContent()); - HttpRequest request = createRequest(failingRequest); - HttpRetryHandler retryHandler = new HttpRetryHandler( - new HttpCredentialsAdapter(TEST_CREDENTIALS), - HttpRetryConfig.builder().setRetryStatusCodes(ImmutableList.of(503)).build()); MockSleeper sleeper = new MockSleeper(); - ((RetryAfterAwareHttpResponseHandler) retryHandler.getResponseHandler()) + HttpBackOffUnsuccessfulResponseHandler responseHandler = + new HttpBackOffUnsuccessfulResponseHandler(RETRY_CONFIG.newBackoff()) .setSleeper(sleeper); + HttpRetryHandler retryHandler = HttpRetryHandler.builder() + .setCredentials(new HttpCredentialsAdapter(TEST_CREDENTIALS)) + .setRetryConfig(RETRY_CONFIG) + .setResponseHandler(responseHandler) + .build(); + HttpRequest request = createRequest(failingRequest); request.setNumberOfRetries(4); request.setUnsuccessfulResponseHandler(retryHandler); @@ -98,13 +80,16 @@ public void testRetryOnHttpError() throws IOException { public void testDoesNotRetryOnUnspecifiedHttpError() throws IOException { CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( new MockLowLevelHttpResponse().setStatusCode(404).setZeroContent()); - HttpRequest request = createRequest(failingRequest); - HttpRetryHandler retryHandler = new HttpRetryHandler( - new HttpCredentialsAdapter(TEST_CREDENTIALS), - HttpRetryConfig.builder().setRetryStatusCodes(ImmutableList.of(503)).build()); MockSleeper sleeper = new MockSleeper(); - ((RetryAfterAwareHttpResponseHandler) retryHandler.getResponseHandler()) - .setSleeper(sleeper); + HttpBackOffUnsuccessfulResponseHandler responseHandler = + new HttpBackOffUnsuccessfulResponseHandler(RETRY_CONFIG.newBackoff()) + .setSleeper(sleeper); + HttpRetryHandler retryHandler = HttpRetryHandler.builder() + .setCredentials(new HttpCredentialsAdapter(TEST_CREDENTIALS)) + .setRetryConfig(RETRY_CONFIG) + .setResponseHandler(responseHandler) + .build(); + HttpRequest request = createRequest(failingRequest); request.setNumberOfRetries(4); request.setUnsuccessfulResponseHandler(retryHandler); @@ -136,15 +121,16 @@ public boolean handleResponse( return false; } }; - HttpRetryHandler retryHandler = new HttpRetryHandler( - credentials, - HttpRetryConfig.builder().setRetryStatusCodes(ImmutableList.of(503)).build()); MockSleeper sleeper = new MockSleeper(); - ((RetryAfterAwareHttpResponseHandler) retryHandler.getResponseHandler()) - .setSleeper(sleeper); - request.setNumberOfRetries(4); + HttpBackOffUnsuccessfulResponseHandler responseHandler = + new HttpBackOffUnsuccessfulResponseHandler(RETRY_CONFIG.newBackoff()) + .setSleeper(sleeper); + HttpRetryHandler retryHandler = HttpRetryHandler.builder() + .setCredentials(credentials) + .setRetryConfig(RETRY_CONFIG) + .setResponseHandler(responseHandler) + .build(); request.setUnsuccessfulResponseHandler(retryHandler); - try { request.execute(); fail("No exception thrown for HTTP error"); diff --git a/src/test/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandlerTest.java b/src/test/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandlerTest.java index 240726841..02be3af3a 100644 --- a/src/test/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandlerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandlerTest.java @@ -19,7 +19,6 @@ import static com.google.common.base.Preconditions.checkNotNull; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import com.google.api.client.http.EmptyContent; @@ -53,7 +52,7 @@ public class RetryAfterAwareHttpResponseHandlerTest { .build(); @Test - public void testRetryWithBackoffWhenRetryAfterIsAbsent() throws IOException { + public void testRetryWithBackOffWhenRetryAfterIsAbsent() throws IOException { RetryAfterAwareHttpResponseHandler handler = new RetryAfterAwareHttpResponseHandler( TEST_RETRY_CONFIG); MultipleCallSleeper sleeper = new MultipleCallSleeper(); From 1244ff8eb0b4a3f8d03b21e86a380a05ff42321d Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Thu, 14 Feb 2019 23:14:02 -0800 Subject: [PATCH 04/19] Moved the status code checking logic --- .../internal/FirebaseRequestInitializer.java | 20 +---- .../firebase/internal/HttpRetryHandler.java | 49 +++--------- .../RetryAfterAwareHttpResponseHandler.java | 5 ++ .../internal/HttpRetryHandlerTest.java | 80 +++++++------------ ...etryAfterAwareHttpResponseHandlerTest.java | 30 ++++++- 5 files changed, 70 insertions(+), 114 deletions(-) diff --git a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java index 89453b4b8..101babfb8 100644 --- a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java +++ b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java @@ -21,11 +21,9 @@ import com.google.api.client.http.HttpRequestInitializer; import com.google.auth.http.HttpCredentialsAdapter; import com.google.auth.oauth2.GoogleCredentials; -import com.google.common.collect.ImmutableList; import com.google.firebase.FirebaseApp; import com.google.firebase.ImplFirebaseTrampolines; import java.io.IOException; -import java.util.concurrent.TimeUnit; /** * {@code HttpRequestInitializer} for configuring outgoing REST calls. Handles OAuth2 authorization @@ -33,16 +31,6 @@ */ public class FirebaseRequestInitializer implements HttpRequestInitializer { - private static final int STATUS_INTERNAL_SERVER_ERROR = 500; - private static final int STATUS_SERVICE_UNAVAILABLE = 503; - private static final HttpRetryConfig DEFAULT_RETRY_CONFIG = HttpRetryConfig.builder() - .setRetryStatusCodes(ImmutableList.of( - STATUS_INTERNAL_SERVER_ERROR, STATUS_SERVICE_UNAVAILABLE)) - .setMaxRetries(4) - .setBackoffMultiplier(2.0) - .setMaxIntervalMillis((int) TimeUnit.MINUTES.toMillis(2)) - .build(); - private final HttpCredentialsAdapter credentialsAdapter; private final HttpRetryConfig retryConfig; private final int connectTimeout; @@ -67,12 +55,8 @@ public void initialize(HttpRequest httpRequest) throws IOException { httpRequest.setReadTimeout(readTimeout); if (retryConfig != null) { httpRequest.setNumberOfRetries(retryConfig.getMaxRetries()); - HttpRetryHandler retryHandler = HttpRetryHandler.builder() - .setCredentials(credentialsAdapter) - .setRetryConfig(retryConfig) - .setResponseHandler(new RetryAfterAwareHttpResponseHandler(retryConfig)) - .build(); - httpRequest.setUnsuccessfulResponseHandler(retryHandler); + httpRequest.setUnsuccessfulResponseHandler( + new HttpRetryHandler(credentialsAdapter, retryConfig)); httpRequest.setIOExceptionHandler( new HttpBackOffIOExceptionHandler(retryConfig.newBackoff())); } else { diff --git a/src/main/java/com/google/firebase/internal/HttpRetryHandler.java b/src/main/java/com/google/firebase/internal/HttpRetryHandler.java index a6a19f839..f96e7f4e8 100644 --- a/src/main/java/com/google/firebase/internal/HttpRetryHandler.java +++ b/src/main/java/com/google/firebase/internal/HttpRetryHandler.java @@ -27,13 +27,16 @@ final class HttpRetryHandler implements HttpUnsuccessfulResponseHandler { private final HttpCredentialsAdapter credentials; - private final HttpRetryConfig retryConfig; private final HttpUnsuccessfulResponseHandler responseHandler; - private HttpRetryHandler(Builder builder) { - this.credentials = checkNotNull(builder.credentials); - this.retryConfig = checkNotNull(builder.retryConfig); - this.responseHandler = checkNotNull(builder.responseHandler); + HttpRetryHandler( + HttpCredentialsAdapter credentials, HttpUnsuccessfulResponseHandler responseHandler) { + this.credentials = checkNotNull(credentials); + this.responseHandler = checkNotNull(responseHandler); + } + + HttpRetryHandler(HttpCredentialsAdapter credentials, HttpRetryConfig retryConfig) { + this(credentials, new RetryAfterAwareHttpResponseHandler(retryConfig)); } @Override @@ -42,44 +45,10 @@ public boolean handleResponse( boolean retry = credentials.handleResponse(request, response, supportsRetry); if (!retry) { - int status = response.getStatusCode(); - if (retryConfig.getRetryStatusCodes().contains(status)) { - retry = responseHandler.handleResponse(request, response, supportsRetry); - } + retry = responseHandler.handleResponse(request, response, supportsRetry); } request.setUnsuccessfulResponseHandler(this); return retry; } - - static Builder builder() { - return new Builder(); - } - - static class Builder { - private HttpCredentialsAdapter credentials; - private HttpRetryConfig retryConfig; - private HttpUnsuccessfulResponseHandler responseHandler; - - private Builder() { } - - Builder setCredentials(HttpCredentialsAdapter credentials) { - this.credentials = credentials; - return this; - } - - Builder setRetryConfig(HttpRetryConfig retryConfig) { - this.retryConfig = retryConfig; - return this; - } - - Builder setResponseHandler(HttpUnsuccessfulResponseHandler responseHandler) { - this.responseHandler = responseHandler; - return this; - } - - HttpRetryHandler build() { - return new HttpRetryHandler(this); - } - } } diff --git a/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java b/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java index ddd5a4137..f2bb6182a 100644 --- a/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java +++ b/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java @@ -57,6 +57,11 @@ public boolean handleResponse( return false; } + int statusCode = response.getStatusCode(); + if (!retryConfig.getRetryStatusCodes().contains(statusCode)) { + return false; + } + String retryAfter = response.getHeaders().getRetryAfter(); if (!Strings.isNullOrEmpty(retryAfter)) { long delayMillis = parseRetryAfter(retryAfter.trim()); diff --git a/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java b/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java index 127c20978..a57d0de1b 100644 --- a/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java +++ b/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertSame; import static org.junit.Assert.fail; import com.google.api.client.http.EmptyContent; @@ -49,63 +50,43 @@ public class HttpRetryHandlerTest { .build(); @Test - public void testRetryOnHttpError() throws IOException { + public void testRetryCredentialsCheck() throws IOException { CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( - new MockLowLevelHttpResponse().setStatusCode(503).setZeroContent()); - MockSleeper sleeper = new MockSleeper(); - HttpBackOffUnsuccessfulResponseHandler responseHandler = - new HttpBackOffUnsuccessfulResponseHandler(RETRY_CONFIG.newBackoff()) - .setSleeper(sleeper); - HttpRetryHandler retryHandler = HttpRetryHandler.builder() - .setCredentials(new HttpCredentialsAdapter(TEST_CREDENTIALS)) - .setRetryConfig(RETRY_CONFIG) - .setResponseHandler(responseHandler) - .build(); + new MockLowLevelHttpResponse().setStatusCode(401).setZeroContent()); HttpRequest request = createRequest(failingRequest); - request.setNumberOfRetries(4); - request.setUnsuccessfulResponseHandler(retryHandler); - - try { - request.execute(); - fail("No exception thrown for HTTP error"); - } catch (HttpResponseException e) { - assertEquals(503, e.getStatusCode()); - } - - assertEquals(4, sleeper.getCount()); - assertEquals(5, failingRequest.getCount()); - } - - @Test - public void testDoesNotRetryOnUnspecifiedHttpError() throws IOException { - CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( - new MockLowLevelHttpResponse().setStatusCode(404).setZeroContent()); + HttpCredentialsAdapter credentials = new HttpCredentialsAdapter(TEST_CREDENTIALS){ + @Override + public boolean handleResponse( + HttpRequest request, HttpResponse response, boolean supportsRetry) { + String authorization = request.getHeaders().getAuthorization(); + if (!"Bearer retry".equals(authorization)) { + request.getHeaders().setAuthorization("Bearer retry"); + return true; + } + return false; + } + }; MockSleeper sleeper = new MockSleeper(); HttpBackOffUnsuccessfulResponseHandler responseHandler = new HttpBackOffUnsuccessfulResponseHandler(RETRY_CONFIG.newBackoff()) .setSleeper(sleeper); - HttpRetryHandler retryHandler = HttpRetryHandler.builder() - .setCredentials(new HttpCredentialsAdapter(TEST_CREDENTIALS)) - .setRetryConfig(RETRY_CONFIG) - .setResponseHandler(responseHandler) - .build(); - HttpRequest request = createRequest(failingRequest); - request.setNumberOfRetries(4); + HttpRetryHandler retryHandler = new HttpRetryHandler(credentials, responseHandler); request.setUnsuccessfulResponseHandler(retryHandler); - try { request.execute(); fail("No exception thrown for HTTP error"); } catch (HttpResponseException e) { - assertEquals(404, e.getStatusCode()); + assertEquals(401, e.getStatusCode()); } + assertEquals("Bearer retry", request.getHeaders().getAuthorization()); assertEquals(0, sleeper.getCount()); - assertEquals(1, failingRequest.getCount()); + assertEquals(2, failingRequest.getCount()); + assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); } @Test - public void testRetryCredentialsCheck() throws IOException { + public void testDelegateCalledAfterCredentials() throws IOException { CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( new MockLowLevelHttpResponse().setStatusCode(401).setZeroContent()); HttpRequest request = createRequest(failingRequest); @@ -124,13 +105,11 @@ public boolean handleResponse( MockSleeper sleeper = new MockSleeper(); HttpBackOffUnsuccessfulResponseHandler responseHandler = new HttpBackOffUnsuccessfulResponseHandler(RETRY_CONFIG.newBackoff()) - .setSleeper(sleeper); - HttpRetryHandler retryHandler = HttpRetryHandler.builder() - .setCredentials(credentials) - .setRetryConfig(RETRY_CONFIG) - .setResponseHandler(responseHandler) - .build(); + .setSleeper(sleeper) + .setBackOffRequired(HttpBackOffUnsuccessfulResponseHandler.BackOffRequired.ALWAYS); + HttpRetryHandler retryHandler = new HttpRetryHandler(credentials, responseHandler); request.setUnsuccessfulResponseHandler(retryHandler); + request.setNumberOfRetries(4); try { request.execute(); fail("No exception thrown for HTTP error"); @@ -139,8 +118,9 @@ public boolean handleResponse( } assertEquals("Bearer retry", request.getHeaders().getAuthorization()); - assertEquals(0, sleeper.getCount()); - assertEquals(2, failingRequest.getCount()); + assertEquals(3, sleeper.getCount()); + assertEquals(5, failingRequest.getCount()); + assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); } private HttpRequest createRequest(MockLowLevelHttpRequest request) throws IOException { @@ -166,10 +146,6 @@ static CountingHttpRequest fromResponse(LowLevelHttpResponse response) { return new CountingHttpRequest(checkNotNull(response), null); } - static CountingHttpRequest fromException(IOException exception) { - return new CountingHttpRequest(null, checkNotNull(exception)); - } - @Override public void addHeader(String name, String value) { } diff --git a/src/test/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandlerTest.java b/src/test/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandlerTest.java index 02be3af3a..171224671 100644 --- a/src/test/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandlerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandlerTest.java @@ -51,6 +51,32 @@ public class RetryAfterAwareHttpResponseHandlerTest { .setRetryStatusCodes(ImmutableList.of(503)) .build(); + @Test + public void testDoesNotRetryOnUnspecifiedHttpStatus() throws IOException { + RetryAfterAwareHttpResponseHandler handler = new RetryAfterAwareHttpResponseHandler( + TEST_RETRY_CONFIG); + MultipleCallSleeper sleeper = new MultipleCallSleeper(); + handler.setSleeper(sleeper); + CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + new MockLowLevelHttpResponse() + .addHeader("retry-after", "121") + .setStatusCode(404) + .setZeroContent()); + HttpRequest request = createRequest(failingRequest); + request.setNumberOfRetries(4); + request.setUnsuccessfulResponseHandler(handler); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (HttpResponseException e) { + assertEquals(404, e.getStatusCode()); + } + + assertEquals(0, sleeper.getCount()); + assertEquals(1, failingRequest.getCount()); + } + @Test public void testRetryWithBackOffWhenRetryAfterIsAbsent() throws IOException { RetryAfterAwareHttpResponseHandler handler = new RetryAfterAwareHttpResponseHandler( @@ -184,10 +210,6 @@ static CountingHttpRequest fromResponse(LowLevelHttpResponse response) { return new CountingHttpRequest(checkNotNull(response), null); } - static CountingHttpRequest fromException(IOException exception) { - return new CountingHttpRequest(null, checkNotNull(exception)); - } - @Override public void addHeader(String name, String value) { } From b381379e4e23fab8e4b08444858f27f3d831dd7e Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Fri, 15 Feb 2019 01:04:51 -0800 Subject: [PATCH 05/19] Updated tests --- .../internal/FirebaseRequestInitializer.java | 2 +- .../firebase/internal/HttpRetryConfig.java | 24 ++++---- .../RetryAfterAwareHttpResponseHandler.java | 34 ++++++----- .../internal/HttpRetryConfigTest.java | 57 ++++++++++--------- .../internal/HttpRetryHandlerTest.java | 4 +- 5 files changed, 65 insertions(+), 56 deletions(-) diff --git a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java index 101babfb8..f89114ca8 100644 --- a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java +++ b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java @@ -58,7 +58,7 @@ public void initialize(HttpRequest httpRequest) throws IOException { httpRequest.setUnsuccessfulResponseHandler( new HttpRetryHandler(credentialsAdapter, retryConfig)); httpRequest.setIOExceptionHandler( - new HttpBackOffIOExceptionHandler(retryConfig.newBackoff())); + new HttpBackOffIOExceptionHandler(retryConfig.newBackOff())); } else { httpRequest.setNumberOfRetries(0); } diff --git a/src/main/java/com/google/firebase/internal/HttpRetryConfig.java b/src/main/java/com/google/firebase/internal/HttpRetryConfig.java index 7f8d3c1d7..14aa07114 100644 --- a/src/main/java/com/google/firebase/internal/HttpRetryConfig.java +++ b/src/main/java/com/google/firebase/internal/HttpRetryConfig.java @@ -30,7 +30,7 @@ public final class HttpRetryConfig { private final List retryStatusCodes; private final int maxRetries; - private final ExponentialBackOff.Builder backoffBuilder; + private final ExponentialBackOff.Builder backOffBuilder; private HttpRetryConfig(Builder builder) { if (builder.retryStatusCodes != null) { @@ -41,14 +41,14 @@ private HttpRetryConfig(Builder builder) { checkArgument(builder.maxRetries >= 0, "maxRetries must not be negative"); this.maxRetries = builder.maxRetries; - this.backoffBuilder = new ExponentialBackOff.Builder() + this.backOffBuilder = new ExponentialBackOff.Builder() .setInitialIntervalMillis(INITIAL_INTERVAL_MILLIS) .setMaxIntervalMillis(builder.maxIntervalMillis) - .setMultiplier(builder.backoffMultiplier) + .setMultiplier(builder.backOffMultiplier) .setRandomizationFactor(0); // Force validation of arguments by building the BackOff object - this.backoffBuilder.build(); + this.backOffBuilder.build(); } List getRetryStatusCodes() { @@ -60,15 +60,15 @@ int getMaxRetries() { } int getMaxIntervalMillis() { - return backoffBuilder.getMaxIntervalMillis(); + return backOffBuilder.getMaxIntervalMillis(); } - double getBackoffMultiplier() { - return backoffBuilder.getMultiplier(); + double getBackOffMultiplier() { + return backOffBuilder.getMultiplier(); } - BackOff newBackoff() { - return backoffBuilder.build(); + BackOff newBackOff() { + return backOffBuilder.build(); } public static Builder builder() { @@ -80,7 +80,7 @@ public static final class Builder { private List retryStatusCodes; private int maxRetries; private int maxIntervalMillis = (int) TimeUnit.MINUTES.toMillis(2); - private double backoffMultiplier = 2.0; + private double backOffMultiplier = 2.0; private Builder() { } @@ -99,8 +99,8 @@ public Builder setMaxIntervalMillis(int maxIntervalMillis) { return this; } - public Builder setBackoffMultiplier(double backoffMultiplier) { - this.backoffMultiplier = backoffMultiplier; + public Builder setBackOffMultiplier(double backOffMultiplier) { + this.backOffMultiplier = backOffMultiplier; return this; } diff --git a/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java b/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java index f2bb6182a..23573ff0b 100644 --- a/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java +++ b/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java @@ -41,7 +41,7 @@ final class RetryAfterAwareHttpResponseHandler implements HttpUnsuccessfulRespon RetryAfterAwareHttpResponseHandler(HttpRetryConfig retryConfig, Clock clock) { this.retryConfig = checkNotNull(retryConfig); - this.backOffHandler = new HttpBackOffUnsuccessfulResponseHandler(retryConfig.newBackoff()); + this.backOffHandler = new HttpBackOffUnsuccessfulResponseHandler(retryConfig.newBackOff()); this.clock = checkNotNull(clock); } @@ -64,27 +64,31 @@ public boolean handleResponse( String retryAfter = response.getHeaders().getRetryAfter(); if (!Strings.isNullOrEmpty(retryAfter)) { - long delayMillis = parseRetryAfter(retryAfter.trim()); - if (delayMillis > retryConfig.getMaxIntervalMillis()) { - return false; - } - - if (delayMillis > 0) { - try { - backOffHandler.getSleeper().sleep(delayMillis); - } catch (InterruptedException e) { - // ignore - } - } - return true; + return handleRetryAfter(retryAfter); } return backOffHandler.handleResponse(request, response, true); } + private boolean handleRetryAfter(String retryAfter) { + long delayMillis = parseRetryAfter(retryAfter.trim()); + if (delayMillis > retryConfig.getMaxIntervalMillis()) { + return false; + } + + if (delayMillis > 0) { + try { + backOffHandler.getSleeper().sleep(delayMillis); + } catch (InterruptedException e) { + // ignore + } + } + return true; + } + private long parseRetryAfter(String retryAfter) { try { - return Long.parseLong(retryAfter.trim()) * 1000; + return Long.parseLong(retryAfter) * 1000; } catch (NumberFormatException e) { Date date = DateUtils.parseDate(retryAfter); if (date != null) { diff --git a/src/test/java/com/google/firebase/internal/HttpRetryConfigTest.java b/src/test/java/com/google/firebase/internal/HttpRetryConfigTest.java index de4ead19e..9dbbf9f83 100644 --- a/src/test/java/com/google/firebase/internal/HttpRetryConfigTest.java +++ b/src/test/java/com/google/firebase/internal/HttpRetryConfigTest.java @@ -20,6 +20,7 @@ import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertTrue; +import com.google.api.client.util.BackOff; import com.google.api.client.util.ExponentialBackOff; import com.google.common.collect.ImmutableList; import java.io.IOException; @@ -34,14 +35,14 @@ public void testEmptyBuilder() { assertTrue(config.getRetryStatusCodes().isEmpty()); assertEquals(0, config.getMaxRetries()); assertEquals(2 * 60 * 1000, config.getMaxIntervalMillis()); - assertEquals(2.0, config.getBackoffMultiplier(), 0.01); - - ExponentialBackOff backoff = (ExponentialBackOff) config.newBackoff(); - assertEquals(2 * 60 * 1000, backoff.getMaxIntervalMillis()); - assertEquals(2.0, backoff.getMultiplier(), 0.01); - assertEquals(500, backoff.getInitialIntervalMillis()); - assertEquals(0.0, backoff.getRandomizationFactor(), 0.01); - assertNotSame(backoff, config.newBackoff()); + assertEquals(2.0, config.getBackOffMultiplier(), 0.01); + + ExponentialBackOff backOff = (ExponentialBackOff) config.newBackOff(); + assertEquals(2 * 60 * 1000, backOff.getMaxIntervalMillis()); + assertEquals(2.0, backOff.getMultiplier(), 0.01); + assertEquals(500, backOff.getInitialIntervalMillis()); + assertEquals(0.0, backOff.getRandomizationFactor(), 0.01); + assertNotSame(backOff, config.newBackOff()); } @Test @@ -51,7 +52,7 @@ public void testBuilder() { .setMaxRetries(4) .setRetryStatusCodes(statusCodes) .setMaxIntervalMillis(5 * 60 * 1000) - .setBackoffMultiplier(1.5) + .setBackOffMultiplier(1.5) .build(); assertEquals(2, config.getRetryStatusCodes().size()); @@ -59,26 +60,30 @@ public void testBuilder() { assertEquals(statusCodes.get(1), config.getRetryStatusCodes().get(1)); assertEquals(4, config.getMaxRetries()); assertEquals(5 * 60 * 1000, config.getMaxIntervalMillis()); - assertEquals(1.5, config.getBackoffMultiplier(), 0.01); - - ExponentialBackOff backoff = (ExponentialBackOff) config.newBackoff(); - assertEquals(500, backoff.getInitialIntervalMillis()); - assertEquals(5 * 60 * 1000, backoff.getMaxIntervalMillis()); - assertEquals(1.5, backoff.getMultiplier(), 0.01); - assertEquals(0.0, backoff.getRandomizationFactor(), 0.01); - assertNotSame(backoff, config.newBackoff()); + assertEquals(1.5, config.getBackOffMultiplier(), 0.01); + + ExponentialBackOff backOff = (ExponentialBackOff) config.newBackOff(); + assertEquals(500, backOff.getInitialIntervalMillis()); + assertEquals(5 * 60 * 1000, backOff.getMaxIntervalMillis()); + assertEquals(1.5, backOff.getMultiplier(), 0.01); + assertEquals(0.0, backOff.getRandomizationFactor(), 0.01); + assertNotSame(backOff, config.newBackOff()); } @Test - public void testExponentialBackoff() throws IOException { - HttpRetryConfig config = HttpRetryConfig.builder().build(); + public void testExponentialBackOff() throws IOException { + HttpRetryConfig config = HttpRetryConfig.builder() + .setMaxIntervalMillis(12000) + .build(); - ExponentialBackOff backoff = (ExponentialBackOff) config.newBackoff(); + BackOff backOff = config.newBackOff(); - assertEquals(500, backoff.nextBackOffMillis()); - assertEquals(1000, backoff.nextBackOffMillis()); - assertEquals(2000, backoff.nextBackOffMillis()); - assertEquals(4000, backoff.nextBackOffMillis()); + assertEquals(500, backOff.nextBackOffMillis()); + assertEquals(1000, backOff.nextBackOffMillis()); + assertEquals(2000, backOff.nextBackOffMillis()); + assertEquals(4000, backOff.nextBackOffMillis()); + assertEquals(8000, backOff.nextBackOffMillis()); + assertEquals(12000, backOff.nextBackOffMillis()); } @Test(expected = IllegalArgumentException.class) @@ -96,9 +101,9 @@ public void testMaxIntervalMillisTooSmall() { } @Test(expected = IllegalArgumentException.class) - public void testBackoffMultiplierTooSmall() { + public void testBackOffMultiplierTooSmall() { HttpRetryConfig.builder() - .setBackoffMultiplier(0.99) + .setBackOffMultiplier(0.99) .build(); } } diff --git a/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java b/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java index a57d0de1b..0ec1e39d1 100644 --- a/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java +++ b/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java @@ -68,7 +68,7 @@ public boolean handleResponse( }; MockSleeper sleeper = new MockSleeper(); HttpBackOffUnsuccessfulResponseHandler responseHandler = - new HttpBackOffUnsuccessfulResponseHandler(RETRY_CONFIG.newBackoff()) + new HttpBackOffUnsuccessfulResponseHandler(RETRY_CONFIG.newBackOff()) .setSleeper(sleeper); HttpRetryHandler retryHandler = new HttpRetryHandler(credentials, responseHandler); request.setUnsuccessfulResponseHandler(retryHandler); @@ -104,7 +104,7 @@ public boolean handleResponse( }; MockSleeper sleeper = new MockSleeper(); HttpBackOffUnsuccessfulResponseHandler responseHandler = - new HttpBackOffUnsuccessfulResponseHandler(RETRY_CONFIG.newBackoff()) + new HttpBackOffUnsuccessfulResponseHandler(RETRY_CONFIG.newBackOff()) .setSleeper(sleeper) .setBackOffRequired(HttpBackOffUnsuccessfulResponseHandler.BackOffRequired.ALWAYS); HttpRetryHandler retryHandler = new HttpRetryHandler(credentials, responseHandler); From 9ba7900725d32c4311806a1053b8a74f4a11caf0 Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Fri, 15 Feb 2019 14:21:49 -0800 Subject: [PATCH 06/19] Updated class names and tests --- ... CredentialsResponseHandlerDecorator.java} | 16 ++-- .../internal/FirebaseRequestInitializer.java | 46 ++++++----- ...{HttpRetryConfig.java => RetryConfig.java} | 8 +- .../firebase/internal/RetryInitializer.java | 55 +++++++++++++ ... => RetryUnsuccessfulResponseHandler.java} | 20 ++--- ...dentialsResponseHandlerDecoratorTest.java} | 72 ++++++++--------- .../FirebaseRequestInitializerTest.java | 5 +- ...ryConfigTest.java => RetryConfigTest.java} | 16 ++-- .../internal/RetryInitializerTest.java | 79 +++++++++++++++++++ ...RetryUnsuccessfulResponseHandlerTest.java} | 20 ++--- 10 files changed, 237 insertions(+), 100 deletions(-) rename src/main/java/com/google/firebase/internal/{HttpRetryHandler.java => CredentialsResponseHandlerDecorator.java} (74%) rename src/main/java/com/google/firebase/internal/{HttpRetryConfig.java => RetryConfig.java} (95%) create mode 100644 src/main/java/com/google/firebase/internal/RetryInitializer.java rename src/main/java/com/google/firebase/internal/{RetryAfterAwareHttpResponseHandler.java => RetryUnsuccessfulResponseHandler.java} (80%) rename src/test/java/com/google/firebase/internal/{HttpRetryHandlerTest.java => CredentialsResponseHandlerDecoratorTest.java} (75%) rename src/test/java/com/google/firebase/internal/{HttpRetryConfigTest.java => RetryConfigTest.java} (91%) create mode 100644 src/test/java/com/google/firebase/internal/RetryInitializerTest.java rename src/test/java/com/google/firebase/internal/{RetryAfterAwareHttpResponseHandlerTest.java => RetryUnsuccessfulResponseHandlerTest.java} (90%) diff --git a/src/main/java/com/google/firebase/internal/HttpRetryHandler.java b/src/main/java/com/google/firebase/internal/CredentialsResponseHandlerDecorator.java similarity index 74% rename from src/main/java/com/google/firebase/internal/HttpRetryHandler.java rename to src/main/java/com/google/firebase/internal/CredentialsResponseHandlerDecorator.java index f96e7f4e8..5ef4a673f 100644 --- a/src/main/java/com/google/firebase/internal/HttpRetryHandler.java +++ b/src/main/java/com/google/firebase/internal/CredentialsResponseHandlerDecorator.java @@ -24,19 +24,15 @@ import com.google.auth.http.HttpCredentialsAdapter; import java.io.IOException; -final class HttpRetryHandler implements HttpUnsuccessfulResponseHandler { +final class CredentialsResponseHandlerDecorator implements HttpUnsuccessfulResponseHandler { private final HttpCredentialsAdapter credentials; - private final HttpUnsuccessfulResponseHandler responseHandler; + private final HttpUnsuccessfulResponseHandler delegate; - HttpRetryHandler( - HttpCredentialsAdapter credentials, HttpUnsuccessfulResponseHandler responseHandler) { + CredentialsResponseHandlerDecorator( + HttpCredentialsAdapter credentials, HttpUnsuccessfulResponseHandler delegate) { this.credentials = checkNotNull(credentials); - this.responseHandler = checkNotNull(responseHandler); - } - - HttpRetryHandler(HttpCredentialsAdapter credentials, HttpRetryConfig retryConfig) { - this(credentials, new RetryAfterAwareHttpResponseHandler(retryConfig)); + this.delegate = checkNotNull(delegate); } @Override @@ -45,7 +41,7 @@ public boolean handleResponse( boolean retry = credentials.handleResponse(request, response, supportsRetry); if (!retry) { - retry = responseHandler.handleResponse(request, response, supportsRetry); + retry = delegate.handleResponse(request, response, supportsRetry); } request.setUnsuccessfulResponseHandler(this); diff --git a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java index f89114ca8..9d7cda8ce 100644 --- a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java +++ b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java @@ -16,12 +16,12 @@ package com.google.firebase.internal; -import com.google.api.client.http.HttpBackOffIOExceptionHandler; import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpRequestInitializer; import com.google.auth.http.HttpCredentialsAdapter; import com.google.auth.oauth2.GoogleCredentials; import com.google.firebase.FirebaseApp; +import com.google.firebase.FirebaseOptions; import com.google.firebase.ImplFirebaseTrampolines; import java.io.IOException; @@ -32,35 +32,41 @@ public class FirebaseRequestInitializer implements HttpRequestInitializer { private final HttpCredentialsAdapter credentialsAdapter; - private final HttpRetryConfig retryConfig; - private final int connectTimeout; - private final int readTimeout; + private final TimeoutInitializer timeoutInitializer; + private final RetryInitializer retryInitializer; public FirebaseRequestInitializer(FirebaseApp app) { this(app, null); } - public FirebaseRequestInitializer(FirebaseApp app, @Nullable HttpRetryConfig retryConfig) { + public FirebaseRequestInitializer(FirebaseApp app, @Nullable RetryConfig retryConfig) { GoogleCredentials credentials = ImplFirebaseTrampolines.getCredentials(app); this.credentialsAdapter = new HttpCredentialsAdapter(credentials); - this.retryConfig = retryConfig; - this.connectTimeout = app.getOptions().getConnectTimeout(); - this.readTimeout = app.getOptions().getReadTimeout(); + this.timeoutInitializer = new TimeoutInitializer(app.getOptions()); + this.retryInitializer = new RetryInitializer(this.credentialsAdapter, retryConfig); } @Override - public void initialize(HttpRequest httpRequest) throws IOException { - credentialsAdapter.initialize(httpRequest); - httpRequest.setConnectTimeout(connectTimeout); - httpRequest.setReadTimeout(readTimeout); - if (retryConfig != null) { - httpRequest.setNumberOfRetries(retryConfig.getMaxRetries()); - httpRequest.setUnsuccessfulResponseHandler( - new HttpRetryHandler(credentialsAdapter, retryConfig)); - httpRequest.setIOExceptionHandler( - new HttpBackOffIOExceptionHandler(retryConfig.newBackOff())); - } else { - httpRequest.setNumberOfRetries(0); + public void initialize(HttpRequest request) throws IOException { + credentialsAdapter.initialize(request); + timeoutInitializer.initialize(request); + retryInitializer.initialize(request); + } + + private static class TimeoutInitializer implements HttpRequestInitializer { + + private final int connectTimeout; + private final int readTimeout; + + TimeoutInitializer(FirebaseOptions options) { + this.connectTimeout = options.getConnectTimeout(); + this.readTimeout = options.getReadTimeout(); + } + + @Override + public void initialize(HttpRequest request) { + request.setConnectTimeout(connectTimeout); + request.setReadTimeout(readTimeout); } } } diff --git a/src/main/java/com/google/firebase/internal/HttpRetryConfig.java b/src/main/java/com/google/firebase/internal/RetryConfig.java similarity index 95% rename from src/main/java/com/google/firebase/internal/HttpRetryConfig.java rename to src/main/java/com/google/firebase/internal/RetryConfig.java index 14aa07114..60b11ed56 100644 --- a/src/main/java/com/google/firebase/internal/HttpRetryConfig.java +++ b/src/main/java/com/google/firebase/internal/RetryConfig.java @@ -24,7 +24,7 @@ import java.util.List; import java.util.concurrent.TimeUnit; -public final class HttpRetryConfig { +public final class RetryConfig { private static final int INITIAL_INTERVAL_MILLIS = 500; @@ -32,7 +32,7 @@ public final class HttpRetryConfig { private final int maxRetries; private final ExponentialBackOff.Builder backOffBuilder; - private HttpRetryConfig(Builder builder) { + private RetryConfig(Builder builder) { if (builder.retryStatusCodes != null) { this.retryStatusCodes = ImmutableList.copyOf(builder.retryStatusCodes); } else { @@ -104,8 +104,8 @@ public Builder setBackOffMultiplier(double backOffMultiplier) { return this; } - public HttpRetryConfig build() { - return new HttpRetryConfig(this); + public RetryConfig build() { + return new RetryConfig(this); } } } diff --git a/src/main/java/com/google/firebase/internal/RetryInitializer.java b/src/main/java/com/google/firebase/internal/RetryInitializer.java new file mode 100644 index 000000000..d102dba54 --- /dev/null +++ b/src/main/java/com/google/firebase/internal/RetryInitializer.java @@ -0,0 +1,55 @@ +/* + * Copyright 2019 Google Inc. + * + * 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.internal; + +import static com.google.common.base.Preconditions.checkNotNull; + +import com.google.api.client.http.HttpBackOffIOExceptionHandler; +import com.google.api.client.http.HttpRequest; +import com.google.api.client.http.HttpRequestInitializer; +import com.google.api.client.http.HttpUnsuccessfulResponseHandler; +import com.google.auth.http.HttpCredentialsAdapter; + +final class RetryInitializer implements HttpRequestInitializer { + + private final HttpCredentialsAdapter credentials; + private final RetryConfig retryConfig; + + RetryInitializer(HttpCredentialsAdapter credentials, RetryConfig retryConfig) { + this.credentials = checkNotNull(credentials); + this.retryConfig = retryConfig; + } + + @Override + public void initialize(HttpRequest request) { + if (retryConfig != null) { + request.setNumberOfRetries(retryConfig.getMaxRetries()); + request.setUnsuccessfulResponseHandler( + newUnsuccessfulResponseHandler()); + request.setIOExceptionHandler( + new HttpBackOffIOExceptionHandler(retryConfig.newBackOff())); + } else { + request.setNumberOfRetries(0); + } + } + + private HttpUnsuccessfulResponseHandler newUnsuccessfulResponseHandler() { + HttpUnsuccessfulResponseHandler retryingHandler = + new RetryUnsuccessfulResponseHandler(retryConfig); + return new CredentialsResponseHandlerDecorator(credentials, retryingHandler); + } +} diff --git a/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java b/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java similarity index 80% rename from src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java rename to src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java index 23573ff0b..bd09daa09 100644 --- a/src/main/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandler.java +++ b/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java @@ -29,17 +29,17 @@ import java.util.Date; import org.apache.http.client.utils.DateUtils; -final class RetryAfterAwareHttpResponseHandler implements HttpUnsuccessfulResponseHandler { +final class RetryUnsuccessfulResponseHandler implements HttpUnsuccessfulResponseHandler { - private final HttpRetryConfig retryConfig; + private final RetryConfig retryConfig; private final HttpBackOffUnsuccessfulResponseHandler backOffHandler; private final Clock clock; - RetryAfterAwareHttpResponseHandler(HttpRetryConfig retryConfig) { + RetryUnsuccessfulResponseHandler(RetryConfig retryConfig) { this(retryConfig, Clock.SYSTEM); } - RetryAfterAwareHttpResponseHandler(HttpRetryConfig retryConfig, Clock clock) { + RetryUnsuccessfulResponseHandler(RetryConfig retryConfig, Clock clock) { this.retryConfig = checkNotNull(retryConfig); this.backOffHandler = new HttpBackOffUnsuccessfulResponseHandler(retryConfig.newBackOff()); this.clock = checkNotNull(clock); @@ -62,16 +62,16 @@ public boolean handleResponse( return false; } - String retryAfter = response.getHeaders().getRetryAfter(); - if (!Strings.isNullOrEmpty(retryAfter)) { - return handleRetryAfter(retryAfter); + String retryAfterHeader = response.getHeaders().getRetryAfter(); + if (!Strings.isNullOrEmpty(retryAfterHeader)) { + return handleRetryAfterHeader(retryAfterHeader); } return backOffHandler.handleResponse(request, response, true); } - private boolean handleRetryAfter(String retryAfter) { - long delayMillis = parseRetryAfter(retryAfter.trim()); + private boolean handleRetryAfterHeader(String retryAfter) { + long delayMillis = parseRetryAfterHeader(retryAfter.trim()); if (delayMillis > retryConfig.getMaxIntervalMillis()) { return false; } @@ -86,7 +86,7 @@ private boolean handleRetryAfter(String retryAfter) { return true; } - private long parseRetryAfter(String retryAfter) { + private long parseRetryAfterHeader(String retryAfter) { try { return Long.parseLong(retryAfter) * 1000; } catch (NumberFormatException e) { diff --git a/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java b/src/test/java/com/google/firebase/internal/CredentialsResponseHandlerDecoratorTest.java similarity index 75% rename from src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java rename to src/test/java/com/google/firebase/internal/CredentialsResponseHandlerDecoratorTest.java index 0ec1e39d1..4ddde92d8 100644 --- a/src/test/java/com/google/firebase/internal/HttpRetryHandlerTest.java +++ b/src/test/java/com/google/firebase/internal/CredentialsResponseHandlerDecoratorTest.java @@ -41,37 +41,28 @@ import java.io.IOException; import org.junit.Test; -public class HttpRetryHandlerTest { +public class CredentialsResponseHandlerDecoratorTest { private static final GenericUrl TEST_URL = new GenericUrl("https://firebase.google.com"); - private static final GoogleCredentials TEST_CREDENTIALS = new MockGoogleCredentials(); - private static final HttpRetryConfig RETRY_CONFIG = HttpRetryConfig.builder() + public static final GoogleCredentials TEST_CREDENTIALS = new MockGoogleCredentials(); + private static final RetryConfig RETRY_CONFIG = RetryConfig.builder() + .setMaxRetries(5) .setRetryStatusCodes(ImmutableList.of(503)) .build(); @Test public void testRetryCredentialsCheck() throws IOException { - CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( - new MockLowLevelHttpResponse().setStatusCode(401).setZeroContent()); - HttpRequest request = createRequest(failingRequest); - HttpCredentialsAdapter credentials = new HttpCredentialsAdapter(TEST_CREDENTIALS){ - @Override - public boolean handleResponse( - HttpRequest request, HttpResponse response, boolean supportsRetry) { - String authorization = request.getHeaders().getAuthorization(); - if (!"Bearer retry".equals(authorization)) { - request.getHeaders().setAuthorization("Bearer retry"); - return true; - } - return false; - } - }; MockSleeper sleeper = new MockSleeper(); HttpBackOffUnsuccessfulResponseHandler responseHandler = new HttpBackOffUnsuccessfulResponseHandler(RETRY_CONFIG.newBackOff()) .setSleeper(sleeper); - HttpRetryHandler retryHandler = new HttpRetryHandler(credentials, responseHandler); + CredentialsResponseHandlerDecorator retryHandler = new CredentialsResponseHandlerDecorator( + new MockHttpCredentialsAdapter(), responseHandler); + CountingHttpRequest failingRequest = CountingHttpRequest + .fromResponse(new MockLowLevelHttpResponse().setStatusCode(401).setZeroContent()); + HttpRequest request = createRequest(failingRequest); request.setUnsuccessfulResponseHandler(retryHandler); + try { request.execute(); fail("No exception thrown for HTTP error"); @@ -87,29 +78,19 @@ public boolean handleResponse( @Test public void testDelegateCalledAfterCredentials() throws IOException { - CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( - new MockLowLevelHttpResponse().setStatusCode(401).setZeroContent()); - HttpRequest request = createRequest(failingRequest); - HttpCredentialsAdapter credentials = new HttpCredentialsAdapter(TEST_CREDENTIALS){ - @Override - public boolean handleResponse( - HttpRequest request, HttpResponse response, boolean supportsRetry) { - String authorization = request.getHeaders().getAuthorization(); - if (!"Bearer retry".equals(authorization)) { - request.getHeaders().setAuthorization("Bearer retry"); - return true; - } - return false; - } - }; MockSleeper sleeper = new MockSleeper(); HttpBackOffUnsuccessfulResponseHandler responseHandler = new HttpBackOffUnsuccessfulResponseHandler(RETRY_CONFIG.newBackOff()) .setSleeper(sleeper) .setBackOffRequired(HttpBackOffUnsuccessfulResponseHandler.BackOffRequired.ALWAYS); - HttpRetryHandler retryHandler = new HttpRetryHandler(credentials, responseHandler); - request.setUnsuccessfulResponseHandler(retryHandler); + CredentialsResponseHandlerDecorator retryHandler = new CredentialsResponseHandlerDecorator( + new MockHttpCredentialsAdapter(), responseHandler); + CountingHttpRequest failingRequest = CountingHttpRequest + .fromResponse(new MockLowLevelHttpResponse().setStatusCode(401).setZeroContent()); + HttpRequest request = createRequest(failingRequest); request.setNumberOfRetries(4); + request.setUnsuccessfulResponseHandler(retryHandler); + try { request.execute(); fail("No exception thrown for HTTP error"); @@ -123,6 +104,24 @@ public boolean handleResponse( assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); } + private static class MockHttpCredentialsAdapter extends HttpCredentialsAdapter { + + private MockHttpCredentialsAdapter() { + super(TEST_CREDENTIALS); + } + + @Override + public boolean handleResponse( + HttpRequest request, HttpResponse response, boolean supportsRetry) { + String authorization = request.getHeaders().getAuthorization(); + if (!"Bearer retry".equals(authorization)) { + request.getHeaders().setAuthorization("Bearer retry"); + return true; + } + return false; + } + } + private HttpRequest createRequest(MockLowLevelHttpRequest request) throws IOException { HttpTransport transport = new MockHttpTransport.Builder() .setLowLevelHttpRequest(request) @@ -162,4 +161,5 @@ int getCount() { return count; } } + } diff --git a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java index 90885adf6..015861972 100644 --- a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java @@ -109,7 +109,7 @@ public void testExplicitRetryConfig() throws Exception { .setCredentials(new MockGoogleCredentials("token")) .build()); HttpTransport transport = new MockHttpTransport(); - HttpRetryConfig retryConfig = HttpRetryConfig.builder() + RetryConfig retryConfig = RetryConfig.builder() .setMaxRetries(5) .build(); HttpRequestFactory factory = transport.createRequestFactory( @@ -123,6 +123,7 @@ public void testExplicitRetryConfig() throws Exception { assertEquals("Bearer token", request.getHeaders().getAuthorization()); assertEquals(5, request.getNumberOfRetries()); assertTrue(request.getIOExceptionHandler() instanceof HttpBackOffIOExceptionHandler); - assertTrue(request.getUnsuccessfulResponseHandler() instanceof HttpRetryHandler); + assertTrue( + request.getUnsuccessfulResponseHandler() instanceof CredentialsResponseHandlerDecorator); } } diff --git a/src/test/java/com/google/firebase/internal/HttpRetryConfigTest.java b/src/test/java/com/google/firebase/internal/RetryConfigTest.java similarity index 91% rename from src/test/java/com/google/firebase/internal/HttpRetryConfigTest.java rename to src/test/java/com/google/firebase/internal/RetryConfigTest.java index 9dbbf9f83..37c144ae6 100644 --- a/src/test/java/com/google/firebase/internal/HttpRetryConfigTest.java +++ b/src/test/java/com/google/firebase/internal/RetryConfigTest.java @@ -26,11 +26,11 @@ import java.io.IOException; import org.junit.Test; -public class HttpRetryConfigTest { +public class RetryConfigTest { @Test public void testEmptyBuilder() { - HttpRetryConfig config = HttpRetryConfig.builder().build(); + RetryConfig config = RetryConfig.builder().build(); assertTrue(config.getRetryStatusCodes().isEmpty()); assertEquals(0, config.getMaxRetries()); @@ -46,9 +46,9 @@ public void testEmptyBuilder() { } @Test - public void testBuilder() { + public void testBuilderWithAllSettings() { ImmutableList statusCodes = ImmutableList.of(500, 503); - HttpRetryConfig config = HttpRetryConfig.builder() + RetryConfig config = RetryConfig.builder() .setMaxRetries(4) .setRetryStatusCodes(statusCodes) .setMaxIntervalMillis(5 * 60 * 1000) @@ -72,7 +72,7 @@ public void testBuilder() { @Test public void testExponentialBackOff() throws IOException { - HttpRetryConfig config = HttpRetryConfig.builder() + RetryConfig config = RetryConfig.builder() .setMaxIntervalMillis(12000) .build(); @@ -88,21 +88,21 @@ public void testExponentialBackOff() throws IOException { @Test(expected = IllegalArgumentException.class) public void testNegativeMaxRetriesNotAllowed() { - HttpRetryConfig.builder() + RetryConfig.builder() .setMaxRetries(-1) .build(); } @Test(expected = IllegalArgumentException.class) public void testMaxIntervalMillisTooSmall() { - HttpRetryConfig.builder() + RetryConfig.builder() .setMaxIntervalMillis(499) .build(); } @Test(expected = IllegalArgumentException.class) public void testBackOffMultiplierTooSmall() { - HttpRetryConfig.builder() + RetryConfig.builder() .setBackOffMultiplier(0.99) .build(); } diff --git a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java new file mode 100644 index 000000000..5f068b433 --- /dev/null +++ b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java @@ -0,0 +1,79 @@ +/* + * Copyright 2019 Google Inc. + * + * 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.internal; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import com.google.api.client.http.EmptyContent; +import com.google.api.client.http.GenericUrl; +import com.google.api.client.http.HttpBackOffIOExceptionHandler; +import com.google.api.client.http.HttpRequest; +import com.google.api.client.http.HttpRequestFactory; +import com.google.api.client.http.HttpTransport; +import com.google.api.client.testing.http.MockHttpTransport; +import com.google.api.client.testing.http.MockLowLevelHttpRequest; +import com.google.auth.http.HttpCredentialsAdapter; +import com.google.common.collect.ImmutableList; +import com.google.firebase.auth.MockGoogleCredentials; +import java.io.IOException; +import org.junit.Test; + +public class RetryInitializerTest { + + private static final GenericUrl TEST_URL = new GenericUrl("https://firebase.google.com"); + private static final HttpCredentialsAdapter TEST_CREDENTIALS = new HttpCredentialsAdapter( + new MockGoogleCredentials()); + private static final RetryConfig RETRY_CONFIG = RetryConfig.builder() + .setMaxRetries(5) + .setRetryStatusCodes(ImmutableList.of(503)) + .build(); + + @Test + public void testEnableRetry() throws IOException { + RetryInitializer initializer = new RetryInitializer(TEST_CREDENTIALS, RETRY_CONFIG); + HttpRequest request = createRequest(); + + initializer.initialize(request); + + assertEquals(5, request.getNumberOfRetries()); + assertTrue( + request.getUnsuccessfulResponseHandler() instanceof CredentialsResponseHandlerDecorator); + assertTrue(request.getIOExceptionHandler() instanceof HttpBackOffIOExceptionHandler); + } + + @Test + public void testDisableRetry() throws IOException { + RetryInitializer initializer = new RetryInitializer(TEST_CREDENTIALS, null); + HttpRequest request = createRequest(); + + initializer.initialize(request); + + assertEquals(0, request.getNumberOfRetries()); + assertNull(request.getUnsuccessfulResponseHandler()); + assertNull(request.getIOExceptionHandler()); + } + + private HttpRequest createRequest() throws IOException { + HttpTransport transport = new MockHttpTransport.Builder() + .setLowLevelHttpRequest(new MockLowLevelHttpRequest()) + .build(); + HttpRequestFactory requestFactory = transport.createRequestFactory(); + return requestFactory.buildPostRequest(TEST_URL, new EmptyContent()); + } +} diff --git a/src/test/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandlerTest.java b/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java similarity index 90% rename from src/test/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandlerTest.java rename to src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java index 171224671..b23942367 100644 --- a/src/test/java/com/google/firebase/internal/RetryAfterAwareHttpResponseHandlerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java @@ -44,16 +44,16 @@ import java.util.TimeZone; import org.junit.Test; -public class RetryAfterAwareHttpResponseHandlerTest { +public class RetryUnsuccessfulResponseHandlerTest { private static final GenericUrl TEST_URL = new GenericUrl("https://firebase.google.com"); - private static final HttpRetryConfig TEST_RETRY_CONFIG = HttpRetryConfig.builder() + private static final RetryConfig TEST_RETRY_CONFIG = RetryConfig.builder() .setRetryStatusCodes(ImmutableList.of(503)) .build(); @Test public void testDoesNotRetryOnUnspecifiedHttpStatus() throws IOException { - RetryAfterAwareHttpResponseHandler handler = new RetryAfterAwareHttpResponseHandler( + RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( TEST_RETRY_CONFIG); MultipleCallSleeper sleeper = new MultipleCallSleeper(); handler.setSleeper(sleeper); @@ -78,8 +78,8 @@ public void testDoesNotRetryOnUnspecifiedHttpStatus() throws IOException { } @Test - public void testRetryWithBackOffWhenRetryAfterIsAbsent() throws IOException { - RetryAfterAwareHttpResponseHandler handler = new RetryAfterAwareHttpResponseHandler( + public void testRetryAfterIsAbsent() throws IOException { + RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( TEST_RETRY_CONFIG); MultipleCallSleeper sleeper = new MultipleCallSleeper(); handler.setSleeper(sleeper); @@ -102,8 +102,8 @@ public void testRetryWithBackOffWhenRetryAfterIsAbsent() throws IOException { } @Test - public void testRetryWithRetryAfterGivenAsSeconds() throws IOException { - RetryAfterAwareHttpResponseHandler handler = new RetryAfterAwareHttpResponseHandler( + public void testRetryAfterGivenAsSeconds() throws IOException { + RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( TEST_RETRY_CONFIG); MultipleCallSleeper sleeper = new MultipleCallSleeper(); handler.setSleeper(sleeper); @@ -129,14 +129,14 @@ public void testRetryWithRetryAfterGivenAsSeconds() throws IOException { } @Test - public void testRetryWithRetryAfterGivenAsDate() throws IOException { + public void testRetryAfterGivenAsDate() throws IOException { SimpleDateFormat dateFormat = new SimpleDateFormat("EEE, dd MMM yyyy HH:mm:ss zzz"); dateFormat.setTimeZone(TimeZone.getTimeZone("GMT")); Date date = new Date(1000); Clock clock = new FixedClock(date.getTime()); String retryAfter = dateFormat.format(new Date(date.getTime() + 30000)); - RetryAfterAwareHttpResponseHandler handler = new RetryAfterAwareHttpResponseHandler( + RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( TEST_RETRY_CONFIG, clock); MultipleCallSleeper sleeper = new MultipleCallSleeper(); handler.setSleeper(sleeper); @@ -163,7 +163,7 @@ public void testRetryWithRetryAfterGivenAsDate() throws IOException { @Test public void testDoesNotRetryWhenRetryAfterIsTooLong() throws IOException { - RetryAfterAwareHttpResponseHandler handler = new RetryAfterAwareHttpResponseHandler( + RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( TEST_RETRY_CONFIG); MultipleCallSleeper sleeper = new MultipleCallSleeper(); handler.setSleeper(sleeper); From f59e809fd478239fd1f0bfa97767e6bebd9f085c Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Sun, 17 Feb 2019 17:11:12 -0800 Subject: [PATCH 07/19] Refactored retry impl and tests --- .../CredentialsResponseHandlerDecorator.java | 50 ------ .../google/firebase/internal/RetryConfig.java | 16 ++ .../firebase/internal/RetryInitializer.java | 20 ++- .../RetryUnsuccessfulResponseHandler.java | 45 +++-- ...edentialsResponseHandlerDecoratorTest.java | 165 ------------------ .../FirebaseRequestInitializerTest.java | 4 +- .../firebase/internal/RetryConfigTest.java | 14 ++ .../internal/RetryInitializerTest.java | 134 +++++++++++++- .../RetryUnsuccessfulResponseHandlerTest.java | 85 +++++++-- 9 files changed, 270 insertions(+), 263 deletions(-) delete mode 100644 src/main/java/com/google/firebase/internal/CredentialsResponseHandlerDecorator.java delete mode 100644 src/test/java/com/google/firebase/internal/CredentialsResponseHandlerDecoratorTest.java diff --git a/src/main/java/com/google/firebase/internal/CredentialsResponseHandlerDecorator.java b/src/main/java/com/google/firebase/internal/CredentialsResponseHandlerDecorator.java deleted file mode 100644 index 5ef4a673f..000000000 --- a/src/main/java/com/google/firebase/internal/CredentialsResponseHandlerDecorator.java +++ /dev/null @@ -1,50 +0,0 @@ -/* - * Copyright 2019 Google Inc. - * - * 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.internal; - -import static com.google.common.base.Preconditions.checkNotNull; - -import com.google.api.client.http.HttpRequest; -import com.google.api.client.http.HttpResponse; -import com.google.api.client.http.HttpUnsuccessfulResponseHandler; -import com.google.auth.http.HttpCredentialsAdapter; -import java.io.IOException; - -final class CredentialsResponseHandlerDecorator implements HttpUnsuccessfulResponseHandler { - - private final HttpCredentialsAdapter credentials; - private final HttpUnsuccessfulResponseHandler delegate; - - CredentialsResponseHandlerDecorator( - HttpCredentialsAdapter credentials, HttpUnsuccessfulResponseHandler delegate) { - this.credentials = checkNotNull(credentials); - this.delegate = checkNotNull(delegate); - } - - @Override - public boolean handleResponse( - HttpRequest request, HttpResponse response, boolean supportsRetry) throws IOException { - - boolean retry = credentials.handleResponse(request, response, supportsRetry); - if (!retry) { - retry = delegate.handleResponse(request, response, supportsRetry); - } - - request.setUnsuccessfulResponseHandler(this); - return retry; - } -} diff --git a/src/main/java/com/google/firebase/internal/RetryConfig.java b/src/main/java/com/google/firebase/internal/RetryConfig.java index 60b11ed56..b9acdee81 100644 --- a/src/main/java/com/google/firebase/internal/RetryConfig.java +++ b/src/main/java/com/google/firebase/internal/RetryConfig.java @@ -17,9 +17,12 @@ package com.google.firebase.internal; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import com.google.api.client.util.BackOff; import com.google.api.client.util.ExponentialBackOff; +import com.google.api.client.util.Sleeper; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import java.util.List; import java.util.concurrent.TimeUnit; @@ -30,6 +33,7 @@ public final class RetryConfig { private final List retryStatusCodes; private final int maxRetries; + private final Sleeper sleeper; private final ExponentialBackOff.Builder backOffBuilder; private RetryConfig(Builder builder) { @@ -41,6 +45,7 @@ private RetryConfig(Builder builder) { checkArgument(builder.maxRetries >= 0, "maxRetries must not be negative"); this.maxRetries = builder.maxRetries; + this.sleeper = checkNotNull(builder.sleeper); this.backOffBuilder = new ExponentialBackOff.Builder() .setInitialIntervalMillis(INITIAL_INTERVAL_MILLIS) .setMaxIntervalMillis(builder.maxIntervalMillis) @@ -67,6 +72,10 @@ int getMaxIntervalMillis() { return backOffBuilder.getMultiplier(); } + Sleeper getSleeper() { + return sleeper; + } + BackOff newBackOff() { return backOffBuilder.build(); } @@ -81,6 +90,7 @@ public static final class Builder { private int maxRetries; private int maxIntervalMillis = (int) TimeUnit.MINUTES.toMillis(2); private double backOffMultiplier = 2.0; + private Sleeper sleeper = Sleeper.DEFAULT; private Builder() { } @@ -104,6 +114,12 @@ public Builder setBackOffMultiplier(double backOffMultiplier) { return this; } + @VisibleForTesting + Builder setSleeper(Sleeper sleeper) { + this.sleeper = sleeper; + return this; + } + public RetryConfig build() { return new RetryConfig(this); } diff --git a/src/main/java/com/google/firebase/internal/RetryInitializer.java b/src/main/java/com/google/firebase/internal/RetryInitializer.java index d102dba54..9917e43c8 100644 --- a/src/main/java/com/google/firebase/internal/RetryInitializer.java +++ b/src/main/java/com/google/firebase/internal/RetryInitializer.java @@ -21,8 +21,10 @@ import com.google.api.client.http.HttpBackOffIOExceptionHandler; import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpRequestInitializer; +import com.google.api.client.http.HttpResponse; import com.google.api.client.http.HttpUnsuccessfulResponseHandler; import com.google.auth.http.HttpCredentialsAdapter; +import java.io.IOException; final class RetryInitializer implements HttpRequestInitializer { @@ -48,8 +50,22 @@ public void initialize(HttpRequest request) { } private HttpUnsuccessfulResponseHandler newUnsuccessfulResponseHandler() { - HttpUnsuccessfulResponseHandler retryingHandler = + final HttpUnsuccessfulResponseHandler retryHandler = new RetryUnsuccessfulResponseHandler(retryConfig); - return new CredentialsResponseHandlerDecorator(credentials, retryingHandler); + return new HttpUnsuccessfulResponseHandler() { + @Override + public boolean handleResponse( + HttpRequest request, + HttpResponse response, + boolean supportsRetry) throws IOException { + boolean retry = credentials.handleResponse(request, response, supportsRetry); + if (!retry) { + retry = retryHandler.handleResponse(request, response, supportsRetry); + } + + request.setUnsuccessfulResponseHandler(this); + return retry; + } + }; } } diff --git a/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java b/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java index bd09daa09..f75658614 100644 --- a/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java +++ b/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java @@ -23,7 +23,6 @@ import com.google.api.client.http.HttpResponse; import com.google.api.client.http.HttpUnsuccessfulResponseHandler; import com.google.api.client.util.Clock; -import com.google.api.client.util.Sleeper; import com.google.common.base.Strings; import java.io.IOException; import java.util.Date; @@ -41,14 +40,12 @@ final class RetryUnsuccessfulResponseHandler implements HttpUnsuccessfulResponse RetryUnsuccessfulResponseHandler(RetryConfig retryConfig, Clock clock) { this.retryConfig = checkNotNull(retryConfig); - this.backOffHandler = new HttpBackOffUnsuccessfulResponseHandler(retryConfig.newBackOff()); + this.backOffHandler = new HttpBackOffUnsuccessfulResponseHandler(retryConfig.newBackOff()) + .setBackOffRequired(HttpBackOffUnsuccessfulResponseHandler.BackOffRequired.ALWAYS) + .setSleeper(retryConfig.getSleeper()); this.clock = checkNotNull(clock); } - void setSleeper(Sleeper sleeper) { - backOffHandler.setSleeper(sleeper); - } - @Override public boolean handleResponse( HttpRequest request, HttpResponse response, boolean supportsRetry) throws IOException { @@ -64,28 +61,15 @@ public boolean handleResponse( String retryAfterHeader = response.getHeaders().getRetryAfter(); if (!Strings.isNullOrEmpty(retryAfterHeader)) { - return handleRetryAfterHeader(retryAfterHeader); + long delayMillis = parseRetryAfterHeader(retryAfterHeader.trim()); + if (delayMillis > 0) { + return waitFor(delayMillis); + } } return backOffHandler.handleResponse(request, response, true); } - private boolean handleRetryAfterHeader(String retryAfter) { - long delayMillis = parseRetryAfterHeader(retryAfter.trim()); - if (delayMillis > retryConfig.getMaxIntervalMillis()) { - return false; - } - - if (delayMillis > 0) { - try { - backOffHandler.getSleeper().sleep(delayMillis); - } catch (InterruptedException e) { - // ignore - } - } - return true; - } - private long parseRetryAfterHeader(String retryAfter) { try { return Long.parseLong(retryAfter) * 1000; @@ -95,6 +79,19 @@ private long parseRetryAfterHeader(String retryAfter) { return date.getTime() - clock.currentTimeMillis(); } } - return 0L; + return -1L; + } + + private boolean waitFor(long delayMillis) { + if (delayMillis > retryConfig.getMaxIntervalMillis()) { + return false; + } + + try { + backOffHandler.getSleeper().sleep(delayMillis); + } catch (InterruptedException e) { + // ignore + } + return true; } } diff --git a/src/test/java/com/google/firebase/internal/CredentialsResponseHandlerDecoratorTest.java b/src/test/java/com/google/firebase/internal/CredentialsResponseHandlerDecoratorTest.java deleted file mode 100644 index 4ddde92d8..000000000 --- a/src/test/java/com/google/firebase/internal/CredentialsResponseHandlerDecoratorTest.java +++ /dev/null @@ -1,165 +0,0 @@ -/* - * Copyright 2019 Google Inc. - * - * 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.internal; - -import static com.google.common.base.Preconditions.checkNotNull; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertSame; -import static org.junit.Assert.fail; - -import com.google.api.client.http.EmptyContent; -import com.google.api.client.http.GenericUrl; -import com.google.api.client.http.HttpBackOffUnsuccessfulResponseHandler; -import com.google.api.client.http.HttpRequest; -import com.google.api.client.http.HttpRequestFactory; -import com.google.api.client.http.HttpResponse; -import com.google.api.client.http.HttpResponseException; -import com.google.api.client.http.HttpTransport; -import com.google.api.client.http.LowLevelHttpResponse; -import com.google.api.client.testing.http.MockHttpTransport; -import com.google.api.client.testing.http.MockLowLevelHttpRequest; -import com.google.api.client.testing.http.MockLowLevelHttpResponse; -import com.google.api.client.testing.util.MockSleeper; -import com.google.auth.http.HttpCredentialsAdapter; -import com.google.auth.oauth2.GoogleCredentials; -import com.google.common.collect.ImmutableList; -import com.google.firebase.auth.MockGoogleCredentials; -import java.io.IOException; -import org.junit.Test; - -public class CredentialsResponseHandlerDecoratorTest { - - private static final GenericUrl TEST_URL = new GenericUrl("https://firebase.google.com"); - public static final GoogleCredentials TEST_CREDENTIALS = new MockGoogleCredentials(); - private static final RetryConfig RETRY_CONFIG = RetryConfig.builder() - .setMaxRetries(5) - .setRetryStatusCodes(ImmutableList.of(503)) - .build(); - - @Test - public void testRetryCredentialsCheck() throws IOException { - MockSleeper sleeper = new MockSleeper(); - HttpBackOffUnsuccessfulResponseHandler responseHandler = - new HttpBackOffUnsuccessfulResponseHandler(RETRY_CONFIG.newBackOff()) - .setSleeper(sleeper); - CredentialsResponseHandlerDecorator retryHandler = new CredentialsResponseHandlerDecorator( - new MockHttpCredentialsAdapter(), responseHandler); - CountingHttpRequest failingRequest = CountingHttpRequest - .fromResponse(new MockLowLevelHttpResponse().setStatusCode(401).setZeroContent()); - HttpRequest request = createRequest(failingRequest); - request.setUnsuccessfulResponseHandler(retryHandler); - - try { - request.execute(); - fail("No exception thrown for HTTP error"); - } catch (HttpResponseException e) { - assertEquals(401, e.getStatusCode()); - } - - assertEquals("Bearer retry", request.getHeaders().getAuthorization()); - assertEquals(0, sleeper.getCount()); - assertEquals(2, failingRequest.getCount()); - assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); - } - - @Test - public void testDelegateCalledAfterCredentials() throws IOException { - MockSleeper sleeper = new MockSleeper(); - HttpBackOffUnsuccessfulResponseHandler responseHandler = - new HttpBackOffUnsuccessfulResponseHandler(RETRY_CONFIG.newBackOff()) - .setSleeper(sleeper) - .setBackOffRequired(HttpBackOffUnsuccessfulResponseHandler.BackOffRequired.ALWAYS); - CredentialsResponseHandlerDecorator retryHandler = new CredentialsResponseHandlerDecorator( - new MockHttpCredentialsAdapter(), responseHandler); - CountingHttpRequest failingRequest = CountingHttpRequest - .fromResponse(new MockLowLevelHttpResponse().setStatusCode(401).setZeroContent()); - HttpRequest request = createRequest(failingRequest); - request.setNumberOfRetries(4); - request.setUnsuccessfulResponseHandler(retryHandler); - - try { - request.execute(); - fail("No exception thrown for HTTP error"); - } catch (HttpResponseException e) { - assertEquals(401, e.getStatusCode()); - } - - assertEquals("Bearer retry", request.getHeaders().getAuthorization()); - assertEquals(3, sleeper.getCount()); - assertEquals(5, failingRequest.getCount()); - assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); - } - - private static class MockHttpCredentialsAdapter extends HttpCredentialsAdapter { - - private MockHttpCredentialsAdapter() { - super(TEST_CREDENTIALS); - } - - @Override - public boolean handleResponse( - HttpRequest request, HttpResponse response, boolean supportsRetry) { - String authorization = request.getHeaders().getAuthorization(); - if (!"Bearer retry".equals(authorization)) { - request.getHeaders().setAuthorization("Bearer retry"); - return true; - } - return false; - } - } - - private HttpRequest createRequest(MockLowLevelHttpRequest request) throws IOException { - HttpTransport transport = new MockHttpTransport.Builder() - .setLowLevelHttpRequest(request) - .build(); - HttpRequestFactory requestFactory = transport.createRequestFactory(); - return requestFactory.buildPostRequest(TEST_URL, new EmptyContent()); - } - - private static class CountingHttpRequest extends MockLowLevelHttpRequest { - - private final LowLevelHttpResponse response; - private final IOException exception; - private int count; - - private CountingHttpRequest(LowLevelHttpResponse response, IOException exception) { - this.response = response; - this.exception = exception; - } - - static CountingHttpRequest fromResponse(LowLevelHttpResponse response) { - return new CountingHttpRequest(checkNotNull(response), null); - } - - @Override - public void addHeader(String name, String value) { } - - @Override - public LowLevelHttpResponse execute() throws IOException { - count++; - if (response != null) { - return response; - } - throw exception; - } - - int getCount() { - return count; - } - } - -} diff --git a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java index 015861972..70b5a88ad 100644 --- a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java @@ -17,6 +17,7 @@ package com.google.firebase.internal; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -123,7 +124,6 @@ public void testExplicitRetryConfig() throws Exception { assertEquals("Bearer token", request.getHeaders().getAuthorization()); assertEquals(5, request.getNumberOfRetries()); assertTrue(request.getIOExceptionHandler() instanceof HttpBackOffIOExceptionHandler); - assertTrue( - request.getUnsuccessfulResponseHandler() instanceof CredentialsResponseHandlerDecorator); + assertNotNull(request.getUnsuccessfulResponseHandler()); } } diff --git a/src/test/java/com/google/firebase/internal/RetryConfigTest.java b/src/test/java/com/google/firebase/internal/RetryConfigTest.java index 37c144ae6..88be103ea 100644 --- a/src/test/java/com/google/firebase/internal/RetryConfigTest.java +++ b/src/test/java/com/google/firebase/internal/RetryConfigTest.java @@ -18,10 +18,13 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; +import com.google.api.client.testing.util.MockSleeper; import com.google.api.client.util.BackOff; import com.google.api.client.util.ExponentialBackOff; +import com.google.api.client.util.Sleeper; import com.google.common.collect.ImmutableList; import java.io.IOException; import org.junit.Test; @@ -36,6 +39,7 @@ public void testEmptyBuilder() { assertEquals(0, config.getMaxRetries()); assertEquals(2 * 60 * 1000, config.getMaxIntervalMillis()); assertEquals(2.0, config.getBackOffMultiplier(), 0.01); + assertSame(Sleeper.DEFAULT, config.getSleeper()); ExponentialBackOff backOff = (ExponentialBackOff) config.newBackOff(); assertEquals(2 * 60 * 1000, backOff.getMaxIntervalMillis()); @@ -48,11 +52,13 @@ public void testEmptyBuilder() { @Test public void testBuilderWithAllSettings() { ImmutableList statusCodes = ImmutableList.of(500, 503); + Sleeper sleeper = new MockSleeper(); RetryConfig config = RetryConfig.builder() .setMaxRetries(4) .setRetryStatusCodes(statusCodes) .setMaxIntervalMillis(5 * 60 * 1000) .setBackOffMultiplier(1.5) + .setSleeper(sleeper) .build(); assertEquals(2, config.getRetryStatusCodes().size()); @@ -61,6 +67,7 @@ public void testBuilderWithAllSettings() { assertEquals(4, config.getMaxRetries()); assertEquals(5 * 60 * 1000, config.getMaxIntervalMillis()); assertEquals(1.5, config.getBackOffMultiplier(), 0.01); + assertSame(sleeper, config.getSleeper()); ExponentialBackOff backOff = (ExponentialBackOff) config.newBackOff(); assertEquals(500, backOff.getInitialIntervalMillis()); @@ -106,4 +113,11 @@ public void testBackOffMultiplierTooSmall() { .setBackOffMultiplier(0.99) .build(); } + + @Test(expected = NullPointerException.class) + public void testSleeperCannotBeNull() { + RetryConfig.builder() + .setSleeper(null) + .build(); + } } diff --git a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java index 5f068b433..8f671189f 100644 --- a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java @@ -16,18 +16,28 @@ package com.google.firebase.internal; +import static com.google.common.base.Preconditions.checkNotNull; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import com.google.api.client.http.EmptyContent; import com.google.api.client.http.GenericUrl; import com.google.api.client.http.HttpBackOffIOExceptionHandler; import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpRequestFactory; +import com.google.api.client.http.HttpResponse; +import com.google.api.client.http.HttpResponseException; import com.google.api.client.http.HttpTransport; +import com.google.api.client.http.HttpUnsuccessfulResponseHandler; +import com.google.api.client.http.LowLevelHttpResponse; import com.google.api.client.testing.http.MockHttpTransport; import com.google.api.client.testing.http.MockLowLevelHttpRequest; +import com.google.api.client.testing.http.MockLowLevelHttpResponse; +import com.google.api.client.testing.util.MockSleeper; import com.google.auth.http.HttpCredentialsAdapter; import com.google.common.collect.ImmutableList; import com.google.firebase.auth.MockGoogleCredentials; @@ -52,8 +62,7 @@ public void testEnableRetry() throws IOException { initializer.initialize(request); assertEquals(5, request.getNumberOfRetries()); - assertTrue( - request.getUnsuccessfulResponseHandler() instanceof CredentialsResponseHandlerDecorator); + assertNotNull(request.getUnsuccessfulResponseHandler()); assertTrue(request.getIOExceptionHandler() instanceof HttpBackOffIOExceptionHandler); } @@ -69,6 +78,127 @@ public void testDisableRetry() throws IOException { assertNull(request.getIOExceptionHandler()); } + @Test + public void testRetryCredentialsCheck() throws IOException { + MockSleeper sleeper = new MockSleeper(); + HttpCredentialsAdapter credentials = new HttpCredentialsAdapter(new MockGoogleCredentials()){ + @Override + public boolean handleResponse(HttpRequest request, HttpResponse response, boolean + supportsRetry) { + String auth = request.getHeaders().getAuthorization(); + if (!"Bearer retry".equals(auth)) { + request.getHeaders().setAuthorization("Bearer retry"); + return true; + } + return false; + } + }; + RetryInitializer initializer = new RetryInitializer(credentials, RetryConfig.builder() + .setMaxRetries(4) + .setRetryStatusCodes(ImmutableList.of(503)) + .setSleeper(sleeper) + .build()); + + CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + new MockLowLevelHttpResponse().setStatusCode(401).setZeroContent()); + HttpRequest request = createRequest(failingRequest); + initializer.initialize(request); + final HttpUnsuccessfulResponseHandler retryHandler = request.getUnsuccessfulResponseHandler(); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (HttpResponseException e) { + assertEquals(401, e.getStatusCode()); + } + + assertEquals("Bearer retry", request.getHeaders().getAuthorization()); + assertEquals(0, sleeper.getCount()); + assertEquals(2, failingRequest.getCount()); + assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); + } + + @Test + public void testDelegateCalledAfterCredentials() throws IOException { + MockSleeper sleeper = new MockSleeper(); + HttpCredentialsAdapter credentials = new HttpCredentialsAdapter(new MockGoogleCredentials()){ + @Override + public boolean handleResponse(HttpRequest request, HttpResponse response, boolean + supportsRetry) { + String auth = request.getHeaders().getAuthorization(); + if (!"Bearer retry".equals(auth)) { + request.getHeaders().setAuthorization("Bearer retry"); + return true; + } + return false; + } + }; + RetryInitializer initializer = new RetryInitializer(credentials, RetryConfig.builder() + .setMaxRetries(4) + .setRetryStatusCodes(ImmutableList.of(401)) + .setSleeper(sleeper) + .build()); + + CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + new MockLowLevelHttpResponse().setStatusCode(401).setZeroContent()); + HttpRequest request = createRequest(failingRequest); + initializer.initialize(request); + final HttpUnsuccessfulResponseHandler retryHandler = request.getUnsuccessfulResponseHandler(); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (HttpResponseException e) { + assertEquals(401, e.getStatusCode()); + } + + assertEquals("Bearer retry", request.getHeaders().getAuthorization()); + assertEquals(3, sleeper.getCount()); + assertEquals(5, failingRequest.getCount()); + assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); + } + + + private HttpRequest createRequest(MockLowLevelHttpRequest request) throws IOException { + HttpTransport transport = new MockHttpTransport.Builder() + .setLowLevelHttpRequest(request) + .build(); + HttpRequestFactory requestFactory = transport.createRequestFactory(); + return requestFactory.buildPostRequest(TEST_URL, new EmptyContent()); + } + + private static class CountingHttpRequest extends MockLowLevelHttpRequest { + + private final LowLevelHttpResponse response; + private final IOException exception; + private int count; + + private CountingHttpRequest(LowLevelHttpResponse response, IOException exception) { + this.response = response; + this.exception = exception; + } + + static CountingHttpRequest fromResponse(LowLevelHttpResponse response) { + return new CountingHttpRequest(checkNotNull(response), null); + } + + @Override + public void addHeader(String name, String value) { } + + @Override + public LowLevelHttpResponse execute() throws IOException { + count++; + if (response != null) { + return response; + } + throw exception; + } + + int getCount() { + return count; + } + } + private HttpRequest createRequest() throws IOException { HttpTransport transport = new MockHttpTransport.Builder() .setLowLevelHttpRequest(new MockLowLevelHttpRequest()) diff --git a/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java b/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java index b23942367..33c6324b2 100644 --- a/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java @@ -34,6 +34,7 @@ import com.google.api.client.testing.http.MockLowLevelHttpResponse; import com.google.api.client.testing.util.MockSleeper; import com.google.api.client.util.Clock; +import com.google.api.client.util.Sleeper; import com.google.common.collect.ImmutableList; import com.google.common.primitives.Longs; import java.io.IOException; @@ -47,16 +48,14 @@ public class RetryUnsuccessfulResponseHandlerTest { private static final GenericUrl TEST_URL = new GenericUrl("https://firebase.google.com"); - private static final RetryConfig TEST_RETRY_CONFIG = RetryConfig.builder() - .setRetryStatusCodes(ImmutableList.of(503)) - .build(); + private static final RetryConfig.Builder TEST_RETRY_CONFIG = RetryConfig.builder() + .setRetryStatusCodes(ImmutableList.of(429, 503)); @Test public void testDoesNotRetryOnUnspecifiedHttpStatus() throws IOException { - RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( - TEST_RETRY_CONFIG); MultipleCallSleeper sleeper = new MultipleCallSleeper(); - handler.setSleeper(sleeper); + RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( + testRetryConfig(sleeper)); CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( new MockLowLevelHttpResponse() .addHeader("retry-after", "121") @@ -78,11 +77,34 @@ public void testDoesNotRetryOnUnspecifiedHttpStatus() throws IOException { } @Test - public void testRetryAfterIsAbsent() throws IOException { + public void testRetryOnHttpClientErrorWhenConfigured() throws IOException { + MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( - TEST_RETRY_CONFIG); + testRetryConfig(sleeper)); + CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + new MockLowLevelHttpResponse().setStatusCode(429).setZeroContent()); + HttpRequest request = createRequest(failingRequest); + request.setNumberOfRetries(4); + request.setUnsuccessfulResponseHandler(handler); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (HttpResponseException e) { + assertEquals(429, e.getStatusCode()); + } + + assertEquals(4, sleeper.getCount()); + assertArrayEquals(new long[]{500, 1000, 2000, 4000}, sleeper.getDelays()); + assertEquals(5, failingRequest.getCount()); + } + + + @Test + public void testRetryAfterIsAbsent() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); - handler.setSleeper(sleeper); + RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( + testRetryConfig(sleeper)); CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( new MockLowLevelHttpResponse().setStatusCode(503).setZeroContent()); HttpRequest request = createRequest(failingRequest); @@ -103,10 +125,9 @@ public void testRetryAfterIsAbsent() throws IOException { @Test public void testRetryAfterGivenAsSeconds() throws IOException { - RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( - TEST_RETRY_CONFIG); MultipleCallSleeper sleeper = new MultipleCallSleeper(); - handler.setSleeper(sleeper); + RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( + testRetryConfig(sleeper)); CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( new MockLowLevelHttpResponse() .addHeader("retry-after", "2") @@ -136,10 +157,9 @@ public void testRetryAfterGivenAsDate() throws IOException { Clock clock = new FixedClock(date.getTime()); String retryAfter = dateFormat.format(new Date(date.getTime() + 30000)); - RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( - TEST_RETRY_CONFIG, clock); MultipleCallSleeper sleeper = new MultipleCallSleeper(); - handler.setSleeper(sleeper); + RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( + testRetryConfig(sleeper), clock); CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( new MockLowLevelHttpResponse() .addHeader("retry-after", retryAfter) @@ -162,11 +182,36 @@ public void testRetryAfterGivenAsDate() throws IOException { } @Test - public void testDoesNotRetryWhenRetryAfterIsTooLong() throws IOException { + public void testInvalidRetryAfterFailsOverToExpBackoff() throws IOException { + MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( - TEST_RETRY_CONFIG); + testRetryConfig(sleeper)); + CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + new MockLowLevelHttpResponse() + .addHeader("retry-after", "not valid") + .setStatusCode(503) + .setZeroContent()); + HttpRequest request = createRequest(failingRequest); + request.setNumberOfRetries(4); + request.setUnsuccessfulResponseHandler(handler); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (HttpResponseException e) { + assertEquals(503, e.getStatusCode()); + } + + assertEquals(4, sleeper.getCount()); + assertArrayEquals(new long[]{500, 1000, 2000, 4000}, sleeper.getDelays()); + assertEquals(5, failingRequest.getCount()); + } + + @Test + public void testDoesNotRetryWhenRetryAfterIsTooLong() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); - handler.setSleeper(sleeper); + RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( + testRetryConfig(sleeper)); CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( new MockLowLevelHttpResponse() .addHeader("retry-after", "121") @@ -195,6 +240,10 @@ private HttpRequest createRequest(MockLowLevelHttpRequest request) throws IOExce return requestFactory.buildPostRequest(TEST_URL, new EmptyContent()); } + private RetryConfig testRetryConfig(Sleeper sleeper) { + return TEST_RETRY_CONFIG.setSleeper(sleeper).build(); + } + private static class CountingHttpRequest extends MockLowLevelHttpRequest { private final LowLevelHttpResponse response; From 3918dbfc592440d1cd005f120561bc6e959883ad Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Sun, 17 Feb 2019 23:12:22 -0800 Subject: [PATCH 08/19] Simplified the retry handler --- .../RetryUnsuccessfulResponseHandler.java | 43 ++++++++++--------- .../RetryUnsuccessfulResponseHandlerTest.java | 32 +++++++++++++- 2 files changed, 53 insertions(+), 22 deletions(-) diff --git a/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java b/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java index f75658614..f193f81a7 100644 --- a/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java +++ b/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java @@ -18,11 +18,13 @@ import static com.google.common.base.Preconditions.checkNotNull; -import com.google.api.client.http.HttpBackOffUnsuccessfulResponseHandler; import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpResponse; import com.google.api.client.http.HttpUnsuccessfulResponseHandler; +import com.google.api.client.util.BackOff; +import com.google.api.client.util.BackOffUtils; import com.google.api.client.util.Clock; +import com.google.api.client.util.Sleeper; import com.google.common.base.Strings; import java.io.IOException; import java.util.Date; @@ -31,7 +33,8 @@ final class RetryUnsuccessfulResponseHandler implements HttpUnsuccessfulResponseHandler { private final RetryConfig retryConfig; - private final HttpBackOffUnsuccessfulResponseHandler backOffHandler; + private final BackOff backOff; + private final Sleeper sleeper; private final Clock clock; RetryUnsuccessfulResponseHandler(RetryConfig retryConfig) { @@ -40,9 +43,8 @@ final class RetryUnsuccessfulResponseHandler implements HttpUnsuccessfulResponse RetryUnsuccessfulResponseHandler(RetryConfig retryConfig, Clock clock) { this.retryConfig = checkNotNull(retryConfig); - this.backOffHandler = new HttpBackOffUnsuccessfulResponseHandler(retryConfig.newBackOff()) - .setBackOffRequired(HttpBackOffUnsuccessfulResponseHandler.BackOffRequired.ALWAYS) - .setSleeper(retryConfig.getSleeper()); + this.backOff = retryConfig.newBackOff(); + this.sleeper = retryConfig.getSleeper(); this.clock = checkNotNull(clock); } @@ -59,15 +61,27 @@ public boolean handleResponse( return false; } + try { + return waitAndRetry(response); + } catch (InterruptedException e) { + // ignore + } + return false; + } + + private boolean waitAndRetry(HttpResponse response) throws IOException, InterruptedException { String retryAfterHeader = response.getHeaders().getRetryAfter(); if (!Strings.isNullOrEmpty(retryAfterHeader)) { long delayMillis = parseRetryAfterHeader(retryAfterHeader.trim()); - if (delayMillis > 0) { - return waitFor(delayMillis); + if (delayMillis > retryConfig.getMaxIntervalMillis()) { + return false; + } else if (delayMillis > 0) { + sleeper.sleep(delayMillis); + return true; } } - return backOffHandler.handleResponse(request, response, true); + return BackOffUtils.next(sleeper, backOff); } private long parseRetryAfterHeader(String retryAfter) { @@ -81,17 +95,4 @@ private long parseRetryAfterHeader(String retryAfter) { } return -1L; } - - private boolean waitFor(long delayMillis) { - if (delayMillis > retryConfig.getMaxIntervalMillis()) { - return false; - } - - try { - backOffHandler.getSleeper().sleep(delayMillis); - } catch (InterruptedException e) { - // ignore - } - return true; - } } diff --git a/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java b/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java index 33c6324b2..8eeb1ed9b 100644 --- a/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java @@ -77,7 +77,7 @@ public void testDoesNotRetryOnUnspecifiedHttpStatus() throws IOException { } @Test - public void testRetryOnHttpClientErrorWhenConfigured() throws IOException { + public void testRetryOnHttpClientErrorWhenSpecified() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); @@ -232,6 +232,36 @@ public void testDoesNotRetryWhenRetryAfterIsTooLong() throws IOException { assertEquals(1, failingRequest.getCount()); } + @Test + public void testDoesNotRetryAfterInterruption() throws IOException { + MockSleeper sleeper = new MockSleeper() { + @Override + public void sleep(long millis) throws InterruptedException { + super.sleep(millis); + throw new InterruptedException(); + } + }; + RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( + testRetryConfig(sleeper)); + CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + new MockLowLevelHttpResponse() + .setStatusCode(503) + .setZeroContent()); + HttpRequest request = createRequest(failingRequest); + request.setNumberOfRetries(4); + request.setUnsuccessfulResponseHandler(handler); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (HttpResponseException e) { + assertEquals(503, e.getStatusCode()); + } + + assertEquals(1, sleeper.getCount()); + assertEquals(1, failingRequest.getCount()); + } + private HttpRequest createRequest(MockLowLevelHttpRequest request) throws IOException { HttpTransport transport = new MockHttpTransport.Builder() .setLowLevelHttpRequest(request) From e44db591098221f11c948609d32a323a6988cf2c Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Sun, 24 Feb 2019 01:47:23 -0800 Subject: [PATCH 09/19] More tests and docs --- .../internal/FirebaseRequestInitializer.java | 6 +- .../google/firebase/internal/RetryConfig.java | 33 +++++ .../firebase/internal/RetryInitializer.java | 11 +- .../RetryUnsuccessfulResponseHandler.java | 18 ++- .../internal/CountingLowLevelHttpRequest.java | 57 +++++++++ .../FirebaseRequestInitializerTest.java | 64 ++++------ .../internal/RetryInitializerTest.java | 113 +++++++++++------- .../RetryUnsuccessfulResponseHandlerTest.java | 106 ++++++++-------- 8 files changed, 264 insertions(+), 144 deletions(-) create mode 100644 src/test/java/com/google/firebase/internal/CountingLowLevelHttpRequest.java diff --git a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java index 9d7cda8ce..42fce053d 100644 --- a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java +++ b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java @@ -26,10 +26,10 @@ import java.io.IOException; /** - * {@code HttpRequestInitializer} for configuring outgoing REST calls. Handles OAuth2 authorization - * and setting timeout values. + * {@code HttpRequestInitializer} for configuring outgoing REST calls. Initializes requests with + * OAuth2 credentials, timeout and retry settings. */ -public class FirebaseRequestInitializer implements HttpRequestInitializer { +public final class FirebaseRequestInitializer implements HttpRequestInitializer { private final HttpCredentialsAdapter credentialsAdapter; private final TimeoutInitializer timeoutInitializer; diff --git a/src/main/java/com/google/firebase/internal/RetryConfig.java b/src/main/java/com/google/firebase/internal/RetryConfig.java index b9acdee81..ac169f2cd 100644 --- a/src/main/java/com/google/firebase/internal/RetryConfig.java +++ b/src/main/java/com/google/firebase/internal/RetryConfig.java @@ -27,6 +27,9 @@ import java.util.List; import java.util.concurrent.TimeUnit; +/** + * Configures when and how HTTP requests should be retried. + */ public final class RetryConfig { private static final int INITIAL_INTERVAL_MILLIS = 500; @@ -94,21 +97,51 @@ public static final class Builder { private Builder() { } + /** + * Sets a list of HTTP status codes that should be retried. If null or empty, HTTP requests + * will not be retried as long as they result in some HTTP response message. I/O errors + * will still be retried. + * + * @param retryStatusCodes A list of status codes. + * @return This builder. + */ public Builder setRetryStatusCodes(List retryStatusCodes) { this.retryStatusCodes = retryStatusCodes; return this; } + /** + * Maximum number of retry attempts for a request. This is the cumulative total for all retries + * regardless of their cause (I/O errors and HTTP error responses). + * + * @param maxRetries A non-negative integer. + * @return This builder. + */ public Builder setMaxRetries(int maxRetries) { this.maxRetries = maxRetries; return this; } + /** + * Maximum interval to wait before a request should be retried. Must be at least 500 + * milliseconds. Defaults to 2 minutes. + * + * @param maxIntervalMillis Interval in milliseconds. + * @return This builder. + */ public Builder setMaxIntervalMillis(int maxIntervalMillis) { this.maxIntervalMillis = maxIntervalMillis; return this; } + /** + * Factor by which the retry interval is multiplied when employing exponential back + * off to delay consecutive retries of the same request. Must be at least 1. Defaults + * to 2. + * + * @param backOffMultiplier Multiplication factor for exponential back off. + * @return This builder. + */ public Builder setBackOffMultiplier(double backOffMultiplier) { this.backOffMultiplier = backOffMultiplier; return this; diff --git a/src/main/java/com/google/firebase/internal/RetryInitializer.java b/src/main/java/com/google/firebase/internal/RetryInitializer.java index 9917e43c8..a00b8cb14 100644 --- a/src/main/java/com/google/firebase/internal/RetryInitializer.java +++ b/src/main/java/com/google/firebase/internal/RetryInitializer.java @@ -26,6 +26,12 @@ import com.google.auth.http.HttpCredentialsAdapter; import java.io.IOException; +/** + * Configures HTTP requests to be retried. Failures caused by I/O errors are always retried + * according to the specified {@link RetryConfig}. Failures caused by unsuccessful HTTP responses + * are first referred to the {@code HttpCredentialsAdapter}. If the request does not get retried + * by the credentials, {@link RetryConfig} is used to schedule additional retries. + */ final class RetryInitializer implements HttpRequestInitializer { private final HttpCredentialsAdapter credentials; @@ -43,7 +49,8 @@ public void initialize(HttpRequest request) { request.setUnsuccessfulResponseHandler( newUnsuccessfulResponseHandler()); request.setIOExceptionHandler( - new HttpBackOffIOExceptionHandler(retryConfig.newBackOff())); + new HttpBackOffIOExceptionHandler(retryConfig.newBackOff()) + .setSleeper(retryConfig.getSleeper())); } else { request.setNumberOfRetries(0); } @@ -63,6 +70,8 @@ public boolean handleResponse( retry = retryHandler.handleResponse(request, response, supportsRetry); } + // HttpCredentialsAdapter sometimes resets the unsuccessful response handler on the + // request. This changes it back. request.setUnsuccessfulResponseHandler(this); return retry; } diff --git a/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java b/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java index f193f81a7..0bdde353b 100644 --- a/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java +++ b/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java @@ -30,6 +30,11 @@ import java.util.Date; import org.apache.http.client.utils.DateUtils; +/** + * An {@code HttpUnsuccessfulResponseHandler} that retries failing requests after an interval. The + * interval is determined by checking the Retry-After header on the last response. If that + * header is not present, uses exponential back off to delay subsequent retries. + */ final class RetryUnsuccessfulResponseHandler implements HttpUnsuccessfulResponseHandler { private final RetryConfig retryConfig; @@ -72,11 +77,13 @@ public boolean handleResponse( private boolean waitAndRetry(HttpResponse response) throws IOException, InterruptedException { String retryAfterHeader = response.getHeaders().getRetryAfter(); if (!Strings.isNullOrEmpty(retryAfterHeader)) { - long delayMillis = parseRetryAfterHeader(retryAfterHeader.trim()); - if (delayMillis > retryConfig.getMaxIntervalMillis()) { + long intervalMillis = parseRetryAfterHeaderAsMillis(retryAfterHeader.trim()); + if (intervalMillis > retryConfig.getMaxIntervalMillis()) { return false; - } else if (delayMillis > 0) { - sleeper.sleep(delayMillis); + } + + if (intervalMillis > 0) { + sleeper.sleep(intervalMillis); return true; } } @@ -84,7 +91,7 @@ private boolean waitAndRetry(HttpResponse response) throws IOException, Interrup return BackOffUtils.next(sleeper, backOff); } - private long parseRetryAfterHeader(String retryAfter) { + private long parseRetryAfterHeaderAsMillis(String retryAfter) { try { return Long.parseLong(retryAfter) * 1000; } catch (NumberFormatException e) { @@ -93,6 +100,7 @@ private long parseRetryAfterHeader(String retryAfter) { return date.getTime() - clock.currentTimeMillis(); } } + return -1L; } } diff --git a/src/test/java/com/google/firebase/internal/CountingLowLevelHttpRequest.java b/src/test/java/com/google/firebase/internal/CountingLowLevelHttpRequest.java new file mode 100644 index 000000000..768660efb --- /dev/null +++ b/src/test/java/com/google/firebase/internal/CountingLowLevelHttpRequest.java @@ -0,0 +1,57 @@ +/* + * Copyright 2019 Google Inc. + * + * 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.internal; + +import static com.google.common.base.Preconditions.checkNotNull; + +import com.google.api.client.http.LowLevelHttpResponse; +import com.google.api.client.testing.http.MockLowLevelHttpRequest; +import java.io.IOException; + +class CountingLowLevelHttpRequest extends MockLowLevelHttpRequest { + + private final LowLevelHttpResponse response; + private final IOException exception; + private int count; + + private CountingLowLevelHttpRequest(LowLevelHttpResponse response, IOException exception) { + this.response = response; + this.exception = exception; + } + + static CountingLowLevelHttpRequest fromResponse(LowLevelHttpResponse response) { + return new CountingLowLevelHttpRequest(checkNotNull(response), null); + } + + static CountingLowLevelHttpRequest fromException(IOException exception) { + return new CountingLowLevelHttpRequest(null, checkNotNull(exception)); + } + + @Override + public LowLevelHttpResponse execute() throws IOException { + count++; + if (response != null) { + return response; + } + throw exception; + } + + int getCount() { + return count; + } + +} diff --git a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java index 70b5a88ad..f95c8e196 100644 --- a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java @@ -21,22 +21,27 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import com.google.api.client.http.EmptyContent; import com.google.api.client.http.GenericUrl; import com.google.api.client.http.HttpBackOffIOExceptionHandler; import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpRequestFactory; import com.google.api.client.http.HttpTransport; import com.google.api.client.testing.http.MockHttpTransport; +import com.google.api.client.testing.http.MockLowLevelHttpRequest; import com.google.auth.http.HttpCredentialsAdapter; import com.google.firebase.FirebaseApp; import com.google.firebase.FirebaseOptions; import com.google.firebase.TestOnlyImplFirebaseTrampolines; import com.google.firebase.auth.MockGoogleCredentials; +import java.io.IOException; import org.junit.After; import org.junit.Test; public class FirebaseRequestInitializerTest { + private static final GenericUrl TEST_URL = new GenericUrl("https://firebase.google.com"); + @After public void tearDown() { TestOnlyImplFirebaseTrampolines.clearInstancesForTest(); @@ -47,19 +52,18 @@ public void testDefaultSettings() throws Exception { FirebaseApp app = FirebaseApp.initializeApp(new FirebaseOptions.Builder() .setCredentials(new MockGoogleCredentials("token")) .build()); - HttpTransport transport = new MockHttpTransport(); - HttpRequestFactory factory = transport.createRequestFactory( - new FirebaseRequestInitializer(app)); + HttpRequest request = createRequest(); + + FirebaseRequestInitializer initializer = new FirebaseRequestInitializer(app); + initializer.initialize(request); - HttpRequest request = factory.buildGetRequest( - new GenericUrl("https://firebase.google.com")); assertEquals(0, request.getConnectTimeout()); assertEquals(0, request.getReadTimeout()); assertEquals("Bearer token", request.getHeaders().getAuthorization()); - // assertEquals(4, request.getNumberOfRetries()); - // assertTrue(request.getIOExceptionHandler() instanceof HttpRetryHandler); - // assertTrue(request.getUnsuccessfulResponseHandler() instanceof HttpRetryHandler); + assertEquals(0, request.getNumberOfRetries()); + assertNull(request.getIOExceptionHandler()); + assertTrue(request.getUnsuccessfulResponseHandler() instanceof HttpCredentialsAdapter); } @Test @@ -69,36 +73,14 @@ public void testExplicitTimeouts() throws Exception { .setConnectTimeout(30000) .setReadTimeout(60000) .build()); - HttpTransport transport = new MockHttpTransport(); - HttpRequestFactory factory = transport.createRequestFactory( - new FirebaseRequestInitializer(app)); + HttpRequest request = createRequest(); - HttpRequest request = factory.buildGetRequest( - new GenericUrl("https://firebase.google.com")); + FirebaseRequestInitializer initializer = new FirebaseRequestInitializer(app); + initializer.initialize(request); assertEquals(30000, request.getConnectTimeout()); assertEquals(60000, request.getReadTimeout()); assertEquals("Bearer token", request.getHeaders().getAuthorization()); - // assertEquals(4, request.getNumberOfRetries()); - // assertTrue(request.getIOExceptionHandler() instanceof HttpRetryHandler); - // assertTrue(request.getUnsuccessfulResponseHandler() instanceof HttpRetryHandler); - } - - @Test - public void testNullRetryConfig() throws Exception { - FirebaseApp app = FirebaseApp.initializeApp(new FirebaseOptions.Builder() - .setCredentials(new MockGoogleCredentials("token")) - .build()); - HttpTransport transport = new MockHttpTransport(); - HttpRequestFactory factory = transport.createRequestFactory( - new FirebaseRequestInitializer(app, null)); - - HttpRequest request = factory.buildGetRequest( - new GenericUrl("https://firebase.google.com")); - - assertEquals(0, request.getConnectTimeout()); - assertEquals(0, request.getReadTimeout()); - assertEquals("Bearer token", request.getHeaders().getAuthorization()); assertEquals(0, request.getNumberOfRetries()); assertNull(request.getIOExceptionHandler()); assertTrue(request.getUnsuccessfulResponseHandler() instanceof HttpCredentialsAdapter); @@ -109,15 +91,13 @@ public void testExplicitRetryConfig() throws Exception { FirebaseApp app = FirebaseApp.initializeApp(new FirebaseOptions.Builder() .setCredentials(new MockGoogleCredentials("token")) .build()); - HttpTransport transport = new MockHttpTransport(); RetryConfig retryConfig = RetryConfig.builder() .setMaxRetries(5) .build(); - HttpRequestFactory factory = transport.createRequestFactory( - new FirebaseRequestInitializer(app, retryConfig)); + HttpRequest request = createRequest(); - HttpRequest request = factory.buildGetRequest( - new GenericUrl("https://firebase.google.com")); + FirebaseRequestInitializer initializer = new FirebaseRequestInitializer(app, retryConfig); + initializer.initialize(request); assertEquals(0, request.getConnectTimeout()); assertEquals(0, request.getReadTimeout()); @@ -126,4 +106,12 @@ public void testExplicitRetryConfig() throws Exception { assertTrue(request.getIOExceptionHandler() instanceof HttpBackOffIOExceptionHandler); assertNotNull(request.getUnsuccessfulResponseHandler()); } + + private HttpRequest createRequest() throws IOException { + HttpTransport transport = new MockHttpTransport.Builder() + .setLowLevelHttpRequest(new MockLowLevelHttpRequest()) + .build(); + HttpRequestFactory requestFactory = transport.createRequestFactory(); + return requestFactory.buildPostRequest(TEST_URL, new EmptyContent()); + } } diff --git a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java index 8f671189f..897c2795e 100644 --- a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java @@ -16,7 +16,6 @@ package com.google.firebase.internal; -import static com.google.common.base.Preconditions.checkNotNull; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; @@ -42,6 +41,7 @@ import com.google.common.collect.ImmutableList; import com.google.firebase.auth.MockGoogleCredentials; import java.io.IOException; +import java.util.concurrent.atomic.AtomicInteger; import org.junit.Test; public class RetryInitializerTest { @@ -99,7 +99,7 @@ public boolean handleResponse(HttpRequest request, HttpResponse response, boolea .setSleeper(sleeper) .build()); - CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( new MockLowLevelHttpResponse().setStatusCode(401).setZeroContent()); HttpRequest request = createRequest(failingRequest); initializer.initialize(request); @@ -118,6 +118,70 @@ public boolean handleResponse(HttpRequest request, HttpResponse response, boolea assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); } + @Test + public void testRetryOnIOException() throws IOException { + MockSleeper sleeper = new MockSleeper(); + HttpCredentialsAdapter credentials = new HttpCredentialsAdapter(new MockGoogleCredentials()); + RetryInitializer initializer = new RetryInitializer(credentials, RetryConfig.builder() + .setMaxRetries(4) + .setSleeper(sleeper) + .build()); + + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromException( + new IOException("test error")); + HttpRequest request = createRequest(failingRequest); + initializer.initialize(request); + final HttpUnsuccessfulResponseHandler retryHandler = request.getUnsuccessfulResponseHandler(); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (IOException e) { + assertEquals("test error", e.getMessage()); + } + + assertEquals(4, sleeper.getCount()); + assertEquals(5, failingRequest.getCount()); + assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); + } + + @Test + public void testMaxRetriesCountIsCumulative() throws IOException { + MockSleeper sleeper = new MockSleeper(); + HttpCredentialsAdapter credentials = new HttpCredentialsAdapter(new MockGoogleCredentials()); + RetryInitializer initializer = new RetryInitializer(credentials, RetryConfig.builder() + .setMaxRetries(4) + .setRetryStatusCodes(ImmutableList.of(503)) + .setSleeper(sleeper) + .build()); + + final AtomicInteger counter = new AtomicInteger(0); + MockLowLevelHttpRequest failingRequest = new MockLowLevelHttpRequest(){ + @Override + public LowLevelHttpResponse execute() throws IOException { + if (counter.getAndIncrement() < 2) { + throw new IOException("test error"); + } else { + return new MockLowLevelHttpResponse().setStatusCode(503).setZeroContent(); + } + } + }; + HttpRequest request = createRequest(failingRequest); + initializer.initialize(request); + final HttpUnsuccessfulResponseHandler retryHandler = request.getUnsuccessfulResponseHandler(); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (HttpResponseException e) { + assertEquals(503, e.getStatusCode()); + } + + assertEquals(4, sleeper.getCount()); + assertEquals(5, counter.get()); + assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); + } + @Test public void testDelegateCalledAfterCredentials() throws IOException { MockSleeper sleeper = new MockSleeper(); @@ -139,7 +203,7 @@ public boolean handleResponse(HttpRequest request, HttpResponse response, boolea .setSleeper(sleeper) .build()); - CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( new MockLowLevelHttpResponse().setStatusCode(401).setZeroContent()); HttpRequest request = createRequest(failingRequest); initializer.initialize(request); @@ -158,6 +222,9 @@ public boolean handleResponse(HttpRequest request, HttpResponse response, boolea assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); } + private HttpRequest createRequest() throws IOException { + return createRequest(new MockLowLevelHttpRequest()); + } private HttpRequest createRequest(MockLowLevelHttpRequest request) throws IOException { HttpTransport transport = new MockHttpTransport.Builder() @@ -166,44 +233,4 @@ private HttpRequest createRequest(MockLowLevelHttpRequest request) throws IOExce HttpRequestFactory requestFactory = transport.createRequestFactory(); return requestFactory.buildPostRequest(TEST_URL, new EmptyContent()); } - - private static class CountingHttpRequest extends MockLowLevelHttpRequest { - - private final LowLevelHttpResponse response; - private final IOException exception; - private int count; - - private CountingHttpRequest(LowLevelHttpResponse response, IOException exception) { - this.response = response; - this.exception = exception; - } - - static CountingHttpRequest fromResponse(LowLevelHttpResponse response) { - return new CountingHttpRequest(checkNotNull(response), null); - } - - @Override - public void addHeader(String name, String value) { } - - @Override - public LowLevelHttpResponse execute() throws IOException { - count++; - if (response != null) { - return response; - } - throw exception; - } - - int getCount() { - return count; - } - } - - private HttpRequest createRequest() throws IOException { - HttpTransport transport = new MockHttpTransport.Builder() - .setLowLevelHttpRequest(new MockLowLevelHttpRequest()) - .build(); - HttpRequestFactory requestFactory = transport.createRequestFactory(); - return requestFactory.buildPostRequest(TEST_URL, new EmptyContent()); - } } diff --git a/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java b/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java index 8eeb1ed9b..a98b9343b 100644 --- a/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java @@ -16,7 +16,6 @@ package com.google.firebase.internal; -import static com.google.common.base.Preconditions.checkNotNull; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; @@ -27,7 +26,6 @@ import com.google.api.client.http.HttpRequestFactory; import com.google.api.client.http.HttpResponseException; import com.google.api.client.http.HttpTransport; -import com.google.api.client.http.LowLevelHttpResponse; import com.google.api.client.testing.http.FixedClock; import com.google.api.client.testing.http.MockHttpTransport; import com.google.api.client.testing.http.MockLowLevelHttpRequest; @@ -49,21 +47,21 @@ public class RetryUnsuccessfulResponseHandlerTest { private static final GenericUrl TEST_URL = new GenericUrl("https://firebase.google.com"); private static final RetryConfig.Builder TEST_RETRY_CONFIG = RetryConfig.builder() - .setRetryStatusCodes(ImmutableList.of(429, 503)); + .setRetryStatusCodes(ImmutableList.of(429, 503)) + .setMaxIntervalMillis(120 * 1000); @Test public void testDoesNotRetryOnUnspecifiedHttpStatus() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( new MockLowLevelHttpResponse() - .addHeader("retry-after", "121") .setStatusCode(404) .setZeroContent()); HttpRequest request = createRequest(failingRequest); - request.setNumberOfRetries(4); request.setUnsuccessfulResponseHandler(handler); + request.setNumberOfRetries(4); try { request.execute(); @@ -81,11 +79,13 @@ public void testRetryOnHttpClientErrorWhenSpecified() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( - new MockLowLevelHttpResponse().setStatusCode(429).setZeroContent()); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( + new MockLowLevelHttpResponse() + .setStatusCode(429) + .setZeroContent()); HttpRequest request = createRequest(failingRequest); - request.setNumberOfRetries(4); request.setUnsuccessfulResponseHandler(handler); + request.setNumberOfRetries(4); try { request.execute(); @@ -105,11 +105,13 @@ public void testRetryAfterIsAbsent() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( - new MockLowLevelHttpResponse().setStatusCode(503).setZeroContent()); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( + new MockLowLevelHttpResponse() + .setStatusCode(503) + .setZeroContent()); HttpRequest request = createRequest(failingRequest); - request.setNumberOfRetries(4); request.setUnsuccessfulResponseHandler(handler); + request.setNumberOfRetries(4); try { request.execute(); @@ -123,19 +125,46 @@ public void testRetryAfterIsAbsent() throws IOException { assertEquals(5, failingRequest.getCount()); } + @Test + public void testExponentialBackOffDoesNotExceedMaxInterval() throws IOException { + MultipleCallSleeper sleeper = new MultipleCallSleeper(); + RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( + testRetryConfig(sleeper)); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( + new MockLowLevelHttpResponse() + .setStatusCode(503) + .setZeroContent()); + HttpRequest request = createRequest(failingRequest); + request.setUnsuccessfulResponseHandler(handler); + request.setNumberOfRetries(10); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (HttpResponseException e) { + assertEquals(503, e.getStatusCode()); + } + + assertEquals(10, sleeper.getCount()); + assertArrayEquals( + new long[]{500, 1000, 2000, 4000, 8000, 16000, 32000, 64000, 120000, 120000}, + sleeper.getDelays()); + assertEquals(11, failingRequest.getCount()); + } + @Test public void testRetryAfterGivenAsSeconds() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( new MockLowLevelHttpResponse() .addHeader("retry-after", "2") .setStatusCode(503) .setZeroContent()); HttpRequest request = createRequest(failingRequest); - request.setNumberOfRetries(4); request.setUnsuccessfulResponseHandler(handler); + request.setNumberOfRetries(4); try { request.execute(); @@ -160,14 +189,14 @@ public void testRetryAfterGivenAsDate() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper), clock); - CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( new MockLowLevelHttpResponse() .addHeader("retry-after", retryAfter) .setStatusCode(503) .setZeroContent()); HttpRequest request = createRequest(failingRequest); - request.setNumberOfRetries(4); request.setUnsuccessfulResponseHandler(handler); + request.setNumberOfRetries(4); try { request.execute(); @@ -182,18 +211,18 @@ public void testRetryAfterGivenAsDate() throws IOException { } @Test - public void testInvalidRetryAfterFailsOverToExpBackoff() throws IOException { + public void testInvalidRetryAfterFailsOverToExpBackOff() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( new MockLowLevelHttpResponse() .addHeader("retry-after", "not valid") .setStatusCode(503) .setZeroContent()); HttpRequest request = createRequest(failingRequest); - request.setNumberOfRetries(4); request.setUnsuccessfulResponseHandler(handler); + request.setNumberOfRetries(4); try { request.execute(); @@ -212,14 +241,14 @@ public void testDoesNotRetryWhenRetryAfterIsTooLong() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( new MockLowLevelHttpResponse() .addHeader("retry-after", "121") .setStatusCode(503) .setZeroContent()); HttpRequest request = createRequest(failingRequest); - request.setNumberOfRetries(4); request.setUnsuccessfulResponseHandler(handler); + request.setNumberOfRetries(4); try { request.execute(); @@ -243,13 +272,13 @@ public void sleep(long millis) throws InterruptedException { }; RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingHttpRequest failingRequest = CountingHttpRequest.fromResponse( + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( new MockLowLevelHttpResponse() .setStatusCode(503) .setZeroContent()); HttpRequest request = createRequest(failingRequest); - request.setNumberOfRetries(4); request.setUnsuccessfulResponseHandler(handler); + request.setNumberOfRetries(4); try { request.execute(); @@ -274,37 +303,6 @@ private RetryConfig testRetryConfig(Sleeper sleeper) { return TEST_RETRY_CONFIG.setSleeper(sleeper).build(); } - private static class CountingHttpRequest extends MockLowLevelHttpRequest { - - private final LowLevelHttpResponse response; - private final IOException exception; - private int count; - - private CountingHttpRequest(LowLevelHttpResponse response, IOException exception) { - this.response = response; - this.exception = exception; - } - - static CountingHttpRequest fromResponse(LowLevelHttpResponse response) { - return new CountingHttpRequest(checkNotNull(response), null); - } - - @Override - public void addHeader(String name, String value) { } - - @Override - public LowLevelHttpResponse execute() throws IOException { - count++; - if (response != null) { - return response; - } - throw exception; - } - - int getCount() { - return count; - } - } private static class MultipleCallSleeper extends MockSleeper { From 0a3264b1ee1f5bc9e99ec0f490dea1ba2fda9bb5 Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Sun, 24 Feb 2019 13:45:12 -0800 Subject: [PATCH 10/19] Further cleaned up the impl and tests --- .../internal/FirebaseRequestInitializer.java | 23 ++++----- .../firebase/internal/RetryInitializer.java | 15 +++--- .../RetryUnsuccessfulResponseHandler.java | 4 +- .../internal/CountingLowLevelHttpRequest.java | 19 +++++++- .../firebase/internal/RetryConfigTest.java | 1 + .../RetryUnsuccessfulResponseHandlerTest.java | 47 ++++--------------- 6 files changed, 52 insertions(+), 57 deletions(-) diff --git a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java index 42fce053d..cc415538c 100644 --- a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java +++ b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java @@ -19,11 +19,12 @@ import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpRequestInitializer; import com.google.auth.http.HttpCredentialsAdapter; -import com.google.auth.oauth2.GoogleCredentials; +import com.google.common.collect.ImmutableList; import com.google.firebase.FirebaseApp; import com.google.firebase.FirebaseOptions; import com.google.firebase.ImplFirebaseTrampolines; import java.io.IOException; +import java.util.List; /** * {@code HttpRequestInitializer} for configuring outgoing REST calls. Initializes requests with @@ -31,26 +32,26 @@ */ public final class FirebaseRequestInitializer implements HttpRequestInitializer { - private final HttpCredentialsAdapter credentialsAdapter; - private final TimeoutInitializer timeoutInitializer; - private final RetryInitializer retryInitializer; + private final List initializers; public FirebaseRequestInitializer(FirebaseApp app) { this(app, null); } public FirebaseRequestInitializer(FirebaseApp app, @Nullable RetryConfig retryConfig) { - GoogleCredentials credentials = ImplFirebaseTrampolines.getCredentials(app); - this.credentialsAdapter = new HttpCredentialsAdapter(credentials); - this.timeoutInitializer = new TimeoutInitializer(app.getOptions()); - this.retryInitializer = new RetryInitializer(this.credentialsAdapter, retryConfig); + HttpCredentialsAdapter credentials = new HttpCredentialsAdapter( + ImplFirebaseTrampolines.getCredentials(app)); + this.initializers = ImmutableList.of( + credentials, + new TimeoutInitializer(app.getOptions()), + new RetryInitializer(credentials, retryConfig)); } @Override public void initialize(HttpRequest request) throws IOException { - credentialsAdapter.initialize(request); - timeoutInitializer.initialize(request); - retryInitializer.initialize(request); + for (HttpRequestInitializer initializer : initializers) { + initializer.initialize(request); + } } private static class TimeoutInitializer implements HttpRequestInitializer { diff --git a/src/main/java/com/google/firebase/internal/RetryInitializer.java b/src/main/java/com/google/firebase/internal/RetryInitializer.java index a00b8cb14..5ccbb6bd6 100644 --- a/src/main/java/com/google/firebase/internal/RetryInitializer.java +++ b/src/main/java/com/google/firebase/internal/RetryInitializer.java @@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.api.client.http.HttpBackOffIOExceptionHandler; +import com.google.api.client.http.HttpIOExceptionHandler; import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpRequestInitializer; import com.google.api.client.http.HttpResponse; @@ -37,7 +38,7 @@ final class RetryInitializer implements HttpRequestInitializer { private final HttpCredentialsAdapter credentials; private final RetryConfig retryConfig; - RetryInitializer(HttpCredentialsAdapter credentials, RetryConfig retryConfig) { + RetryInitializer(HttpCredentialsAdapter credentials, @Nullable RetryConfig retryConfig) { this.credentials = checkNotNull(credentials); this.retryConfig = retryConfig; } @@ -46,11 +47,8 @@ final class RetryInitializer implements HttpRequestInitializer { public void initialize(HttpRequest request) { if (retryConfig != null) { request.setNumberOfRetries(retryConfig.getMaxRetries()); - request.setUnsuccessfulResponseHandler( - newUnsuccessfulResponseHandler()); - request.setIOExceptionHandler( - new HttpBackOffIOExceptionHandler(retryConfig.newBackOff()) - .setSleeper(retryConfig.getSleeper())); + request.setUnsuccessfulResponseHandler(newUnsuccessfulResponseHandler()); + request.setIOExceptionHandler(newIOExceptionHandler()); } else { request.setNumberOfRetries(0); } @@ -77,4 +75,9 @@ public boolean handleResponse( } }; } + + private HttpIOExceptionHandler newIOExceptionHandler() { + return new HttpBackOffIOExceptionHandler(retryConfig.newBackOff()) + .setSleeper(retryConfig.getSleeper()); + } } diff --git a/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java b/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java index 0bdde353b..0d70f90cb 100644 --- a/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java +++ b/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java @@ -77,7 +77,7 @@ public boolean handleResponse( private boolean waitAndRetry(HttpResponse response) throws IOException, InterruptedException { String retryAfterHeader = response.getHeaders().getRetryAfter(); if (!Strings.isNullOrEmpty(retryAfterHeader)) { - long intervalMillis = parseRetryAfterHeaderAsMillis(retryAfterHeader.trim()); + long intervalMillis = parseRetryAfterHeaderIntoMillis(retryAfterHeader.trim()); if (intervalMillis > retryConfig.getMaxIntervalMillis()) { return false; } @@ -91,7 +91,7 @@ private boolean waitAndRetry(HttpResponse response) throws IOException, Interrup return BackOffUtils.next(sleeper, backOff); } - private long parseRetryAfterHeaderAsMillis(String retryAfter) { + private long parseRetryAfterHeaderIntoMillis(String retryAfter) { try { return Long.parseLong(retryAfter) * 1000; } catch (NumberFormatException e) { diff --git a/src/test/java/com/google/firebase/internal/CountingLowLevelHttpRequest.java b/src/test/java/com/google/firebase/internal/CountingLowLevelHttpRequest.java index 768660efb..e7e08cca9 100644 --- a/src/test/java/com/google/firebase/internal/CountingLowLevelHttpRequest.java +++ b/src/test/java/com/google/firebase/internal/CountingLowLevelHttpRequest.java @@ -20,7 +20,9 @@ import com.google.api.client.http.LowLevelHttpResponse; import com.google.api.client.testing.http.MockLowLevelHttpRequest; +import com.google.api.client.testing.http.MockLowLevelHttpResponse; import java.io.IOException; +import java.util.Map; class CountingLowLevelHttpRequest extends MockLowLevelHttpRequest { @@ -33,6 +35,22 @@ private CountingLowLevelHttpRequest(LowLevelHttpResponse response, IOException e this.exception = exception; } + static CountingLowLevelHttpRequest fromResponse(int status) { + return fromResponse(status, null); + } + + static CountingLowLevelHttpRequest fromResponse(int status, Map headers) { + MockLowLevelHttpResponse response = new MockLowLevelHttpResponse() + .setStatusCode(status) + .setZeroContent(); + if (headers != null) { + for (Map.Entry entry : headers.entrySet()) { + response.addHeader(entry.getKey(), entry.getValue()); + } + } + return fromResponse(response); + } + static CountingLowLevelHttpRequest fromResponse(LowLevelHttpResponse response) { return new CountingLowLevelHttpRequest(checkNotNull(response), null); } @@ -53,5 +71,4 @@ public LowLevelHttpResponse execute() throws IOException { int getCount() { return count; } - } diff --git a/src/test/java/com/google/firebase/internal/RetryConfigTest.java b/src/test/java/com/google/firebase/internal/RetryConfigTest.java index 88be103ea..36e250697 100644 --- a/src/test/java/com/google/firebase/internal/RetryConfigTest.java +++ b/src/test/java/com/google/firebase/internal/RetryConfigTest.java @@ -91,6 +91,7 @@ public void testExponentialBackOff() throws IOException { assertEquals(4000, backOff.nextBackOffMillis()); assertEquals(8000, backOff.nextBackOffMillis()); assertEquals(12000, backOff.nextBackOffMillis()); + assertEquals(12000, backOff.nextBackOffMillis()); } @Test(expected = IllegalArgumentException.class) diff --git a/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java b/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java index a98b9343b..bcb4a97e2 100644 --- a/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java @@ -29,11 +29,11 @@ import com.google.api.client.testing.http.FixedClock; import com.google.api.client.testing.http.MockHttpTransport; import com.google.api.client.testing.http.MockLowLevelHttpRequest; -import com.google.api.client.testing.http.MockLowLevelHttpResponse; import com.google.api.client.testing.util.MockSleeper; import com.google.api.client.util.Clock; import com.google.api.client.util.Sleeper; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.primitives.Longs; import java.io.IOException; import java.text.SimpleDateFormat; @@ -55,10 +55,7 @@ public void testDoesNotRetryOnUnspecifiedHttpStatus() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( - new MockLowLevelHttpResponse() - .setStatusCode(404) - .setZeroContent()); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(404); HttpRequest request = createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(4); @@ -79,10 +76,7 @@ public void testRetryOnHttpClientErrorWhenSpecified() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( - new MockLowLevelHttpResponse() - .setStatusCode(429) - .setZeroContent()); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(429); HttpRequest request = createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(4); @@ -105,10 +99,7 @@ public void testRetryAfterIsAbsent() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( - new MockLowLevelHttpResponse() - .setStatusCode(503) - .setZeroContent()); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(503); HttpRequest request = createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(4); @@ -130,10 +121,7 @@ public void testExponentialBackOffDoesNotExceedMaxInterval() throws IOException MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( - new MockLowLevelHttpResponse() - .setStatusCode(503) - .setZeroContent()); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(503); HttpRequest request = createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(10); @@ -158,10 +146,7 @@ public void testRetryAfterGivenAsSeconds() throws IOException { RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( - new MockLowLevelHttpResponse() - .addHeader("retry-after", "2") - .setStatusCode(503) - .setZeroContent()); + 503, ImmutableMap.of("retry-after", "2")); HttpRequest request = createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(4); @@ -190,10 +175,7 @@ public void testRetryAfterGivenAsDate() throws IOException { RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper), clock); CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( - new MockLowLevelHttpResponse() - .addHeader("retry-after", retryAfter) - .setStatusCode(503) - .setZeroContent()); + 503, ImmutableMap.of("retry-after", retryAfter)); HttpRequest request = createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(4); @@ -216,10 +198,7 @@ public void testInvalidRetryAfterFailsOverToExpBackOff() throws IOException { RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( - new MockLowLevelHttpResponse() - .addHeader("retry-after", "not valid") - .setStatusCode(503) - .setZeroContent()); + 503, ImmutableMap.of("retry-after", "not valid")); HttpRequest request = createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(4); @@ -242,10 +221,7 @@ public void testDoesNotRetryWhenRetryAfterIsTooLong() throws IOException { RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( - new MockLowLevelHttpResponse() - .addHeader("retry-after", "121") - .setStatusCode(503) - .setZeroContent()); + 503, ImmutableMap.of("retry-after", "121")); HttpRequest request = createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(4); @@ -272,10 +248,7 @@ public void sleep(long millis) throws InterruptedException { }; RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( - new MockLowLevelHttpResponse() - .setStatusCode(503) - .setZeroContent()); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(503); HttpRequest request = createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(4); From f3e2f7c95e1d6e3946be703bd350618b2e89196d Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Sun, 24 Feb 2019 15:04:21 -0800 Subject: [PATCH 11/19] Decoupled retry initializer from credentials --- .../internal/FirebaseRequestInitializer.java | 6 +- .../firebase/internal/RetryInitializer.java | 86 +++++++++---- .../FirebaseRequestInitializerTest.java | 43 ++++--- .../internal/RetryInitializerTest.java | 118 ++++++------------ .../RetryUnsuccessfulResponseHandlerTest.java | 34 ++--- .../google/firebase/testing/TestUtils.java | 21 +++- 6 files changed, 156 insertions(+), 152 deletions(-) diff --git a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java index cc415538c..14938991c 100644 --- a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java +++ b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java @@ -39,12 +39,10 @@ public FirebaseRequestInitializer(FirebaseApp app) { } public FirebaseRequestInitializer(FirebaseApp app, @Nullable RetryConfig retryConfig) { - HttpCredentialsAdapter credentials = new HttpCredentialsAdapter( - ImplFirebaseTrampolines.getCredentials(app)); this.initializers = ImmutableList.of( - credentials, + new HttpCredentialsAdapter(ImplFirebaseTrampolines.getCredentials(app)), new TimeoutInitializer(app.getOptions()), - new RetryInitializer(credentials, retryConfig)); + new RetryInitializer(retryConfig)); } @Override diff --git a/src/main/java/com/google/firebase/internal/RetryInitializer.java b/src/main/java/com/google/firebase/internal/RetryInitializer.java index 5ccbb6bd6..0e6cd5b95 100644 --- a/src/main/java/com/google/firebase/internal/RetryInitializer.java +++ b/src/main/java/com/google/firebase/internal/RetryInitializer.java @@ -24,22 +24,20 @@ import com.google.api.client.http.HttpRequestInitializer; import com.google.api.client.http.HttpResponse; import com.google.api.client.http.HttpUnsuccessfulResponseHandler; -import com.google.auth.http.HttpCredentialsAdapter; import java.io.IOException; /** * Configures HTTP requests to be retried. Failures caused by I/O errors are always retried * according to the specified {@link RetryConfig}. Failures caused by unsuccessful HTTP responses - * are first referred to the {@code HttpCredentialsAdapter}. If the request does not get retried - * by the credentials, {@link RetryConfig} is used to schedule additional retries. + * are first referred to the {@code HttpUnsuccessfulResponseHandler} already set on the request. If + * the request does not get retried at that level, {@link RetryUnsuccessfulResponseHandler} is used + * to schedule additional retries. */ final class RetryInitializer implements HttpRequestInitializer { - private final HttpCredentialsAdapter credentials; private final RetryConfig retryConfig; - RetryInitializer(HttpCredentialsAdapter credentials, @Nullable RetryConfig retryConfig) { - this.credentials = checkNotNull(credentials); + RetryInitializer(@Nullable RetryConfig retryConfig) { this.retryConfig = retryConfig; } @@ -47,37 +45,71 @@ final class RetryInitializer implements HttpRequestInitializer { public void initialize(HttpRequest request) { if (retryConfig != null) { request.setNumberOfRetries(retryConfig.getMaxRetries()); - request.setUnsuccessfulResponseHandler(newUnsuccessfulResponseHandler()); + request.setUnsuccessfulResponseHandler(newUnsuccessfulResponseHandler(request)); request.setIOExceptionHandler(newIOExceptionHandler()); } else { request.setNumberOfRetries(0); } } - private HttpUnsuccessfulResponseHandler newUnsuccessfulResponseHandler() { - final HttpUnsuccessfulResponseHandler retryHandler = - new RetryUnsuccessfulResponseHandler(retryConfig); - return new HttpUnsuccessfulResponseHandler() { - @Override - public boolean handleResponse( - HttpRequest request, - HttpResponse response, - boolean supportsRetry) throws IOException { - boolean retry = credentials.handleResponse(request, response, supportsRetry); - if (!retry) { - retry = retryHandler.handleResponse(request, response, supportsRetry); - } - - // HttpCredentialsAdapter sometimes resets the unsuccessful response handler on the - // request. This changes it back. - request.setUnsuccessfulResponseHandler(this); - return retry; - } - }; + private HttpUnsuccessfulResponseHandler newUnsuccessfulResponseHandler(HttpRequest request) { + RetryUnsuccessfulResponseHandler retryHandler = new RetryUnsuccessfulResponseHandler( + retryConfig); + return RetryHandlerDecorator.decorate(retryHandler, request); } private HttpIOExceptionHandler newIOExceptionHandler() { return new HttpBackOffIOExceptionHandler(retryConfig.newBackOff()) .setSleeper(retryConfig.getSleeper()); } + + /** + * Makes sure that any error handlers already set on the request are executed before the retry + * handler is called. This is needed since some initializers (e.g. HttpCredentialsAdapter) + * register their own error handlers. + */ + private static class RetryHandlerDecorator implements HttpUnsuccessfulResponseHandler { + + private final HttpUnsuccessfulResponseHandler preRetryHandler; + private final RetryUnsuccessfulResponseHandler retryHandler; + + private RetryHandlerDecorator( + HttpUnsuccessfulResponseHandler preRetryHandler, + RetryUnsuccessfulResponseHandler retryHandler) { + this.preRetryHandler = checkNotNull(preRetryHandler); + this.retryHandler = checkNotNull(retryHandler); + } + + static RetryHandlerDecorator decorate( + RetryUnsuccessfulResponseHandler retryHandler, HttpRequest request) { + + HttpUnsuccessfulResponseHandler preRetryHandler = request.getUnsuccessfulResponseHandler(); + if (preRetryHandler == null) { + preRetryHandler = new HttpUnsuccessfulResponseHandler() { + @Override + public boolean handleResponse( + HttpRequest request, HttpResponse response, boolean supportsRetry) { + return false; + } + }; + } + return new RetryHandlerDecorator(preRetryHandler, retryHandler); + } + + @Override + public boolean handleResponse( + HttpRequest request, + HttpResponse response, + boolean supportsRetry) throws IOException { + boolean retry = preRetryHandler.handleResponse(request, response, supportsRetry); + if (!retry) { + retry = retryHandler.handleResponse(request, response, supportsRetry); + } + + // Pre-retry handler may have reset the unsuccessful response handler on the + // request. This changes it back. + request.setUnsuccessfulResponseHandler(this); + return retry; + } + } } diff --git a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java index f95c8e196..675a29d84 100644 --- a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java @@ -21,27 +21,20 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; -import com.google.api.client.http.EmptyContent; -import com.google.api.client.http.GenericUrl; import com.google.api.client.http.HttpBackOffIOExceptionHandler; import com.google.api.client.http.HttpRequest; -import com.google.api.client.http.HttpRequestFactory; -import com.google.api.client.http.HttpTransport; -import com.google.api.client.testing.http.MockHttpTransport; -import com.google.api.client.testing.http.MockLowLevelHttpRequest; +import com.google.api.client.http.HttpResponseException; import com.google.auth.http.HttpCredentialsAdapter; import com.google.firebase.FirebaseApp; import com.google.firebase.FirebaseOptions; import com.google.firebase.TestOnlyImplFirebaseTrampolines; import com.google.firebase.auth.MockGoogleCredentials; -import java.io.IOException; +import com.google.firebase.testing.TestUtils; import org.junit.After; import org.junit.Test; public class FirebaseRequestInitializerTest { - private static final GenericUrl TEST_URL = new GenericUrl("https://firebase.google.com"); - @After public void tearDown() { TestOnlyImplFirebaseTrampolines.clearInstancesForTest(); @@ -52,7 +45,7 @@ public void testDefaultSettings() throws Exception { FirebaseApp app = FirebaseApp.initializeApp(new FirebaseOptions.Builder() .setCredentials(new MockGoogleCredentials("token")) .build()); - HttpRequest request = createRequest(); + HttpRequest request = TestUtils.createRequest(); FirebaseRequestInitializer initializer = new FirebaseRequestInitializer(app); initializer.initialize(request); @@ -73,7 +66,7 @@ public void testExplicitTimeouts() throws Exception { .setConnectTimeout(30000) .setReadTimeout(60000) .build()); - HttpRequest request = createRequest(); + HttpRequest request = TestUtils.createRequest(); FirebaseRequestInitializer initializer = new FirebaseRequestInitializer(app); initializer.initialize(request); @@ -94,7 +87,7 @@ public void testExplicitRetryConfig() throws Exception { RetryConfig retryConfig = RetryConfig.builder() .setMaxRetries(5) .build(); - HttpRequest request = createRequest(); + HttpRequest request = TestUtils.createRequest(); FirebaseRequestInitializer initializer = new FirebaseRequestInitializer(app, retryConfig); initializer.initialize(request); @@ -107,11 +100,27 @@ public void testExplicitRetryConfig() throws Exception { assertNotNull(request.getUnsuccessfulResponseHandler()); } - private HttpRequest createRequest() throws IOException { - HttpTransport transport = new MockHttpTransport.Builder() - .setLowLevelHttpRequest(new MockLowLevelHttpRequest()) + @Test + public void testCredentialsRetryHandler() throws Exception { + FirebaseApp app = FirebaseApp.initializeApp(new FirebaseOptions.Builder() + .setCredentials(new MockGoogleCredentials("token")) + .build()); + RetryConfig retryConfig = RetryConfig.builder() + .setMaxRetries(5) .build(); - HttpRequestFactory requestFactory = transport.createRequestFactory(); - return requestFactory.buildPostRequest(TEST_URL, new EmptyContent()); + CountingLowLevelHttpRequest countingRequest = CountingLowLevelHttpRequest.fromResponse(401); + HttpRequest request = TestUtils.createRequest(countingRequest); + FirebaseRequestInitializer initializer = new FirebaseRequestInitializer(app, retryConfig); + initializer.initialize(request); + request.getHeaders().setAuthorization((String) null); + + try { + request.execute(); + } catch (HttpResponseException e) { + assertEquals(401, e.getStatusCode()); + } + + assertEquals("Bearer token", request.getHeaders().getAuthorization()); + assertEquals(6, countingRequest.getCount()); } } diff --git a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java index 897c2795e..b872f64b4 100644 --- a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java @@ -23,32 +23,25 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; -import com.google.api.client.http.EmptyContent; -import com.google.api.client.http.GenericUrl; import com.google.api.client.http.HttpBackOffIOExceptionHandler; import com.google.api.client.http.HttpRequest; -import com.google.api.client.http.HttpRequestFactory; import com.google.api.client.http.HttpResponse; import com.google.api.client.http.HttpResponseException; -import com.google.api.client.http.HttpTransport; import com.google.api.client.http.HttpUnsuccessfulResponseHandler; import com.google.api.client.http.LowLevelHttpResponse; -import com.google.api.client.testing.http.MockHttpTransport; import com.google.api.client.testing.http.MockLowLevelHttpRequest; import com.google.api.client.testing.http.MockLowLevelHttpResponse; import com.google.api.client.testing.util.MockSleeper; import com.google.auth.http.HttpCredentialsAdapter; import com.google.common.collect.ImmutableList; import com.google.firebase.auth.MockGoogleCredentials; +import com.google.firebase.testing.TestUtils; import java.io.IOException; import java.util.concurrent.atomic.AtomicInteger; import org.junit.Test; public class RetryInitializerTest { - private static final GenericUrl TEST_URL = new GenericUrl("https://firebase.google.com"); - private static final HttpCredentialsAdapter TEST_CREDENTIALS = new HttpCredentialsAdapter( - new MockGoogleCredentials()); private static final RetryConfig RETRY_CONFIG = RetryConfig.builder() .setMaxRetries(5) .setRetryStatusCodes(ImmutableList.of(503)) @@ -56,8 +49,8 @@ public class RetryInitializerTest { @Test public void testEnableRetry() throws IOException { - RetryInitializer initializer = new RetryInitializer(TEST_CREDENTIALS, RETRY_CONFIG); - HttpRequest request = createRequest(); + RetryInitializer initializer = new RetryInitializer(RETRY_CONFIG); + HttpRequest request = TestUtils.createRequest(); initializer.initialize(request); @@ -68,8 +61,8 @@ public void testEnableRetry() throws IOException { @Test public void testDisableRetry() throws IOException { - RetryInitializer initializer = new RetryInitializer(TEST_CREDENTIALS, null); - HttpRequest request = createRequest(); + RetryInitializer initializer = new RetryInitializer(null); + HttpRequest request = TestUtils.createRequest(); initializer.initialize(request); @@ -79,65 +72,49 @@ public void testDisableRetry() throws IOException { } @Test - public void testRetryCredentialsCheck() throws IOException { + public void testRetryOnIOException() throws IOException { MockSleeper sleeper = new MockSleeper(); - HttpCredentialsAdapter credentials = new HttpCredentialsAdapter(new MockGoogleCredentials()){ - @Override - public boolean handleResponse(HttpRequest request, HttpResponse response, boolean - supportsRetry) { - String auth = request.getHeaders().getAuthorization(); - if (!"Bearer retry".equals(auth)) { - request.getHeaders().setAuthorization("Bearer retry"); - return true; - } - return false; - } - }; - RetryInitializer initializer = new RetryInitializer(credentials, RetryConfig.builder() + RetryInitializer initializer = new RetryInitializer(RetryConfig.builder() .setMaxRetries(4) - .setRetryStatusCodes(ImmutableList.of(503)) .setSleeper(sleeper) .build()); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( - new MockLowLevelHttpResponse().setStatusCode(401).setZeroContent()); - HttpRequest request = createRequest(failingRequest); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromException( + new IOException("test error")); + HttpRequest request = TestUtils.createRequest(failingRequest); initializer.initialize(request); final HttpUnsuccessfulResponseHandler retryHandler = request.getUnsuccessfulResponseHandler(); try { request.execute(); fail("No exception thrown for HTTP error"); - } catch (HttpResponseException e) { - assertEquals(401, e.getStatusCode()); + } catch (IOException e) { + assertEquals("test error", e.getMessage()); } - assertEquals("Bearer retry", request.getHeaders().getAuthorization()); - assertEquals(0, sleeper.getCount()); - assertEquals(2, failingRequest.getCount()); + assertEquals(4, sleeper.getCount()); + assertEquals(5, failingRequest.getCount()); assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); } @Test - public void testRetryOnIOException() throws IOException { + public void testRetryOnHttpError() throws IOException { MockSleeper sleeper = new MockSleeper(); - HttpCredentialsAdapter credentials = new HttpCredentialsAdapter(new MockGoogleCredentials()); - RetryInitializer initializer = new RetryInitializer(credentials, RetryConfig.builder() + RetryInitializer initializer = new RetryInitializer(RetryConfig.builder() .setMaxRetries(4) + .setRetryStatusCodes(ImmutableList.of(503)) .setSleeper(sleeper) .build()); - - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromException( - new IOException("test error")); - HttpRequest request = createRequest(failingRequest); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(503); + HttpRequest request = TestUtils.createRequest(failingRequest); initializer.initialize(request); final HttpUnsuccessfulResponseHandler retryHandler = request.getUnsuccessfulResponseHandler(); try { request.execute(); fail("No exception thrown for HTTP error"); - } catch (IOException e) { - assertEquals("test error", e.getMessage()); + } catch (HttpResponseException e) { + assertEquals(503, e.getStatusCode()); } assertEquals(4, sleeper.getCount()); @@ -148,8 +125,7 @@ public void testRetryOnIOException() throws IOException { @Test public void testMaxRetriesCountIsCumulative() throws IOException { MockSleeper sleeper = new MockSleeper(); - HttpCredentialsAdapter credentials = new HttpCredentialsAdapter(new MockGoogleCredentials()); - RetryInitializer initializer = new RetryInitializer(credentials, RetryConfig.builder() + RetryInitializer initializer = new RetryInitializer(RetryConfig.builder() .setMaxRetries(4) .setRetryStatusCodes(ImmutableList.of(503)) .setSleeper(sleeper) @@ -166,7 +142,7 @@ public LowLevelHttpResponse execute() throws IOException { } } }; - HttpRequest request = createRequest(failingRequest); + HttpRequest request = TestUtils.createRequest(failingRequest); initializer.initialize(request); final HttpUnsuccessfulResponseHandler retryHandler = request.getUnsuccessfulResponseHandler(); @@ -183,29 +159,25 @@ public LowLevelHttpResponse execute() throws IOException { } @Test - public void testDelegateCalledAfterCredentials() throws IOException { - MockSleeper sleeper = new MockSleeper(); - HttpCredentialsAdapter credentials = new HttpCredentialsAdapter(new MockGoogleCredentials()){ + public void testOtherErrorHandlersCalledBeforeRetry() throws IOException { + final AtomicInteger otherErrorHandlerCalls = new AtomicInteger(0); + HttpCredentialsAdapter credentials = new HttpCredentialsAdapter(new MockGoogleCredentials()) { @Override - public boolean handleResponse(HttpRequest request, HttpResponse response, boolean - supportsRetry) { - String auth = request.getHeaders().getAuthorization(); - if (!"Bearer retry".equals(auth)) { - request.getHeaders().setAuthorization("Bearer retry"); - return true; - } - return false; + public boolean handleResponse( + HttpRequest request, HttpResponse response, boolean supportsRetry) { + otherErrorHandlerCalls.incrementAndGet(); + return super.handleResponse(request, response, supportsRetry); } }; - RetryInitializer initializer = new RetryInitializer(credentials, RetryConfig.builder() + MockSleeper sleeper = new MockSleeper(); + RetryInitializer initializer = new RetryInitializer(RetryConfig.builder() .setMaxRetries(4) - .setRetryStatusCodes(ImmutableList.of(401)) + .setRetryStatusCodes(ImmutableList.of(503)) .setSleeper(sleeper) .build()); - - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( - new MockLowLevelHttpResponse().setStatusCode(401).setZeroContent()); - HttpRequest request = createRequest(failingRequest); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(503); + HttpRequest request = TestUtils.createRequest(failingRequest); + credentials.initialize(request); initializer.initialize(request); final HttpUnsuccessfulResponseHandler retryHandler = request.getUnsuccessfulResponseHandler(); @@ -213,24 +185,12 @@ public boolean handleResponse(HttpRequest request, HttpResponse response, boolea request.execute(); fail("No exception thrown for HTTP error"); } catch (HttpResponseException e) { - assertEquals(401, e.getStatusCode()); + assertEquals(503, e.getStatusCode()); } - assertEquals("Bearer retry", request.getHeaders().getAuthorization()); - assertEquals(3, sleeper.getCount()); + assertEquals(5, otherErrorHandlerCalls.get()); + assertEquals(4, sleeper.getCount()); assertEquals(5, failingRequest.getCount()); assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); } - - private HttpRequest createRequest() throws IOException { - return createRequest(new MockLowLevelHttpRequest()); - } - - private HttpRequest createRequest(MockLowLevelHttpRequest request) throws IOException { - HttpTransport transport = new MockHttpTransport.Builder() - .setLowLevelHttpRequest(request) - .build(); - HttpRequestFactory requestFactory = transport.createRequestFactory(); - return requestFactory.buildPostRequest(TEST_URL, new EmptyContent()); - } } diff --git a/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java b/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java index bcb4a97e2..157d591f4 100644 --- a/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java @@ -20,21 +20,16 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; -import com.google.api.client.http.EmptyContent; -import com.google.api.client.http.GenericUrl; import com.google.api.client.http.HttpRequest; -import com.google.api.client.http.HttpRequestFactory; import com.google.api.client.http.HttpResponseException; -import com.google.api.client.http.HttpTransport; import com.google.api.client.testing.http.FixedClock; -import com.google.api.client.testing.http.MockHttpTransport; -import com.google.api.client.testing.http.MockLowLevelHttpRequest; import com.google.api.client.testing.util.MockSleeper; import com.google.api.client.util.Clock; import com.google.api.client.util.Sleeper; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.primitives.Longs; +import com.google.firebase.testing.TestUtils; import java.io.IOException; import java.text.SimpleDateFormat; import java.util.ArrayList; @@ -45,7 +40,6 @@ public class RetryUnsuccessfulResponseHandlerTest { - private static final GenericUrl TEST_URL = new GenericUrl("https://firebase.google.com"); private static final RetryConfig.Builder TEST_RETRY_CONFIG = RetryConfig.builder() .setRetryStatusCodes(ImmutableList.of(429, 503)) .setMaxIntervalMillis(120 * 1000); @@ -56,7 +50,7 @@ public void testDoesNotRetryOnUnspecifiedHttpStatus() throws IOException { RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(404); - HttpRequest request = createRequest(failingRequest); + HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(4); @@ -77,7 +71,7 @@ public void testRetryOnHttpClientErrorWhenSpecified() throws IOException { RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(429); - HttpRequest request = createRequest(failingRequest); + HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(4); @@ -100,7 +94,7 @@ public void testRetryAfterIsAbsent() throws IOException { RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(503); - HttpRequest request = createRequest(failingRequest); + HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(4); @@ -122,7 +116,7 @@ public void testExponentialBackOffDoesNotExceedMaxInterval() throws IOException RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(503); - HttpRequest request = createRequest(failingRequest); + HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(10); @@ -147,7 +141,7 @@ public void testRetryAfterGivenAsSeconds() throws IOException { testRetryConfig(sleeper)); CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( 503, ImmutableMap.of("retry-after", "2")); - HttpRequest request = createRequest(failingRequest); + HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(4); @@ -176,7 +170,7 @@ public void testRetryAfterGivenAsDate() throws IOException { testRetryConfig(sleeper), clock); CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( 503, ImmutableMap.of("retry-after", retryAfter)); - HttpRequest request = createRequest(failingRequest); + HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(4); @@ -199,7 +193,7 @@ public void testInvalidRetryAfterFailsOverToExpBackOff() throws IOException { testRetryConfig(sleeper)); CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( 503, ImmutableMap.of("retry-after", "not valid")); - HttpRequest request = createRequest(failingRequest); + HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(4); @@ -222,7 +216,7 @@ public void testDoesNotRetryWhenRetryAfterIsTooLong() throws IOException { testRetryConfig(sleeper)); CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( 503, ImmutableMap.of("retry-after", "121")); - HttpRequest request = createRequest(failingRequest); + HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(4); @@ -249,7 +243,7 @@ public void sleep(long millis) throws InterruptedException { RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(503); - HttpRequest request = createRequest(failingRequest); + HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(4); @@ -264,14 +258,6 @@ public void sleep(long millis) throws InterruptedException { assertEquals(1, failingRequest.getCount()); } - private HttpRequest createRequest(MockLowLevelHttpRequest request) throws IOException { - HttpTransport transport = new MockHttpTransport.Builder() - .setLowLevelHttpRequest(request) - .build(); - HttpRequestFactory requestFactory = transport.createRequestFactory(); - return requestFactory.buildPostRequest(TEST_URL, new EmptyContent()); - } - private RetryConfig testRetryConfig(Sleeper sleeper) { return TEST_RETRY_CONFIG.setSleeper(sleeper).build(); } diff --git a/src/test/java/com/google/firebase/testing/TestUtils.java b/src/test/java/com/google/firebase/testing/TestUtils.java index 0dec4db3f..3e33a52ea 100644 --- a/src/test/java/com/google/firebase/testing/TestUtils.java +++ b/src/test/java/com/google/firebase/testing/TestUtils.java @@ -19,8 +19,14 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.api.client.googleapis.testing.auth.oauth2.MockTokenServerTransport; +import com.google.api.client.http.EmptyContent; +import com.google.api.client.http.GenericUrl; +import com.google.api.client.http.HttpRequest; +import com.google.api.client.http.HttpRequestFactory; import com.google.api.client.http.HttpTransport; import com.google.api.client.json.webtoken.JsonWebSignature; +import com.google.api.client.testing.http.MockHttpTransport; +import com.google.api.client.testing.http.MockLowLevelHttpRequest; import com.google.auth.http.HttpTransportFactory; import com.google.auth.oauth2.GoogleCredentials; import com.google.common.collect.ImmutableMap; @@ -41,7 +47,8 @@ public class TestUtils { public static final long TEST_TIMEOUT_MILLIS = 7 * 1000; - public static final String TEST_ADC_ACCESS_TOKEN = "test-adc-access-token"; + private static final String TEST_ADC_ACCESS_TOKEN = "test-adc-access-token"; + private static final GenericUrl TEST_URL = new GenericUrl("https://firebase.google.com"); private static GoogleCredentials defaultCredentials; @@ -123,4 +130,16 @@ public HttpTransport create() { }); return defaultCredentials; } + + public static HttpRequest createRequest() throws IOException { + return createRequest(new MockLowLevelHttpRequest()); + } + + public static HttpRequest createRequest(MockLowLevelHttpRequest request) throws IOException { + HttpTransport transport = new MockHttpTransport.Builder() + .setLowLevelHttpRequest(request) + .build(); + HttpRequestFactory requestFactory = transport.createRequestFactory(); + return requestFactory.buildPostRequest(TEST_URL, new EmptyContent()); + } } From b686b4f1006c842208147bd87706b81b825a1bd6 Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Sun, 24 Feb 2019 16:31:44 -0800 Subject: [PATCH 12/19] More code cleanup --- .../firebase/internal/RetryInitializer.java | 39 ++++++++----------- .../internal/RetryInitializerTest.java | 10 ++--- 2 files changed, 21 insertions(+), 28 deletions(-) diff --git a/src/main/java/com/google/firebase/internal/RetryInitializer.java b/src/main/java/com/google/firebase/internal/RetryInitializer.java index 0e6cd5b95..ed0dd67b1 100644 --- a/src/main/java/com/google/firebase/internal/RetryInitializer.java +++ b/src/main/java/com/google/firebase/internal/RetryInitializer.java @@ -27,9 +27,9 @@ import java.io.IOException; /** - * Configures HTTP requests to be retried. Failures caused by I/O errors are always retried - * according to the specified {@link RetryConfig}. Failures caused by unsuccessful HTTP responses - * are first referred to the {@code HttpUnsuccessfulResponseHandler} already set on the request. If + * Configures HTTP requests to be retried. Requests that encounter I/O errors are always retried + * with exponential back off. Requests failing with unsuccessful HTTP responses are first referred + * to the {@code HttpUnsuccessfulResponseHandler} that was originally set on the request. If * the request does not get retried at that level, {@link RetryUnsuccessfulResponseHandler} is used * to schedule additional retries. */ @@ -55,7 +55,7 @@ public void initialize(HttpRequest request) { private HttpUnsuccessfulResponseHandler newUnsuccessfulResponseHandler(HttpRequest request) { RetryUnsuccessfulResponseHandler retryHandler = new RetryUnsuccessfulResponseHandler( retryConfig); - return RetryHandlerDecorator.decorate(retryHandler, request); + return new RetryHandlerDecorator(retryHandler, request); } private HttpIOExceptionHandler newIOExceptionHandler() { @@ -70,19 +70,12 @@ private HttpIOExceptionHandler newIOExceptionHandler() { */ private static class RetryHandlerDecorator implements HttpUnsuccessfulResponseHandler { - private final HttpUnsuccessfulResponseHandler preRetryHandler; private final RetryUnsuccessfulResponseHandler retryHandler; + private final HttpUnsuccessfulResponseHandler preRetryHandler; private RetryHandlerDecorator( - HttpUnsuccessfulResponseHandler preRetryHandler, - RetryUnsuccessfulResponseHandler retryHandler) { - this.preRetryHandler = checkNotNull(preRetryHandler); - this.retryHandler = checkNotNull(retryHandler); - } - - static RetryHandlerDecorator decorate( RetryUnsuccessfulResponseHandler retryHandler, HttpRequest request) { - + this.retryHandler = checkNotNull(retryHandler); HttpUnsuccessfulResponseHandler preRetryHandler = request.getUnsuccessfulResponseHandler(); if (preRetryHandler == null) { preRetryHandler = new HttpUnsuccessfulResponseHandler() { @@ -93,7 +86,7 @@ public boolean handleResponse( } }; } - return new RetryHandlerDecorator(preRetryHandler, retryHandler); + this.preRetryHandler = preRetryHandler; } @Override @@ -101,15 +94,17 @@ public boolean handleResponse( HttpRequest request, HttpResponse response, boolean supportsRetry) throws IOException { - boolean retry = preRetryHandler.handleResponse(request, response, supportsRetry); - if (!retry) { - retry = retryHandler.handleResponse(request, response, supportsRetry); + try { + boolean retry = preRetryHandler.handleResponse(request, response, supportsRetry); + if (!retry) { + retry = retryHandler.handleResponse(request, response, supportsRetry); + } + return retry; + } finally { + // Pre-retry handler may have reset the unsuccessful response handler on the + // request. This changes it back. + request.setUnsuccessfulResponseHandler(this); } - - // Pre-retry handler may have reset the unsuccessful response handler on the - // request. This changes it back. - request.setUnsuccessfulResponseHandler(this); - return retry; } } } diff --git a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java index b872f64b4..6d9800f80 100644 --- a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java @@ -42,14 +42,12 @@ public class RetryInitializerTest { - private static final RetryConfig RETRY_CONFIG = RetryConfig.builder() - .setMaxRetries(5) - .setRetryStatusCodes(ImmutableList.of(503)) - .build(); - @Test public void testEnableRetry() throws IOException { - RetryInitializer initializer = new RetryInitializer(RETRY_CONFIG); + RetryInitializer initializer = new RetryInitializer(RetryConfig.builder() + .setMaxRetries(5) + .setRetryStatusCodes(ImmutableList.of(503)) + .build()); HttpRequest request = TestUtils.createRequest(); initializer.initialize(request); From dd1723477d5bf7e2966b344e06fb8edfc5326a21 Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Sun, 24 Feb 2019 20:44:21 -0800 Subject: [PATCH 13/19] Cleaning up tests --- .../internal/CountingLowLevelHttpRequest.java | 10 ++-- .../FirebaseRequestInitializerTest.java | 22 ++++--- .../internal/RetryInitializerTest.java | 60 +++++++++---------- .../RetryUnsuccessfulResponseHandlerTest.java | 43 ++++++------- 4 files changed, 69 insertions(+), 66 deletions(-) diff --git a/src/test/java/com/google/firebase/internal/CountingLowLevelHttpRequest.java b/src/test/java/com/google/firebase/internal/CountingLowLevelHttpRequest.java index e7e08cca9..f2a4b9f5a 100644 --- a/src/test/java/com/google/firebase/internal/CountingLowLevelHttpRequest.java +++ b/src/test/java/com/google/firebase/internal/CountingLowLevelHttpRequest.java @@ -35,11 +35,11 @@ private CountingLowLevelHttpRequest(LowLevelHttpResponse response, IOException e this.exception = exception; } - static CountingLowLevelHttpRequest fromResponse(int status) { - return fromResponse(status, null); + static CountingLowLevelHttpRequest fromStatus(int status) { + return fromStatus(status, null); } - static CountingLowLevelHttpRequest fromResponse(int status, Map headers) { + static CountingLowLevelHttpRequest fromStatus(int status, Map headers) { MockLowLevelHttpResponse response = new MockLowLevelHttpResponse() .setStatusCode(status) .setZeroContent(); @@ -48,10 +48,10 @@ static CountingLowLevelHttpRequest fromResponse(int status, Map response.addHeader(entry.getKey(), entry.getValue()); } } - return fromResponse(response); + return fromStatus(response); } - static CountingLowLevelHttpRequest fromResponse(LowLevelHttpResponse response) { + static CountingLowLevelHttpRequest fromStatus(LowLevelHttpResponse response) { return new CountingLowLevelHttpRequest(checkNotNull(response), null); } diff --git a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java index 675a29d84..20e2fc695 100644 --- a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java @@ -35,6 +35,10 @@ public class FirebaseRequestInitializerTest { + private static final int MAX_RETRIES = 5; + private static final int CONNECT_TIMEOUT_MILLIS = 30000; + private static final int READ_TIMEOUT_MILLIS = 60000; + @After public void tearDown() { TestOnlyImplFirebaseTrampolines.clearInstancesForTest(); @@ -63,16 +67,16 @@ public void testDefaultSettings() throws Exception { public void testExplicitTimeouts() throws Exception { FirebaseApp app = FirebaseApp.initializeApp(new FirebaseOptions.Builder() .setCredentials(new MockGoogleCredentials("token")) - .setConnectTimeout(30000) - .setReadTimeout(60000) + .setConnectTimeout(CONNECT_TIMEOUT_MILLIS) + .setReadTimeout(READ_TIMEOUT_MILLIS) .build()); HttpRequest request = TestUtils.createRequest(); FirebaseRequestInitializer initializer = new FirebaseRequestInitializer(app); initializer.initialize(request); - assertEquals(30000, request.getConnectTimeout()); - assertEquals(60000, request.getReadTimeout()); + assertEquals(CONNECT_TIMEOUT_MILLIS, request.getConnectTimeout()); + assertEquals(READ_TIMEOUT_MILLIS, request.getReadTimeout()); assertEquals("Bearer token", request.getHeaders().getAuthorization()); assertEquals(0, request.getNumberOfRetries()); assertNull(request.getIOExceptionHandler()); @@ -85,7 +89,7 @@ public void testExplicitRetryConfig() throws Exception { .setCredentials(new MockGoogleCredentials("token")) .build()); RetryConfig retryConfig = RetryConfig.builder() - .setMaxRetries(5) + .setMaxRetries(MAX_RETRIES) .build(); HttpRequest request = TestUtils.createRequest(); @@ -95,7 +99,7 @@ public void testExplicitRetryConfig() throws Exception { assertEquals(0, request.getConnectTimeout()); assertEquals(0, request.getReadTimeout()); assertEquals("Bearer token", request.getHeaders().getAuthorization()); - assertEquals(5, request.getNumberOfRetries()); + assertEquals(MAX_RETRIES, request.getNumberOfRetries()); assertTrue(request.getIOExceptionHandler() instanceof HttpBackOffIOExceptionHandler); assertNotNull(request.getUnsuccessfulResponseHandler()); } @@ -106,9 +110,9 @@ public void testCredentialsRetryHandler() throws Exception { .setCredentials(new MockGoogleCredentials("token")) .build()); RetryConfig retryConfig = RetryConfig.builder() - .setMaxRetries(5) + .setMaxRetries(MAX_RETRIES) .build(); - CountingLowLevelHttpRequest countingRequest = CountingLowLevelHttpRequest.fromResponse(401); + CountingLowLevelHttpRequest countingRequest = CountingLowLevelHttpRequest.fromStatus(401); HttpRequest request = TestUtils.createRequest(countingRequest); FirebaseRequestInitializer initializer = new FirebaseRequestInitializer(app, retryConfig); initializer.initialize(request); @@ -121,6 +125,6 @@ public void testCredentialsRetryHandler() throws Exception { } assertEquals("Bearer token", request.getHeaders().getAuthorization()); - assertEquals(6, countingRequest.getCount()); + assertEquals(MAX_RETRIES + 1, countingRequest.getCount()); } } diff --git a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java index 6d9800f80..b5bdc3e79 100644 --- a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java @@ -32,6 +32,7 @@ import com.google.api.client.testing.http.MockLowLevelHttpRequest; import com.google.api.client.testing.http.MockLowLevelHttpResponse; import com.google.api.client.testing.util.MockSleeper; +import com.google.api.client.util.Sleeper; import com.google.auth.http.HttpCredentialsAdapter; import com.google.common.collect.ImmutableList; import com.google.firebase.auth.MockGoogleCredentials; @@ -42,17 +43,19 @@ public class RetryInitializerTest { + private static final int MAX_RETRIES = 4; + private static final RetryConfig.Builder TEST_RETRY_CONFIG = RetryConfig.builder() + .setMaxRetries(MAX_RETRIES) + .setRetryStatusCodes(ImmutableList.of(503)); + @Test public void testEnableRetry() throws IOException { - RetryInitializer initializer = new RetryInitializer(RetryConfig.builder() - .setMaxRetries(5) - .setRetryStatusCodes(ImmutableList.of(503)) - .build()); + RetryInitializer initializer = new RetryInitializer(TEST_RETRY_CONFIG.build()); HttpRequest request = TestUtils.createRequest(); initializer.initialize(request); - assertEquals(5, request.getNumberOfRetries()); + assertEquals(MAX_RETRIES, request.getNumberOfRetries()); assertNotNull(request.getUnsuccessfulResponseHandler()); assertTrue(request.getIOExceptionHandler() instanceof HttpBackOffIOExceptionHandler); } @@ -72,10 +75,7 @@ public void testDisableRetry() throws IOException { @Test public void testRetryOnIOException() throws IOException { MockSleeper sleeper = new MockSleeper(); - RetryInitializer initializer = new RetryInitializer(RetryConfig.builder() - .setMaxRetries(4) - .setSleeper(sleeper) - .build()); + RetryInitializer initializer = new RetryInitializer(testRetryConfig(sleeper)); CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromException( new IOException("test error")); @@ -90,20 +90,16 @@ public void testRetryOnIOException() throws IOException { assertEquals("test error", e.getMessage()); } - assertEquals(4, sleeper.getCount()); - assertEquals(5, failingRequest.getCount()); + assertEquals(MAX_RETRIES, sleeper.getCount()); + assertEquals(MAX_RETRIES + 1, failingRequest.getCount()); assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); } @Test public void testRetryOnHttpError() throws IOException { MockSleeper sleeper = new MockSleeper(); - RetryInitializer initializer = new RetryInitializer(RetryConfig.builder() - .setMaxRetries(4) - .setRetryStatusCodes(ImmutableList.of(503)) - .setSleeper(sleeper) - .build()); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(503); + RetryInitializer initializer = new RetryInitializer(testRetryConfig(sleeper)); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromStatus(503); HttpRequest request = TestUtils.createRequest(failingRequest); initializer.initialize(request); final HttpUnsuccessfulResponseHandler retryHandler = request.getUnsuccessfulResponseHandler(); @@ -115,19 +111,15 @@ public void testRetryOnHttpError() throws IOException { assertEquals(503, e.getStatusCode()); } - assertEquals(4, sleeper.getCount()); - assertEquals(5, failingRequest.getCount()); + assertEquals(MAX_RETRIES, sleeper.getCount()); + assertEquals(MAX_RETRIES + 1, failingRequest.getCount()); assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); } @Test public void testMaxRetriesCountIsCumulative() throws IOException { MockSleeper sleeper = new MockSleeper(); - RetryInitializer initializer = new RetryInitializer(RetryConfig.builder() - .setMaxRetries(4) - .setRetryStatusCodes(ImmutableList.of(503)) - .setSleeper(sleeper) - .build()); + RetryInitializer initializer = new RetryInitializer(testRetryConfig(sleeper)); final AtomicInteger counter = new AtomicInteger(0); MockLowLevelHttpRequest failingRequest = new MockLowLevelHttpRequest(){ @@ -151,8 +143,8 @@ public LowLevelHttpResponse execute() throws IOException { assertEquals(503, e.getStatusCode()); } - assertEquals(4, sleeper.getCount()); - assertEquals(5, counter.get()); + assertEquals(MAX_RETRIES, sleeper.getCount()); + assertEquals(MAX_RETRIES + 1, counter.get()); assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); } @@ -169,11 +161,11 @@ public boolean handleResponse( }; MockSleeper sleeper = new MockSleeper(); RetryInitializer initializer = new RetryInitializer(RetryConfig.builder() - .setMaxRetries(4) + .setMaxRetries(MAX_RETRIES) .setRetryStatusCodes(ImmutableList.of(503)) .setSleeper(sleeper) .build()); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(503); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromStatus(503); HttpRequest request = TestUtils.createRequest(failingRequest); credentials.initialize(request); initializer.initialize(request); @@ -186,9 +178,15 @@ public boolean handleResponse( assertEquals(503, e.getStatusCode()); } - assertEquals(5, otherErrorHandlerCalls.get()); - assertEquals(4, sleeper.getCount()); - assertEquals(5, failingRequest.getCount()); + assertEquals(MAX_RETRIES, sleeper.getCount()); + assertEquals(MAX_RETRIES + 1, failingRequest.getCount()); + assertEquals(MAX_RETRIES + 1, otherErrorHandlerCalls.get()); assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); } + + private RetryConfig testRetryConfig(Sleeper sleeper) { + return TEST_RETRY_CONFIG + .setSleeper(sleeper) + .build(); + } } diff --git a/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java b/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java index 157d591f4..9588c5690 100644 --- a/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java @@ -40,6 +40,7 @@ public class RetryUnsuccessfulResponseHandlerTest { + private static final int MAX_RETRIES = 4; private static final RetryConfig.Builder TEST_RETRY_CONFIG = RetryConfig.builder() .setRetryStatusCodes(ImmutableList.of(429, 503)) .setMaxIntervalMillis(120 * 1000); @@ -49,10 +50,10 @@ public void testDoesNotRetryOnUnspecifiedHttpStatus() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(404); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromStatus(404); HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); - request.setNumberOfRetries(4); + request.setNumberOfRetries(MAX_RETRIES); try { request.execute(); @@ -70,10 +71,10 @@ public void testRetryOnHttpClientErrorWhenSpecified() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(429); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromStatus(429); HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); - request.setNumberOfRetries(4); + request.setNumberOfRetries(MAX_RETRIES); try { request.execute(); @@ -82,9 +83,9 @@ public void testRetryOnHttpClientErrorWhenSpecified() throws IOException { assertEquals(429, e.getStatusCode()); } - assertEquals(4, sleeper.getCount()); + assertEquals(MAX_RETRIES, sleeper.getCount()); assertArrayEquals(new long[]{500, 1000, 2000, 4000}, sleeper.getDelays()); - assertEquals(5, failingRequest.getCount()); + assertEquals(MAX_RETRIES + 1, failingRequest.getCount()); } @@ -93,10 +94,10 @@ public void testRetryAfterIsAbsent() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(503); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromStatus(503); HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); - request.setNumberOfRetries(4); + request.setNumberOfRetries(MAX_RETRIES); try { request.execute(); @@ -105,9 +106,9 @@ public void testRetryAfterIsAbsent() throws IOException { assertEquals(503, e.getStatusCode()); } - assertEquals(4, sleeper.getCount()); + assertEquals(MAX_RETRIES, sleeper.getCount()); assertArrayEquals(new long[]{500, 1000, 2000, 4000}, sleeper.getDelays()); - assertEquals(5, failingRequest.getCount()); + assertEquals(MAX_RETRIES + 1, failingRequest.getCount()); } @Test @@ -115,7 +116,7 @@ public void testExponentialBackOffDoesNotExceedMaxInterval() throws IOException MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(503); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromStatus(503); HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); request.setNumberOfRetries(10); @@ -139,11 +140,11 @@ public void testRetryAfterGivenAsSeconds() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromStatus( 503, ImmutableMap.of("retry-after", "2")); HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); - request.setNumberOfRetries(4); + request.setNumberOfRetries(MAX_RETRIES); try { request.execute(); @@ -152,9 +153,9 @@ public void testRetryAfterGivenAsSeconds() throws IOException { assertEquals(503, e.getStatusCode()); } - assertEquals(4, sleeper.getCount()); + assertEquals(MAX_RETRIES, sleeper.getCount()); assertArrayEquals(new long[]{2000, 2000, 2000, 2000}, sleeper.getDelays()); - assertEquals(5, failingRequest.getCount()); + assertEquals(MAX_RETRIES + 1, failingRequest.getCount()); } @Test @@ -168,7 +169,7 @@ public void testRetryAfterGivenAsDate() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper), clock); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromStatus( 503, ImmutableMap.of("retry-after", retryAfter)); HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); @@ -191,7 +192,7 @@ public void testInvalidRetryAfterFailsOverToExpBackOff() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromStatus( 503, ImmutableMap.of("retry-after", "not valid")); HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); @@ -214,11 +215,11 @@ public void testDoesNotRetryWhenRetryAfterIsTooLong() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse( + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromStatus( 503, ImmutableMap.of("retry-after", "121")); HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); - request.setNumberOfRetries(4); + request.setNumberOfRetries(MAX_RETRIES); try { request.execute(); @@ -242,10 +243,10 @@ public void sleep(long millis) throws InterruptedException { }; RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( testRetryConfig(sleeper)); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromResponse(503); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromStatus(503); HttpRequest request = TestUtils.createRequest(failingRequest); request.setUnsuccessfulResponseHandler(handler); - request.setNumberOfRetries(4); + request.setNumberOfRetries(MAX_RETRIES); try { request.execute(); From c870d5a3132c66686bdb701ebc89dea755919283 Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Sun, 24 Feb 2019 21:11:20 -0800 Subject: [PATCH 14/19] Not calling any retry code when RetryConfig = null --- .../internal/FirebaseRequestInitializer.java | 12 ++++++++---- .../google/firebase/internal/RetryInitializer.java | 14 +++++--------- .../internal/FirebaseRequestInitializerTest.java | 6 +++--- .../firebase/internal/RetryInitializerTest.java | 14 +++----------- 4 files changed, 19 insertions(+), 27 deletions(-) diff --git a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java index 14938991c..64b3ffc8b 100644 --- a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java +++ b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java @@ -39,10 +39,14 @@ public FirebaseRequestInitializer(FirebaseApp app) { } public FirebaseRequestInitializer(FirebaseApp app, @Nullable RetryConfig retryConfig) { - this.initializers = ImmutableList.of( - new HttpCredentialsAdapter(ImplFirebaseTrampolines.getCredentials(app)), - new TimeoutInitializer(app.getOptions()), - new RetryInitializer(retryConfig)); + ImmutableList.Builder initializers = + ImmutableList.builder() + .add(new HttpCredentialsAdapter(ImplFirebaseTrampolines.getCredentials(app))) + .add(new TimeoutInitializer(app.getOptions())); + if (retryConfig != null) { + initializers.add(new RetryInitializer(retryConfig)); + } + this.initializers = initializers.build(); } @Override diff --git a/src/main/java/com/google/firebase/internal/RetryInitializer.java b/src/main/java/com/google/firebase/internal/RetryInitializer.java index ed0dd67b1..fe30ad6b3 100644 --- a/src/main/java/com/google/firebase/internal/RetryInitializer.java +++ b/src/main/java/com/google/firebase/internal/RetryInitializer.java @@ -37,19 +37,15 @@ final class RetryInitializer implements HttpRequestInitializer { private final RetryConfig retryConfig; - RetryInitializer(@Nullable RetryConfig retryConfig) { - this.retryConfig = retryConfig; + RetryInitializer(RetryConfig retryConfig) { + this.retryConfig = checkNotNull(retryConfig); } @Override public void initialize(HttpRequest request) { - if (retryConfig != null) { - request.setNumberOfRetries(retryConfig.getMaxRetries()); - request.setUnsuccessfulResponseHandler(newUnsuccessfulResponseHandler(request)); - request.setIOExceptionHandler(newIOExceptionHandler()); - } else { - request.setNumberOfRetries(0); - } + request.setNumberOfRetries(retryConfig.getMaxRetries()); + request.setUnsuccessfulResponseHandler(newUnsuccessfulResponseHandler(request)); + request.setIOExceptionHandler(newIOExceptionHandler()); } private HttpUnsuccessfulResponseHandler newUnsuccessfulResponseHandler(HttpRequest request) { diff --git a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java index 20e2fc695..15b36384e 100644 --- a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java @@ -58,7 +58,7 @@ public void testDefaultSettings() throws Exception { assertEquals(0, request.getConnectTimeout()); assertEquals(0, request.getReadTimeout()); assertEquals("Bearer token", request.getHeaders().getAuthorization()); - assertEquals(0, request.getNumberOfRetries()); + assertEquals(HttpRequest.DEFAULT_NUMBER_OF_RETRIES, request.getNumberOfRetries()); assertNull(request.getIOExceptionHandler()); assertTrue(request.getUnsuccessfulResponseHandler() instanceof HttpCredentialsAdapter); } @@ -78,13 +78,13 @@ public void testExplicitTimeouts() throws Exception { assertEquals(CONNECT_TIMEOUT_MILLIS, request.getConnectTimeout()); assertEquals(READ_TIMEOUT_MILLIS, request.getReadTimeout()); assertEquals("Bearer token", request.getHeaders().getAuthorization()); - assertEquals(0, request.getNumberOfRetries()); + assertEquals(HttpRequest.DEFAULT_NUMBER_OF_RETRIES, request.getNumberOfRetries()); assertNull(request.getIOExceptionHandler()); assertTrue(request.getUnsuccessfulResponseHandler() instanceof HttpCredentialsAdapter); } @Test - public void testExplicitRetryConfig() throws Exception { + public void testRetryConfig() throws Exception { FirebaseApp app = FirebaseApp.initializeApp(new FirebaseOptions.Builder() .setCredentials(new MockGoogleCredentials("token")) .build()); diff --git a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java index b5bdc3e79..1add27976 100644 --- a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java @@ -18,7 +18,6 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -60,16 +59,9 @@ public void testEnableRetry() throws IOException { assertTrue(request.getIOExceptionHandler() instanceof HttpBackOffIOExceptionHandler); } - @Test - public void testDisableRetry() throws IOException { - RetryInitializer initializer = new RetryInitializer(null); - HttpRequest request = TestUtils.createRequest(); - - initializer.initialize(request); - - assertEquals(0, request.getNumberOfRetries()); - assertNull(request.getUnsuccessfulResponseHandler()); - assertNull(request.getIOExceptionHandler()); + @Test(expected = NullPointerException.class) + public void testRetryConfigCannotBeNull() throws IOException { + new RetryInitializer(null); } @Test From f000ec750a707bff00784e1a67e6470cd004c5df Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Mon, 4 Mar 2019 14:37:22 -0800 Subject: [PATCH 15/19] Added an option to enable/disable retries on IO errors. Added some comments --- .../internal/FirebaseRequestInitializer.java | 12 ++++---- .../google/firebase/internal/RetryConfig.java | 18 ++++++++++++ .../firebase/internal/RetryInitializer.java | 4 ++- .../RetryUnsuccessfulResponseHandler.java | 3 ++ .../FirebaseRequestInitializerTest.java | 22 ++++++++++++++ .../internal/RetryInitializerTest.java | 29 +++++++++++++++---- 6 files changed, 75 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java index 64b3ffc8b..e73f93953 100644 --- a/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java +++ b/src/main/java/com/google/firebase/internal/FirebaseRequestInitializer.java @@ -58,18 +58,18 @@ public void initialize(HttpRequest request) throws IOException { private static class TimeoutInitializer implements HttpRequestInitializer { - private final int connectTimeout; - private final int readTimeout; + private final int connectTimeoutMillis; + private final int readTimeoutMillis; TimeoutInitializer(FirebaseOptions options) { - this.connectTimeout = options.getConnectTimeout(); - this.readTimeout = options.getReadTimeout(); + this.connectTimeoutMillis = options.getConnectTimeout(); + this.readTimeoutMillis = options.getReadTimeout(); } @Override public void initialize(HttpRequest request) { - request.setConnectTimeout(connectTimeout); - request.setReadTimeout(readTimeout); + request.setConnectTimeout(connectTimeoutMillis); + request.setReadTimeout(readTimeoutMillis); } } } diff --git a/src/main/java/com/google/firebase/internal/RetryConfig.java b/src/main/java/com/google/firebase/internal/RetryConfig.java index ac169f2cd..4d38143fc 100644 --- a/src/main/java/com/google/firebase/internal/RetryConfig.java +++ b/src/main/java/com/google/firebase/internal/RetryConfig.java @@ -35,6 +35,7 @@ public final class RetryConfig { private static final int INITIAL_INTERVAL_MILLIS = 500; private final List retryStatusCodes; + private final boolean retryOnIOExceptions; private final int maxRetries; private final Sleeper sleeper; private final ExponentialBackOff.Builder backOffBuilder; @@ -46,6 +47,7 @@ private RetryConfig(Builder builder) { this.retryStatusCodes = ImmutableList.of(); } + this.retryOnIOExceptions = builder.retryOnIOExceptions; checkArgument(builder.maxRetries >= 0, "maxRetries must not be negative"); this.maxRetries = builder.maxRetries; this.sleeper = checkNotNull(builder.sleeper); @@ -63,6 +65,10 @@ List getRetryStatusCodes() { return retryStatusCodes; } + boolean isRetryOnIOExceptions() { + return retryOnIOExceptions; + } + int getMaxRetries() { return maxRetries; } @@ -90,6 +96,7 @@ public static Builder builder() { public static final class Builder { private List retryStatusCodes; + private boolean retryOnIOExceptions; private int maxRetries; private int maxIntervalMillis = (int) TimeUnit.MINUTES.toMillis(2); private double backOffMultiplier = 2.0; @@ -110,6 +117,17 @@ public Builder setRetryStatusCodes(List retryStatusCodes) { return this; } + /** + * Sets whether requests should be retried on IOExceptions. + * + * @param retryOnIOExceptions A boolean indicating whether to retry on IOExceptions. + * @return This builder. + */ + public Builder setRetryOnIOExceptions(boolean retryOnIOExceptions) { + this.retryOnIOExceptions = retryOnIOExceptions; + return this; + } + /** * Maximum number of retry attempts for a request. This is the cumulative total for all retries * regardless of their cause (I/O errors and HTTP error responses). diff --git a/src/main/java/com/google/firebase/internal/RetryInitializer.java b/src/main/java/com/google/firebase/internal/RetryInitializer.java index fe30ad6b3..c9f44bcc5 100644 --- a/src/main/java/com/google/firebase/internal/RetryInitializer.java +++ b/src/main/java/com/google/firebase/internal/RetryInitializer.java @@ -45,7 +45,9 @@ final class RetryInitializer implements HttpRequestInitializer { public void initialize(HttpRequest request) { request.setNumberOfRetries(retryConfig.getMaxRetries()); request.setUnsuccessfulResponseHandler(newUnsuccessfulResponseHandler(request)); - request.setIOExceptionHandler(newIOExceptionHandler()); + if (retryConfig.isRetryOnIOExceptions()) { + request.setIOExceptionHandler(newIOExceptionHandler()); + } } private HttpUnsuccessfulResponseHandler newUnsuccessfulResponseHandler(HttpRequest request) { diff --git a/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java b/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java index 0d70f90cb..a3d4bfd17 100644 --- a/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java +++ b/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java @@ -78,6 +78,9 @@ private boolean waitAndRetry(HttpResponse response) throws IOException, Interrup String retryAfterHeader = response.getHeaders().getRetryAfter(); if (!Strings.isNullOrEmpty(retryAfterHeader)) { long intervalMillis = parseRetryAfterHeaderIntoMillis(retryAfterHeader.trim()); + // Retry-after header can specify very long delay intervals (e.g. 24 hours). If we cannot + // wait that long, we should not perform any retries at all. In general it is not correct to + // retry earlier than what the server has recommended to us. if (intervalMillis > retryConfig.getMaxIntervalMillis()) { return false; } diff --git a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java index 15b36384e..1f610f5e3 100644 --- a/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/FirebaseRequestInitializerTest.java @@ -96,6 +96,28 @@ public void testRetryConfig() throws Exception { FirebaseRequestInitializer initializer = new FirebaseRequestInitializer(app, retryConfig); initializer.initialize(request); + assertEquals(0, request.getConnectTimeout()); + assertEquals(0, request.getReadTimeout()); + assertEquals("Bearer token", request.getHeaders().getAuthorization()); + assertEquals(MAX_RETRIES, request.getNumberOfRetries()); + assertNull(request.getIOExceptionHandler()); + assertNotNull(request.getUnsuccessfulResponseHandler()); + } + + @Test + public void testRetryConfigWithIOExceptionHandling() throws Exception { + FirebaseApp app = FirebaseApp.initializeApp(new FirebaseOptions.Builder() + .setCredentials(new MockGoogleCredentials("token")) + .build()); + RetryConfig retryConfig = RetryConfig.builder() + .setMaxRetries(MAX_RETRIES) + .setRetryOnIOExceptions(true) + .build(); + HttpRequest request = TestUtils.createRequest(); + + FirebaseRequestInitializer initializer = new FirebaseRequestInitializer(app, retryConfig); + initializer.initialize(request); + assertEquals(0, request.getConnectTimeout()); assertEquals(0, request.getReadTimeout()); assertEquals("Bearer token", request.getHeaders().getAuthorization()); diff --git a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java index 1add27976..5060a9f03 100644 --- a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java @@ -18,6 +18,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -43,13 +44,10 @@ public class RetryInitializerTest { private static final int MAX_RETRIES = 4; - private static final RetryConfig.Builder TEST_RETRY_CONFIG = RetryConfig.builder() - .setMaxRetries(MAX_RETRIES) - .setRetryStatusCodes(ImmutableList.of(503)); @Test public void testEnableRetry() throws IOException { - RetryInitializer initializer = new RetryInitializer(TEST_RETRY_CONFIG.build()); + RetryInitializer initializer = new RetryInitializer(testRetryConfig(new MockSleeper())); HttpRequest request = TestUtils.createRequest(); initializer.initialize(request); @@ -59,8 +57,24 @@ public void testEnableRetry() throws IOException { assertTrue(request.getIOExceptionHandler() instanceof HttpBackOffIOExceptionHandler); } + @Test + public void testRetryOnIOExceptionDisabled() throws IOException { + RetryInitializer initializer = new RetryInitializer(RetryConfig.builder() + .setMaxRetries(MAX_RETRIES) + .setRetryOnIOExceptions(false) + .setRetryStatusCodes(ImmutableList.of(503)) + .build()); + HttpRequest request = TestUtils.createRequest(); + + initializer.initialize(request); + + assertEquals(MAX_RETRIES, request.getNumberOfRetries()); + assertNotNull(request.getUnsuccessfulResponseHandler()); + assertNull(request.getIOExceptionHandler()); + } + @Test(expected = NullPointerException.class) - public void testRetryConfigCannotBeNull() throws IOException { + public void testRetryConfigCannotBeNull() { new RetryInitializer(null); } @@ -177,7 +191,10 @@ public boolean handleResponse( } private RetryConfig testRetryConfig(Sleeper sleeper) { - return TEST_RETRY_CONFIG + return RetryConfig.builder() + .setMaxRetries(MAX_RETRIES) + .setRetryStatusCodes(ImmutableList.of(503)) + .setRetryOnIOExceptions(true) .setSleeper(sleeper) .build(); } From fded489a7f4305c59e9c1ec22377912f45e1dbfd Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Mon, 4 Mar 2019 14:46:12 -0800 Subject: [PATCH 16/19] New test case --- .../internal/RetryInitializerTest.java | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java index 5060a9f03..841bb4749 100644 --- a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java @@ -190,6 +190,41 @@ public boolean handleResponse( assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); } + @Test + public void testRetryHandlerDoesNotGetOverwritten() throws IOException { + final AtomicInteger otherErrorHandlerCalls = new AtomicInteger(0); + HttpUnsuccessfulResponseHandler credentials = new HttpUnsuccessfulResponseHandler() { + @Override + public boolean handleResponse( + HttpRequest request, HttpResponse response, boolean supportsRetry) throws IOException { + otherErrorHandlerCalls.incrementAndGet(); + request.setUnsuccessfulResponseHandler(this); + throw new IOException("test"); + } + }; + MockSleeper sleeper = new MockSleeper(); + RetryInitializer initializer = new RetryInitializer(RetryConfig.builder() + .setMaxRetries(MAX_RETRIES) + .setRetryStatusCodes(ImmutableList.of(503)) + .setSleeper(sleeper) + .build()); + CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromStatus(503); + HttpRequest request = TestUtils.createRequest(failingRequest); + request.setUnsuccessfulResponseHandler(credentials); + initializer.initialize(request); + final HttpUnsuccessfulResponseHandler retryHandler = request.getUnsuccessfulResponseHandler(); + + try { + request.execute(); + fail("No exception thrown for HTTP error"); + } catch (Exception e) { + assertEquals("test", e.getMessage()); + } + + assertEquals(1, otherErrorHandlerCalls.get()); + assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); + } + private RetryConfig testRetryConfig(Sleeper sleeper) { return RetryConfig.builder() .setMaxRetries(MAX_RETRIES) From c4e4c0f59efcb6cb2d4aed052200d500a7bf267c Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Mon, 11 Mar 2019 16:57:32 -0700 Subject: [PATCH 17/19] Updated some comments; Cleaned up tests --- .../google/firebase/internal/RetryConfig.java | 3 +-- .../firebase/internal/RetryInitializer.java | 16 ++++++++----- .../RetryUnsuccessfulResponseHandler.java | 4 ++++ .../firebase/internal/RetryConfigTest.java | 2 ++ .../internal/RetryInitializerTest.java | 20 +++++++++++----- .../RetryUnsuccessfulResponseHandlerTest.java | 23 ------------------- 6 files changed, 31 insertions(+), 37 deletions(-) diff --git a/src/main/java/com/google/firebase/internal/RetryConfig.java b/src/main/java/com/google/firebase/internal/RetryConfig.java index 4d38143fc..a17780ed0 100644 --- a/src/main/java/com/google/firebase/internal/RetryConfig.java +++ b/src/main/java/com/google/firebase/internal/RetryConfig.java @@ -106,8 +106,7 @@ private Builder() { } /** * Sets a list of HTTP status codes that should be retried. If null or empty, HTTP requests - * will not be retried as long as they result in some HTTP response message. I/O errors - * will still be retried. + * will not be retried as long as they result in some HTTP response message. * * @param retryStatusCodes A list of status codes. * @return This builder. diff --git a/src/main/java/com/google/firebase/internal/RetryInitializer.java b/src/main/java/com/google/firebase/internal/RetryInitializer.java index c9f44bcc5..959376051 100644 --- a/src/main/java/com/google/firebase/internal/RetryInitializer.java +++ b/src/main/java/com/google/firebase/internal/RetryInitializer.java @@ -27,11 +27,11 @@ import java.io.IOException; /** - * Configures HTTP requests to be retried. Requests that encounter I/O errors are always retried - * with exponential back off. Requests failing with unsuccessful HTTP responses are first referred - * to the {@code HttpUnsuccessfulResponseHandler} that was originally set on the request. If - * the request does not get retried at that level, {@link RetryUnsuccessfulResponseHandler} is used - * to schedule additional retries. + * Configures HTTP requests to be retried. Requests that encounter I/O errors are retried if the + * {@link RetryConfig#isRetryOnIOExceptions()} is set. Requests failing with unsuccessful HTTP + * responses are first referred to the {@code HttpUnsuccessfulResponseHandler} that was originally + * set on the request. If the request does not get retried at that level, + * {@link RetryUnsuccessfulResponseHandler} is used to schedule additional retries. */ final class RetryInitializer implements HttpRequestInitializer { @@ -66,7 +66,7 @@ private HttpIOExceptionHandler newIOExceptionHandler() { * handler is called. This is needed since some initializers (e.g. HttpCredentialsAdapter) * register their own error handlers. */ - private static class RetryHandlerDecorator implements HttpUnsuccessfulResponseHandler { + static class RetryHandlerDecorator implements HttpUnsuccessfulResponseHandler { private final RetryUnsuccessfulResponseHandler retryHandler; private final HttpUnsuccessfulResponseHandler preRetryHandler; @@ -104,5 +104,9 @@ public boolean handleResponse( request.setUnsuccessfulResponseHandler(this); } } + + RetryUnsuccessfulResponseHandler getRetryHandler() { + return retryHandler; + } } } diff --git a/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java b/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java index a3d4bfd17..ba06e59a1 100644 --- a/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java +++ b/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java @@ -74,6 +74,10 @@ public boolean handleResponse( return false; } + RetryConfig getRetryConfig() { + return retryConfig; + } + private boolean waitAndRetry(HttpResponse response) throws IOException, InterruptedException { String retryAfterHeader = response.getHeaders().getRetryAfter(); if (!Strings.isNullOrEmpty(retryAfterHeader)) { diff --git a/src/test/java/com/google/firebase/internal/RetryConfigTest.java b/src/test/java/com/google/firebase/internal/RetryConfigTest.java index 36e250697..7fab5075d 100644 --- a/src/test/java/com/google/firebase/internal/RetryConfigTest.java +++ b/src/test/java/com/google/firebase/internal/RetryConfigTest.java @@ -56,6 +56,7 @@ public void testBuilderWithAllSettings() { RetryConfig config = RetryConfig.builder() .setMaxRetries(4) .setRetryStatusCodes(statusCodes) + .setRetryOnIOExceptions(true) .setMaxIntervalMillis(5 * 60 * 1000) .setBackOffMultiplier(1.5) .setSleeper(sleeper) @@ -64,6 +65,7 @@ public void testBuilderWithAllSettings() { assertEquals(2, config.getRetryStatusCodes().size()); assertEquals(statusCodes.get(0), config.getRetryStatusCodes().get(0)); assertEquals(statusCodes.get(1), config.getRetryStatusCodes().get(1)); + assertTrue(config.isRetryOnIOExceptions()); assertEquals(4, config.getMaxRetries()); assertEquals(5 * 60 * 1000, config.getMaxIntervalMillis()); assertEquals(1.5, config.getBackOffMultiplier(), 0.01); diff --git a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java index 841bb4749..8732e8bf5 100644 --- a/src/test/java/com/google/firebase/internal/RetryInitializerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryInitializerTest.java @@ -36,6 +36,7 @@ import com.google.auth.http.HttpCredentialsAdapter; import com.google.common.collect.ImmutableList; import com.google.firebase.auth.MockGoogleCredentials; +import com.google.firebase.internal.RetryInitializer.RetryHandlerDecorator; import com.google.firebase.testing.TestUtils; import java.io.IOException; import java.util.concurrent.atomic.AtomicInteger; @@ -47,13 +48,17 @@ public class RetryInitializerTest { @Test public void testEnableRetry() throws IOException { - RetryInitializer initializer = new RetryInitializer(testRetryConfig(new MockSleeper())); + RetryConfig retryConfig = retryOnIOAndServiceUnavailableErrors(new MockSleeper()); + RetryInitializer initializer = new RetryInitializer(retryConfig); HttpRequest request = TestUtils.createRequest(); initializer.initialize(request); assertEquals(MAX_RETRIES, request.getNumberOfRetries()); - assertNotNull(request.getUnsuccessfulResponseHandler()); + assertTrue(request.getUnsuccessfulResponseHandler() instanceof RetryHandlerDecorator); + RetryUnsuccessfulResponseHandler retryHandler = + ((RetryHandlerDecorator) request.getUnsuccessfulResponseHandler()).getRetryHandler(); + assertSame(retryConfig, retryHandler.getRetryConfig()); assertTrue(request.getIOExceptionHandler() instanceof HttpBackOffIOExceptionHandler); } @@ -81,7 +86,8 @@ public void testRetryConfigCannotBeNull() { @Test public void testRetryOnIOException() throws IOException { MockSleeper sleeper = new MockSleeper(); - RetryInitializer initializer = new RetryInitializer(testRetryConfig(sleeper)); + RetryInitializer initializer = new RetryInitializer( + retryOnIOAndServiceUnavailableErrors(sleeper)); CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromException( new IOException("test error")); @@ -104,7 +110,8 @@ public void testRetryOnIOException() throws IOException { @Test public void testRetryOnHttpError() throws IOException { MockSleeper sleeper = new MockSleeper(); - RetryInitializer initializer = new RetryInitializer(testRetryConfig(sleeper)); + RetryInitializer initializer = new RetryInitializer( + retryOnIOAndServiceUnavailableErrors(sleeper)); CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromStatus(503); HttpRequest request = TestUtils.createRequest(failingRequest); initializer.initialize(request); @@ -125,7 +132,8 @@ public void testRetryOnHttpError() throws IOException { @Test public void testMaxRetriesCountIsCumulative() throws IOException { MockSleeper sleeper = new MockSleeper(); - RetryInitializer initializer = new RetryInitializer(testRetryConfig(sleeper)); + RetryInitializer initializer = new RetryInitializer( + retryOnIOAndServiceUnavailableErrors(sleeper)); final AtomicInteger counter = new AtomicInteger(0); MockLowLevelHttpRequest failingRequest = new MockLowLevelHttpRequest(){ @@ -225,7 +233,7 @@ public boolean handleResponse( assertSame(retryHandler, request.getUnsuccessfulResponseHandler()); } - private RetryConfig testRetryConfig(Sleeper sleeper) { + private RetryConfig retryOnIOAndServiceUnavailableErrors(Sleeper sleeper) { return RetryConfig.builder() .setMaxRetries(MAX_RETRIES) .setRetryStatusCodes(ImmutableList.of(503)) diff --git a/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java b/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java index 9588c5690..077e9e52f 100644 --- a/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java +++ b/src/test/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandlerTest.java @@ -88,29 +88,6 @@ public void testRetryOnHttpClientErrorWhenSpecified() throws IOException { assertEquals(MAX_RETRIES + 1, failingRequest.getCount()); } - - @Test - public void testRetryAfterIsAbsent() throws IOException { - MultipleCallSleeper sleeper = new MultipleCallSleeper(); - RetryUnsuccessfulResponseHandler handler = new RetryUnsuccessfulResponseHandler( - testRetryConfig(sleeper)); - CountingLowLevelHttpRequest failingRequest = CountingLowLevelHttpRequest.fromStatus(503); - HttpRequest request = TestUtils.createRequest(failingRequest); - request.setUnsuccessfulResponseHandler(handler); - request.setNumberOfRetries(MAX_RETRIES); - - try { - request.execute(); - fail("No exception thrown for HTTP error"); - } catch (HttpResponseException e) { - assertEquals(503, e.getStatusCode()); - } - - assertEquals(MAX_RETRIES, sleeper.getCount()); - assertArrayEquals(new long[]{500, 1000, 2000, 4000}, sleeper.getDelays()); - assertEquals(MAX_RETRIES + 1, failingRequest.getCount()); - } - @Test public void testExponentialBackOffDoesNotExceedMaxInterval() throws IOException { MultipleCallSleeper sleeper = new MultipleCallSleeper(); From 52cff1f3a408212af43c8be680017df2e5e3c385 Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Mon, 11 Mar 2019 17:01:16 -0700 Subject: [PATCH 18/19] Fixed a typo in a comment --- .../java/com/google/firebase/internal/RetryInitializer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/google/firebase/internal/RetryInitializer.java b/src/main/java/com/google/firebase/internal/RetryInitializer.java index 959376051..fbe0a13ea 100644 --- a/src/main/java/com/google/firebase/internal/RetryInitializer.java +++ b/src/main/java/com/google/firebase/internal/RetryInitializer.java @@ -27,7 +27,7 @@ import java.io.IOException; /** - * Configures HTTP requests to be retried. Requests that encounter I/O errors are retried if the + * Configures HTTP requests to be retried. Requests that encounter I/O errors are retried if * {@link RetryConfig#isRetryOnIOExceptions()} is set. Requests failing with unsuccessful HTTP * responses are first referred to the {@code HttpUnsuccessfulResponseHandler} that was originally * set on the request. If the request does not get retried at that level, From 0c63ed61678db1d521d8b867357fad00f41573da Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Wed, 13 Mar 2019 11:01:06 -0700 Subject: [PATCH 19/19] Removing the hard dependency on Apache HTTP Client (#259) * Copied DateUtils source from Apache HC * Updated reference link * Used locks instead of thread locals; Added tests * Added a NOTICE file for third-party code --- NOTICE.txt | 5 + .../google/firebase/internal/DateUtils.java | 108 ++++++++++++++++++ .../RetryUnsuccessfulResponseHandler.java | 1 - .../firebase/internal/DateUtilsTest.java | 80 +++++++++++++ 4 files changed, 193 insertions(+), 1 deletion(-) create mode 100644 NOTICE.txt create mode 100644 src/main/java/com/google/firebase/internal/DateUtils.java create mode 100644 src/test/java/com/google/firebase/internal/DateUtilsTest.java diff --git a/NOTICE.txt b/NOTICE.txt new file mode 100644 index 000000000..b6c5d02b2 --- /dev/null +++ b/NOTICE.txt @@ -0,0 +1,5 @@ +Firebase Admin Java SDK +Copyright 2019 Google Inc. + +This product includes software developed at +The Apache Software Foundation (http://www.apache.org/). diff --git a/src/main/java/com/google/firebase/internal/DateUtils.java b/src/main/java/com/google/firebase/internal/DateUtils.java new file mode 100644 index 000000000..6c4eeb7b0 --- /dev/null +++ b/src/main/java/com/google/firebase/internal/DateUtils.java @@ -0,0 +1,108 @@ +/* + * 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.internal; + +import static com.google.common.base.Preconditions.checkNotNull; + +import java.text.ParsePosition; +import java.text.SimpleDateFormat; +import java.util.Calendar; +import java.util.Date; +import java.util.TimeZone; + +/** + * A utility class for parsing and formatting HTTP dates as used in cookies and + * other headers. This class handles dates as defined by RFC 2616 section + * 3.3.1 as well as some other common non-standard formats. + * + *

Most of this class was borrowed from the + * + * Apache HTTP client in order to avoid a direct dependency on it. We currently + * have a transitive dependency on this library (via Google API client), but the API + * client team is working towards removing it, so we won't have it in the classpath for long. + * + *

The original implementation of this class uses + * thread locals to cache the {@code SimpleDateFormat} instances. Instead, this implementation + * uses static constants and explicit locking to ensure thread safety. This is probably slower, + * but also simpler and avoids memory leaks that may result from unreleased thread locals. + */ +final class DateUtils { + + /** + * Date format pattern used to parse HTTP date headers in RFC 1123 format. + */ + static final String PATTERN_RFC1123 = "EEE, dd MMM yyyy HH:mm:ss zzz"; + + /** + * Date format pattern used to parse HTTP date headers in RFC 1036 format. + */ + static final String PATTERN_RFC1036 = "EEE, dd-MMM-yy HH:mm:ss zzz"; + + /** + * Date format pattern used to parse HTTP date headers in ANSI C + * {@code asctime()} format. + */ + static final String PATTERN_ASCTIME = "EEE MMM d HH:mm:ss yyyy"; + + private static final SimpleDateFormat[] DEFAULT_PATTERNS = new SimpleDateFormat[] { + new SimpleDateFormat(PATTERN_RFC1123), + new SimpleDateFormat(PATTERN_RFC1036), + new SimpleDateFormat(PATTERN_ASCTIME) + }; + + static final TimeZone GMT = TimeZone.getTimeZone("GMT"); + + static { + final Calendar calendar = Calendar.getInstance(); + calendar.setTimeZone(GMT); + calendar.set(2000, Calendar.JANUARY, 1, 0, 0, 0); + calendar.set(Calendar.MILLISECOND, 0); + final Date defaultTwoDigitYearStart = calendar.getTime(); + + for (final SimpleDateFormat datePattern : DEFAULT_PATTERNS) { + datePattern.set2DigitYearStart(defaultTwoDigitYearStart); + } + } + + /** + * Parses the date value using the given date formats. + * + * @param dateValue the date value to parse + * @return the parsed date or null if input could not be parsed + */ + public static Date parseDate(final String dateValue) { + String v = checkNotNull(dateValue); + // trim single quotes around date if present + // see issue #5279 + if (v.length() > 1 && v.startsWith("'") && v.endsWith("'")) { + v = v.substring(1, v.length() - 1); + } + + for (final SimpleDateFormat datePattern : DEFAULT_PATTERNS) { + final ParsePosition pos = new ParsePosition(0); + synchronized (datePattern) { + final Date result = datePattern.parse(v, pos); + if (pos.getIndex() != 0) { + return result; + } + } + } + return null; + } + + /** This class should not be instantiated. */ + private DateUtils() { + } +} diff --git a/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java b/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java index ba06e59a1..dd00d2bfb 100644 --- a/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java +++ b/src/main/java/com/google/firebase/internal/RetryUnsuccessfulResponseHandler.java @@ -28,7 +28,6 @@ import com.google.common.base.Strings; import java.io.IOException; import java.util.Date; -import org.apache.http.client.utils.DateUtils; /** * An {@code HttpUnsuccessfulResponseHandler} that retries failing requests after an interval. The diff --git a/src/test/java/com/google/firebase/internal/DateUtilsTest.java b/src/test/java/com/google/firebase/internal/DateUtilsTest.java new file mode 100644 index 000000000..7c2dfd089 --- /dev/null +++ b/src/test/java/com/google/firebase/internal/DateUtilsTest.java @@ -0,0 +1,80 @@ +/* + * Copyright 2019 Google Inc. + * + * 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.internal; + +import java.util.Calendar; +import java.util.Date; +import org.junit.Assert; +import org.junit.Test; + +/** + * Unit tests for the {@link DateUtils}. Adapted from the tests available in the + * + * Apache HTTP client library. + */ +public class DateUtilsTest { + + @Test + public void testBasicDateParse() { + final Calendar calendar = Calendar.getInstance(); + calendar.setTimeZone(DateUtils.GMT); + calendar.set(2005, Calendar.OCTOBER, 14, 0, 0, 0); + calendar.set(Calendar.MILLISECOND, 0); + final Date date1 = calendar.getTime(); + + Date date2 = DateUtils.parseDate("Fri, 14 Oct 2005 00:00:00 GMT"); + Assert.assertEquals(date1, date2); + date2 = DateUtils.parseDate("Fri, 14 Oct 2005 00:00:00 GMT"); + Assert.assertEquals(date1, date2); + date2 = DateUtils.parseDate("Fri, 14 Oct 2005 00:00:00 GMT"); + Assert.assertEquals(date1, date2); + } + + @Test + public void testInvalidInput() { + try { + DateUtils.parseDate(null); + Assert.fail("NullPointerException should have been thrown"); + } catch (NullPointerException ex) { + // expected + } + } + + @Test + public void testTwoDigitYearDateParse() { + final Calendar calendar = Calendar.getInstance(); + calendar.setTimeZone(DateUtils.GMT); + calendar.set(2005, Calendar.OCTOBER, 14, 0, 0, 0); + calendar.set(Calendar.MILLISECOND, 0); + Date date1 = calendar.getTime(); + + Date date2 = DateUtils.parseDate("Friday, 14-Oct-05 00:00:00 GMT"); + Assert.assertEquals(date1, date2); + } + + @Test + public void testParseQuotedDate() { + final Calendar calendar = Calendar.getInstance(); + calendar.setTimeZone(DateUtils.GMT); + calendar.set(2005, Calendar.OCTOBER, 14, 0, 0, 0); + calendar.set(Calendar.MILLISECOND, 0); + final Date date1 = calendar.getTime(); + + final Date date2 = DateUtils.parseDate("'Fri, 14 Oct 2005 00:00:00 GMT'"); + Assert.assertEquals(date1, date2); + } +}