Skip to content

Commit 229f433

Browse files
rmistryrmistry
committed
[Issue 83]: Handling connection failure
http://codereview.appspot.com/5989068/
1 parent f9d50dc commit 229f433

File tree

2 files changed

+167
-33
lines changed

2 files changed

+167
-33
lines changed

‎google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java‎

Lines changed: 64 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,12 @@ static String executeAndGetValueOfSomeCustomHeader(HttpRequest request) {
173173
*/
174174
private boolean throwExceptionOnExecuteError = true;
175175

176+
/**
177+
* Whether to retry the request if an {@link IOException} is encountered in
178+
* {@link LowLevelHttpRequest#execute()}.
179+
*/
180+
private boolean retryOnExecuteIOException = false;
181+
176182
/**
177183
* @param transport HTTP transport
178184
* @param method HTTP request method (may be {@code null}
@@ -661,6 +667,31 @@ public HttpRequest setThrowExceptionOnExecuteError(boolean throwExceptionOnExecu
661667
return this;
662668
}
663669

670+
/**
671+
* Returns whether to retry the request if an {@link IOException} is encountered in
672+
* {@link LowLevelHttpRequest#execute()}.
673+
*
674+
* @since 1.9
675+
*/
676+
public boolean getRetryOnExecuteIOException() {
677+
return retryOnExecuteIOException;
678+
}
679+
680+
/**
681+
* Sets whether to retry the request if an {@link IOException} is encountered in
682+
* {@link LowLevelHttpRequest#execute()}.
683+
*
684+
* <p>
685+
* The default value is {@code false}.
686+
* </p>
687+
*
688+
* @since 1.9
689+
*/
690+
public HttpRequest setRetryOnExecuteIOException(boolean retryOnExecuteIOException) {
691+
this.retryOnExecuteIOException = retryOnExecuteIOException;
692+
return this;
693+
}
694+
664695
/**
665696
* Execute the HTTP request and returns the HTTP response.
666697
* <p>
@@ -678,7 +709,6 @@ public HttpRequest setThrowExceptionOnExecuteError(boolean throwExceptionOnExecu
678709
* @see HttpResponse#isSuccessStatusCode()
679710
*/
680711
public HttpResponse execute() throws IOException {
681-
boolean requiresRetry = false;
682712
boolean retrySupported = false;
683713
Preconditions.checkArgument(numRetries >= 0);
684714
int retriesRemaining = numRetries;
@@ -687,6 +717,7 @@ public HttpResponse execute() throws IOException {
687717
backOffPolicy.reset();
688718
}
689719
HttpResponse response = null;
720+
IOException executeException;
690721

691722
Preconditions.checkNotNull(method);
692723
Preconditions.checkNotNull(url);
@@ -696,6 +727,10 @@ public HttpResponse execute() throws IOException {
696727
if (response != null) {
697728
response.ignore();
698729
}
730+
731+
response = null;
732+
executeException = null;
733+
699734
// run the interceptor
700735
if (interceptor != null) {
701736
interceptor.intercept(this);
@@ -809,16 +844,24 @@ public HttpResponse execute() throws IOException {
809844
logger.config(logbuf.toString());
810845
}
811846

812-
// execute
813-
lowLevelHttpRequest.setTimeout(connectTimeout, readTimeout);
814-
response = new HttpResponse(this, lowLevelHttpRequest.execute());
815-
816847
// We need to make sure our content type can support retry
817848
// null content is inherently able to be retried
818849
retrySupported = retriesRemaining > 0 && (content == null || content.retrySupported());
819-
requiresRetry = false;
820850

821-
if (!response.isSuccessStatusCode()) {
851+
// execute
852+
lowLevelHttpRequest.setTimeout(connectTimeout, readTimeout);
853+
try {
854+
response = new HttpResponse(this, lowLevelHttpRequest.execute());
855+
} catch (IOException e) {
856+
if (!retryOnExecuteIOException) {
857+
throw e;
858+
}
859+
// Save the exception in case the retries do not work and we need to re-throw it later.
860+
executeException = e;
861+
logger.log(Level.WARNING, e.getMessage(), e);
862+
}
863+
864+
if (response != null && !response.isSuccessStatusCode()) {
822865
boolean errorHandled = false;
823866
boolean redirectRequest = false;
824867
boolean backOffRetry = false;
@@ -846,16 +889,24 @@ public HttpResponse execute() throws IOException {
846889
}
847890
// A retry is required if the error was successfully handled or if it is a redirect request
848891
// or if the back off policy determined a retry is necessary.
849-
requiresRetry = errorHandled || redirectRequest || backOffRetry;
850-
// Once there are no more retries remaining, this will be -1
851-
// Count redirects as retries, we want a finite limit of redirects.
852-
retriesRemaining--;
892+
retrySupported &= (errorHandled || redirectRequest || backOffRetry);
853893
// need to close the response stream before retrying a request
854-
if (requiresRetry && retrySupported) {
894+
if (retrySupported) {
855895
response.ignore();
856896
}
897+
} else {
898+
// Retry is not required for a successful status code unless the response is null.
899+
retrySupported &= (response == null);
857900
}
858-
} while (requiresRetry && retrySupported);
901+
// Once there are no more retries remaining, this will be -1
902+
// Count redirects as retries, we want a finite limit of redirects.
903+
retriesRemaining--;
904+
} while (retrySupported);
905+
906+
if (response == null) {
907+
// Retries did not help resolve the execute exception, re-throw it.
908+
throw executeException;
909+
}
859910

860911
if (throwExceptionOnExecuteError && !response.isSuccessStatusCode()) {
861912
throw new HttpResponseException(response);

‎google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java‎

Lines changed: 103 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
/*
22
* Copyright (c) 2010 Google Inc.
3-
*
3+
*
44
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
55
* in compliance with the License. You may obtain a copy of the License at
6-
*
6+
*
77
* http://www.apache.org/licenses/LICENSE-2.0
8-
*
8+
*
99
* Unless required by applicable law or agreed to in writing, software distributed under the License
1010
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
1111
* or implied. See the License for the specific language governing permissions and limitations under
@@ -37,7 +37,7 @@
3737

3838
/**
3939
* Tests {@link HttpRequest}.
40-
*
40+
*
4141
* @author Yaniv Inbar
4242
*/
4343
public class HttpRequestTest extends TestCase {
@@ -272,13 +272,13 @@ public void testMissingLocationRedirect() throws IOException {
272272
Assert.assertEquals(1, fakeTransport.lowLevelExecCalls);
273273
}
274274

275-
static private class FailThenSuccessTransport extends MockHttpTransport {
275+
static private class FailThenSuccessBackoffTransport extends MockHttpTransport {
276276

277277
public int lowLevelExecCalls;
278278
int errorStatusCode;
279279
int callsBeforeSuccess;
280280

281-
protected FailThenSuccessTransport(int errorStatusCode, int callsBeforeSuccess) {
281+
protected FailThenSuccessBackoffTransport(int errorStatusCode, int callsBeforeSuccess) {
282282
this.errorStatusCode = errorStatusCode;
283283
this.callsBeforeSuccess = callsBeforeSuccess;
284284
}
@@ -310,9 +310,88 @@ public LowLevelHttpRequest buildGetRequest(String url) {
310310
}
311311
}
312312

313+
static private class FailThenSuccessConnectionErrorTransport extends MockHttpTransport {
314+
315+
public int lowLevelExecCalls;
316+
int callsBeforeSuccess;
317+
318+
protected FailThenSuccessConnectionErrorTransport(int callsBeforeSuccess) {
319+
this.callsBeforeSuccess = callsBeforeSuccess;
320+
}
321+
322+
public LowLevelHttpRequest retryableGetRequest = new MockLowLevelHttpRequest() {
323+
324+
@Override
325+
public LowLevelHttpResponse execute() throws IOException {
326+
lowLevelExecCalls++;
327+
328+
if (lowLevelExecCalls <= callsBeforeSuccess) {
329+
throw new IOException();
330+
}
331+
// Return success when count is more than callsBeforeSuccess
332+
MockLowLevelHttpResponse response = new MockLowLevelHttpResponse();
333+
response.setStatusCode(200);
334+
return response;
335+
}
336+
};
337+
338+
@Override
339+
public LowLevelHttpRequest buildGetRequest(String url) {
340+
return retryableGetRequest;
341+
}
342+
}
343+
344+
public void testExecuteErrorWithRetryEnabled() throws IOException {
345+
int callsBeforeSuccess = 3;
346+
FailThenSuccessConnectionErrorTransport fakeTransport =
347+
new FailThenSuccessConnectionErrorTransport(callsBeforeSuccess);
348+
HttpRequest req =
349+
fakeTransport.createRequestFactory().buildGetRequest(new GenericUrl("http://not/used"));
350+
req.setRetryOnExecuteIOException(true);
351+
req.setNumberOfRetries(callsBeforeSuccess + 1);
352+
HttpResponse resp = req.execute();
353+
354+
Assert.assertEquals(200, resp.getStatusCode());
355+
Assert.assertEquals(4, fakeTransport.lowLevelExecCalls);
356+
}
357+
358+
public void testExecuteErrorWithRetryEnabledBeyondRetryLimit() throws IOException {
359+
int callsBeforeSuccess = 11;
360+
FailThenSuccessConnectionErrorTransport fakeTransport =
361+
new FailThenSuccessConnectionErrorTransport(callsBeforeSuccess);
362+
HttpRequest req =
363+
fakeTransport.createRequestFactory().buildGetRequest(new GenericUrl("http://not/used"));
364+
req.setRetryOnExecuteIOException(true);
365+
req.setNumberOfRetries(callsBeforeSuccess - 1);
366+
try {
367+
req.execute();
368+
fail("Expected: " + IOException.class);
369+
} catch (IOException e) {
370+
// Expected
371+
}
372+
Assert.assertEquals(callsBeforeSuccess, fakeTransport.lowLevelExecCalls);
373+
}
374+
375+
public void testExecuteErrorWithRetryDisabled() throws IOException {
376+
int callsBeforeSuccess = 3;
377+
FailThenSuccessConnectionErrorTransport fakeTransport =
378+
new FailThenSuccessConnectionErrorTransport(callsBeforeSuccess);
379+
HttpRequest req =
380+
fakeTransport.createRequestFactory().buildGetRequest(new GenericUrl("http://not/used"));
381+
// retryOnExecuteError is disabled by default.
382+
req.setNumberOfRetries(callsBeforeSuccess + 1);
383+
try {
384+
req.execute();
385+
fail("Expected: " + IOException.class);
386+
} catch (IOException e) {
387+
// Expected
388+
}
389+
Assert.assertEquals(1, fakeTransport.lowLevelExecCalls);
390+
}
391+
313392
public void testAbnormalResponseHandlerWithNoBackOff() throws IOException {
314-
FailThenSuccessTransport fakeTransport =
315-
new FailThenSuccessTransport(HttpStatusCodes.STATUS_CODE_UNAUTHORIZED, 1);
393+
FailThenSuccessBackoffTransport fakeTransport =
394+
new FailThenSuccessBackoffTransport(HttpStatusCodes.STATUS_CODE_UNAUTHORIZED, 1);
316395
MockHttpUnsuccessfulResponseHandler handler = new MockHttpUnsuccessfulResponseHandler(true);
317396

318397
HttpRequest req =
@@ -327,8 +406,8 @@ public void testAbnormalResponseHandlerWithNoBackOff() throws IOException {
327406
}
328407

329408
public void testAbnormalResponseHandlerWithBackOff() throws IOException {
330-
FailThenSuccessTransport fakeTransport =
331-
new FailThenSuccessTransport(HttpStatusCodes.STATUS_CODE_SERVER_ERROR, 1);
409+
FailThenSuccessBackoffTransport fakeTransport =
410+
new FailThenSuccessBackoffTransport(HttpStatusCodes.STATUS_CODE_SERVER_ERROR, 1);
332411
MockHttpUnsuccessfulResponseHandler handler = new MockHttpUnsuccessfulResponseHandler(true);
333412
MockBackOffPolicy backOffPolicy = new MockBackOffPolicy();
334413

@@ -346,8 +425,8 @@ public void testAbnormalResponseHandlerWithBackOff() throws IOException {
346425
}
347426

348427
public void testBackOffSingleCall() throws IOException {
349-
FailThenSuccessTransport fakeTransport =
350-
new FailThenSuccessTransport(HttpStatusCodes.STATUS_CODE_SERVER_ERROR, 1);
428+
FailThenSuccessBackoffTransport fakeTransport =
429+
new FailThenSuccessBackoffTransport(HttpStatusCodes.STATUS_CODE_SERVER_ERROR, 1);
351430
MockHttpUnsuccessfulResponseHandler handler = new MockHttpUnsuccessfulResponseHandler(false);
352431
MockBackOffPolicy backOffPolicy = new MockBackOffPolicy();
353432

@@ -366,8 +445,9 @@ public void testBackOffSingleCall() throws IOException {
366445

367446
public void testBackOffMultipleCalls() throws IOException {
368447
int callsBeforeSuccess = 5;
369-
FailThenSuccessTransport fakeTransport =
370-
new FailThenSuccessTransport(HttpStatusCodes.STATUS_CODE_SERVER_ERROR, callsBeforeSuccess);
448+
FailThenSuccessBackoffTransport fakeTransport =
449+
new FailThenSuccessBackoffTransport(HttpStatusCodes.STATUS_CODE_SERVER_ERROR,
450+
callsBeforeSuccess);
371451
MockHttpUnsuccessfulResponseHandler handler = new MockHttpUnsuccessfulResponseHandler(false);
372452
MockBackOffPolicy backOffPolicy = new MockBackOffPolicy();
373453

@@ -386,13 +466,15 @@ public void testBackOffMultipleCalls() throws IOException {
386466

387467
public void testBackOffCallsBeyondRetryLimit() throws IOException {
388468
int callsBeforeSuccess = 11;
389-
FailThenSuccessTransport fakeTransport =
390-
new FailThenSuccessTransport(HttpStatusCodes.STATUS_CODE_SERVER_ERROR, callsBeforeSuccess);
469+
FailThenSuccessBackoffTransport fakeTransport =
470+
new FailThenSuccessBackoffTransport(HttpStatusCodes.STATUS_CODE_SERVER_ERROR,
471+
callsBeforeSuccess);
391472
MockHttpUnsuccessfulResponseHandler handler = new MockHttpUnsuccessfulResponseHandler(false);
392473
MockBackOffPolicy backOffPolicy = new MockBackOffPolicy();
393474

394475
HttpRequest req =
395476
fakeTransport.createRequestFactory().buildGetRequest(new GenericUrl("http://not/used"));
477+
req.setNumberOfRetries(callsBeforeSuccess - 1);
396478
req.setUnsuccessfulResponseHandler(handler);
397479
req.setBackOffPolicy(backOffPolicy);
398480
try {
@@ -407,8 +489,8 @@ public void testBackOffCallsBeyondRetryLimit() throws IOException {
407489
}
408490

409491
public void testBackOffUnRecognizedStatusCode() throws IOException {
410-
FailThenSuccessTransport fakeTransport =
411-
new FailThenSuccessTransport(HttpStatusCodes.STATUS_CODE_UNAUTHORIZED, 1);
492+
FailThenSuccessBackoffTransport fakeTransport =
493+
new FailThenSuccessBackoffTransport(HttpStatusCodes.STATUS_CODE_UNAUTHORIZED, 1);
412494
MockHttpUnsuccessfulResponseHandler handler = new MockHttpUnsuccessfulResponseHandler(false);
413495
MockBackOffPolicy backOffPolicy = new MockBackOffPolicy();
414496

@@ -430,8 +512,9 @@ public void testBackOffUnRecognizedStatusCode() throws IOException {
430512

431513
public void testBackOffStop() throws IOException {
432514
int callsBeforeSuccess = 5;
433-
FailThenSuccessTransport fakeTransport =
434-
new FailThenSuccessTransport(HttpStatusCodes.STATUS_CODE_SERVER_ERROR, callsBeforeSuccess);
515+
FailThenSuccessBackoffTransport fakeTransport =
516+
new FailThenSuccessBackoffTransport(HttpStatusCodes.STATUS_CODE_SERVER_ERROR,
517+
callsBeforeSuccess);
435518
MockHttpUnsuccessfulResponseHandler handler = new MockHttpUnsuccessfulResponseHandler(false);
436519
MockBackOffPolicy backOffPolicy = new MockBackOffPolicy();
437520
backOffPolicy.returnBackOffStop = true;

0 commit comments

Comments
 (0)