Adding retryOnExecuteError in HttpRequest to handle allow retries when there is an IOException

This CL is to fix issue http://code.google.com/p/google-http-java-client/issues/detail?id=83 : Handling connection failure

I decided only to use retries and not exponential backoff because BackoffPolicy is not really suited for this since it requires a statusCode to determine when it should be invoked. In this case HttpResponse will be null, I believe retries should be sufficient here.

I also debugged HttpRequest.execute in eclipse and disconnected my network connection to see what would happen. LowLevelHttpRequest.execute threw an java.net.UnknownHostException (subclass of IOException).

[yanivi, 2012-04-10 12:10:56]

I haven't gotten a chance to review yet, but can you please also check how
this behaves with ApacheTransport and UrlFetchTransport?
-- Yaniv

On Monday, April 9, 2012, wrote:

> Reviewers: yanivi,
>
> Description:
> This CL is to fix issue
>
http://code.google.com/p/**google-http-java-client/**issues/detail?id=83<http...:
> Handling connection failure
>
> I decided only to use retries and not exponential backoff because
> BackoffPolicy is not really suited for this since it requires a
> statusCode to determine when it should be invoked. In this case
> HttpResponse will be null, I believe retries should be sufficient here.
>
> I also debugged HttpRequest.execute in eclipse and disconnected my
> network connection to see what would happen. LowLevelHttpRequest.execute
> threw an java.net.UnknownHostException (subclass of IOException).
>
>
> Please review this at
http://codereview.appspot.com/**5989068/<http://codereview.appspot.com/5989068/>
>
> Affected files:
>  M google-http-client/src/main/**java/com/google/api/client/**
> http/HttpRequest.java
>  M google-http-client/src/test/**java/com/google/api/client/**
> http/HttpRequestTest.java
>
>
>

-- 
Yaniv Inbar
Senior Software Engineer
Google Inc.

[rmistry, 2012-04-10 15:16:05]

On 2012/04/10 12:10:56, yanivi wrote:
> I haven't gotten a chance to review yet, but can you please also check how
> this behaves with ApacheTransport and UrlFetchTransport?
> -- Yaniv

Tested with the other transports, they throw IOException and go through the
retry mechanism in this CL.

> 
> On Monday, April 9, 2012, wrote:
> 
> > Reviewers: yanivi,
> >
> > Description:
> > This CL is to fix issue
> >
>
http://code.google.com/p/**google-http-java-client/**issues/detail?id=83%3Cht...:
> > Handling connection failure
> >
> > I decided only to use retries and not exponential backoff because
> > BackoffPolicy is not really suited for this since it requires a
> > statusCode to determine when it should be invoked. In this case
> > HttpResponse will be null, I believe retries should be sufficient here.
> >
> > I also debugged HttpRequest.execute in eclipse and disconnected my
> > network connection to see what would happen. LowLevelHttpRequest.execute
> > threw an java.net.UnknownHostException (subclass of IOException).
> >
> >
> > Please review this at
>
http://codereview.appspot.com/**5989068/%3Chttp://codereview.appspot.com/5989...>
> >
> > Affected files:
> >  M google-http-client/src/main/**java/com/google/api/client/**
> > http/HttpRequest.java
> >  M google-http-client/src/test/**java/com/google/api/client/**
> > http/HttpRequestTest.java
> >
> >
> >
> 
> -- 
> Yaniv Inbar
> Senior Software Engineer
> Google Inc.

[yanivi, 2012-04-17 13:15:39]

some tricky design questions...

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:180:
private boolean retryOnExecuteError = false;
should we possibly name this retryOnExecuteIOException?  I'm afraid "error" is
too general.

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:671:
* Returns whether to retry the request if an IOException is encountered in
{@link IOException}
similarly below

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:712:
boolean requiresRetry = false;
readability: please remove one of requiresRetry or retrySupported

It is really hard to understand the subtle difference between requiresRetry and
retrySupported.  We should just merge them.

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:720:
HttpResponse response = null;
don't we need response = null and executeException = null inside the
do-while-loop, otherwise they are not properly initialized the second time
around?

would be great to add a unit test for this

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:852:
response = new HttpResponse(this, lowLevelHttpRequest.execute());
[optional] Can we please separate this into two lines?  Ideally I'd like new
HttpResponse() outside of the try-catch here since it doesn't throw an
exception.

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:854:
if (retryOnExecuteError) {
[optional] cleaner way to write this:

        if (!retryOnExecuteError) {
          throw e;
        }
        requiresRetry = true;
        // Save the exception incase the retries do not work and we need to
rethrow it later.
        executeException = e;


Not that I am being very picky about style and readability issues here because
this method is already very complicated.

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:856:
// Save the exception incase the retries do not work and we need to rethrow it
later.
"in case"

[optional] you might want to enable Eclipse spell checker (General > Editors >
Text Editors > Spelling)

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:857:
executeException = e;
design: I wonder if we should log the exception at a config level.

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:863:
if (response != null && !response.isSuccessStatusCode()) {
response won't be null the second time around

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:878:
} else if (retrySupported && backOffPolicy != null
design question: would the user expect backOffPolicy to be activated on a
connection error?  Is there a reasonable expectation that if cannot cannot now,
that we will be able to connect 3ms later?

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:891:
requiresRetry = errorHandled || redirectRequest || backOffRetry;
here you could possibly do:

        retrySupported &= errorHandled || redirectRequest || backOffRetry;

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:905:
} else if (throwExceptionOnExecuteError && !response.isSuccessStatusCode()) {
[optional] remove the "else" since that's implied by the throw

[rmistry, 2012-04-18 11:33:54]

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:180:
private boolean retryOnExecuteError = false;
On 2012/04/17 13:15:40, yanivi wrote:
> should we possibly name this retryOnExecuteIOException?  I'm afraid "error" is
> too general.

Done.

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:671:
* Returns whether to retry the request if an IOException is encountered in
On 2012/04/17 13:15:40, yanivi wrote:
> {@link IOException}
> similarly below

Done.

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:712:
boolean requiresRetry = false;
On 2012/04/17 13:15:40, yanivi wrote:
> readability: please remove one of requiresRetry or retrySupported
> 
> It is really hard to understand the subtle difference between requiresRetry
and
> retrySupported.  We should just merge them.

Yes it is hard to understand, I guess we have stared at them for so long that
they made sense for us. Merged them.

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:720:
HttpResponse response = null;
On 2012/04/17 13:15:40, yanivi wrote:
> don't we need response = null and executeException = null inside the
> do-while-loop, otherwise they are not properly initialized the second time
> around?
> 
> would be great to add a unit test for this

Done.

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:852:
response = new HttpResponse(this, lowLevelHttpRequest.execute());
On 2012/04/17 13:15:40, yanivi wrote:
> [optional] Can we please separate this into two lines?  Ideally I'd like new
> HttpResponse() outside of the try-catch here since it doesn't throw an
> exception.

This does not work because if an exception is thrown then LowLevelHttpResponse
is null and throws an NPE in HttpResponse constructor.

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:854:
if (retryOnExecuteError) {
On 2012/04/17 13:15:40, yanivi wrote:
> [optional] cleaner way to write this:
> 
>         if (!retryOnExecuteError) {
>           throw e;
>         }
>         requiresRetry = true;
>         // Save the exception incase the retries do not work and we need to
> rethrow it later.
>         executeException = e;
> 
> 
> Not that I am being very picky about style and readability issues here because
> this method is already very complicated.  

Done.

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:856:
// Save the exception incase the retries do not work and we need to rethrow it
later.
On 2012/04/17 13:15:40, yanivi wrote:
> "in case"
> 
> [optional] you might want to enable Eclipse spell checker (General > Editors >
> Text Editors > Spelling)

Done.

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:857:
executeException = e;
On 2012/04/17 13:15:40, yanivi wrote:
> design: I wonder if we should log the exception at a config level.

Can you elaborate on this please?

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:863:
if (response != null && !response.isSuccessStatusCode()) {
On 2012/04/17 13:15:40, yanivi wrote:
> response won't be null the second time around

If an IOException is thrown and we catch it then response could be null.

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:878:
} else if (retrySupported && backOffPolicy != null
On 2012/04/17 13:15:40, yanivi wrote:
> design question: would the user expect backOffPolicy to be activated on a
> connection error?  Is there a reasonable expectation that if cannot cannot
now,
> that we will be able to connect 3ms later?

I thought of this earlier and decided not to add exponential backoff for
connection errors. My reasoning:

Exponential backoff makes the most sense for server errors because if the server
is down we do not want to bombard it with simultaneous requests at the same time
when it is trying to come back up. For connection errors this reasoning does not
apply, connection could be as bad or as good in 3ms as right now, which is why I
concluded that simple retries would suffice.

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:891:
requiresRetry = errorHandled || redirectRequest || backOffRetry;
On 2012/04/17 13:15:40, yanivi wrote:
> here you could possibly do:
> 
>         retrySupported &= errorHandled || redirectRequest || backOffRetry;

Done.

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:905:
} else if (throwExceptionOnExecuteError && !response.isSuccessStatusCode()) {
On 2012/04/17 13:15:40, yanivi wrote:
> [optional] remove the "else" since that's implied by the throw

Done.

[yanivi, 2012-04-19 20:31:57]

just one design question, but otherwise looks good

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:857:
executeException = e;
On 2012/04/18 11:33:55, rmistry wrote:
> On 2012/04/17 13:15:40, yanivi wrote:
> > design: I wonder if we should log the exception at a config level.
> 
> Can you elaborate on this please?

Question is whether we should do this:

        logger.log(Level.WARNING, e.getMessage(), e);

[rmistry, 2012-04-20 11:19:54]

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
File
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java
(right):

http://codereview.appspot.com/5989068/diff/1001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:857:
executeException = e;
On 2012/04/19 20:31:57, yanivi wrote:
> On 2012/04/18 11:33:55, rmistry wrote:
> > On 2012/04/17 13:15:40, yanivi wrote:
> > > design: I wonder if we should log the exception at a config level.
> > 
> > Can you elaborate on this please?
> 
> Question is whether we should do this:
> 
>         logger.log(Level.WARNING, e.getMessage(), e);

If the exception is silently consumed and the next retry succeeds then there
would be no indication that there ever was an exception. Makes sense to me that
we should log it.
Done.

[yanivi, 2012-04-20 13:57:57]

LGTM