Retry http request when unexpected error occurs #97

Merged
merged 4 commits into from Jul 13, 2017

Conversation

Projects
None yet
5 participants
@serihiro
Contributor

serihiro commented Feb 25, 2017

What will this PR change?

  • implement #89
    • retry when request failed or timeout in GCECredentials#fetch_access_token and UserRefreshCredentials#revoke!
    • when give up retrying, raise Signet::AuthorizationError with the string of original_error.inpsect
@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Feb 25, 2017

Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.
Collaborator

googlebot commented Feb 25, 2017

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no label Feb 25, 2017

@serihiro

This comment has been minimized.

Show comment
Hide comment
@serihiro

serihiro Feb 25, 2017

Contributor

@googlebot I signed it!

Contributor

serihiro commented Feb 25, 2017

@googlebot I signed it!

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Feb 25, 2017

Collaborator

CLAs look good, thanks!

Collaborator

googlebot commented Feb 25, 2017

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Feb 25, 2017

@serihiro

This comment has been minimized.

Show comment
Hide comment
@serihiro

serihiro Feb 25, 2017

Contributor

@blowmage
Could you review this? I implemented for this issue #89 which you wrote.

Contributor

serihiro commented Feb 25, 2017

@blowmage
Could you review this? I implemented for this issue #89 which you wrote.

@blowmage

Looks great!

lib/googleauth/signet.rb
+
+ if retry_count > 0
+ retry_count -= 1
+ retry

This comment has been minimized.

@blowmage

blowmage Feb 25, 2017

Contributor

Do you want to implement an incremental backoff before the retry?

@blowmage

blowmage Feb 25, 2017

Contributor

Do you want to implement an incremental backoff before the retry?

This comment has been minimized.

@serihiro

serihiro Feb 25, 2017

Contributor

@blowmage
Thank you for your review and comment!

Do you want to implement an incremental backoff before the retry?

Yes.
I think there is no problem.

I think this rescue block behaves like the following:

  • retry_count == 5 =>retry
  • retry_count == 4 =>retry
  • retry_count == 3 =>retry
  • retry_count == 2 =>retry
  • retry_count == 1 =>retry
  • retry_count == 0 =>raise

example:

client = GCECredentials.new
client.retry_with_error do
  puts 'hey'
  raise
end
hey # first execute
hey # 1st retry
hey # 2nd retry
hey # 3rd retry
hey # 4th retry
hey # 5th retry
# raise Signet::AuthorizationError

Do you have something to worry about this behavior?

@serihiro

serihiro Feb 25, 2017

Contributor

@blowmage
Thank you for your review and comment!

Do you want to implement an incremental backoff before the retry?

Yes.
I think there is no problem.

I think this rescue block behaves like the following:

  • retry_count == 5 =>retry
  • retry_count == 4 =>retry
  • retry_count == 3 =>retry
  • retry_count == 2 =>retry
  • retry_count == 1 =>retry
  • retry_count == 0 =>raise

example:

client = GCECredentials.new
client.retry_with_error do
  puts 'hey'
  raise
end
hey # first execute
hey # 1st retry
hey # 2nd retry
hey # 3rd retry
hey # 4th retry
hey # 5th retry
# raise Signet::AuthorizationError

Do you have something to worry about this behavior?

This comment has been minimized.

@blowmage

blowmage Feb 26, 2017

Contributor

I've usually seen retries with an accompanying backoff. So there is an increasing delay between each retry. That seems to be a common Googleism.

@blowmage

blowmage Feb 26, 2017

Contributor

I've usually seen retries with an accompanying backoff. So there is an increasing delay between each retry. That seems to be a common Googleism.

This comment has been minimized.

@serihiro

serihiro Feb 26, 2017

Contributor

I see. So I will fix like this. Is this what you mean?

def retry_with_error(max_retry_count = 5)
  retry_count = 0

  begin
    yield
  rescue => e
    raise e if e.is_a?(Signet::AuthorizationError) || e.is_a?(Signet::ParseError)

    if retry_count < max_retry_count
      retry_count += 1
      retry
    else
      msg = "Unexpected error: #{e.inspect}"
      raise(Signet::AuthorizationError, msg)
    end
  end
end
@serihiro

serihiro Feb 26, 2017

Contributor

I see. So I will fix like this. Is this what you mean?

def retry_with_error(max_retry_count = 5)
  retry_count = 0

  begin
    yield
  rescue => e
    raise e if e.is_a?(Signet::AuthorizationError) || e.is_a?(Signet::ParseError)

    if retry_count < max_retry_count
      retry_count += 1
      retry
    else
      msg = "Unexpected error: #{e.inspect}"
      raise(Signet::AuthorizationError, msg)
    end
  end
end

This comment has been minimized.

@blowmage

blowmage Feb 26, 2017

Contributor

I would expect to see some sort of increment of the delay between the retries.

def retry_with_error(max_retry_count = 5)
  retry_count = 0

  begin
    yield
  rescue => e
    raise e if e.is_a?(Signet::AuthorizationError) || e.is_a?(Signet::ParseError)

    if retry_count < max_retry_count
      retry_count += 1
      sleep retry_count*0.3
      retry
    else
      msg = "Unexpected error: #{e.inspect}"
      raise(Signet::AuthorizationError, msg)
    end
  end
end
@blowmage

blowmage Feb 26, 2017

Contributor

I would expect to see some sort of increment of the delay between the retries.

def retry_with_error(max_retry_count = 5)
  retry_count = 0

  begin
    yield
  rescue => e
    raise e if e.is_a?(Signet::AuthorizationError) || e.is_a?(Signet::ParseError)

    if retry_count < max_retry_count
      retry_count += 1
      sleep retry_count*0.3
      retry
    else
      msg = "Unexpected error: #{e.inspect}"
      raise(Signet::AuthorizationError, msg)
    end
  end
end
@serihiro

This comment has been minimized.

Show comment
Hide comment
@serihiro

serihiro Feb 26, 2017

Contributor

@blowmage
Thank you fore your feedback. I fixed retry_with_error method and squashed into 960cac2 .
Travis CI job was failed for rubocop error, but I confirmed all specs were passed in my env(ruby 2.3.3p222).

Contributor

serihiro commented Feb 26, 2017

@blowmage
Thank you fore your feedback. I fixed retry_with_error method and squashed into 960cac2 .
Travis CI job was failed for rubocop error, but I confirmed all specs were passed in my env(ruby 2.3.3p222).

@blowmage blowmage referenced this pull request in GoogleCloudPlatform/google-cloud-ruby Feb 27, 2017

Closed

Datastore: invalid UnauthenticatedError exception #896

@hxiong388

This comment has been minimized.

Show comment
Hide comment
@hxiong388

hxiong388 Jul 13, 2017

Member

@serihiro Thanks for submitting this PR.

Our team has recently taken over the ownership of this repository, and I'm in process of triaging the PRs and issues. I'll merge this PR once you can fix the rubocop violations.

Member

hxiong388 commented Jul 13, 2017

@serihiro Thanks for submitting this PR.

Our team has recently taken over the ownership of this repository, and I'm in process of triaging the PRs and issues. I'll merge this PR once you can fix the rubocop violations.

lib/googleauth/compute_engine.rb
+ when 404
+ raise(Signet::AuthorizationError, NO_METADATA_SERVER_ERROR)
+ else
+ msg = "Unexpected error code #{resp.status}" + UNEXPECTED_ERROR_SUFFIX

This comment has been minimized.

@hxiong388

hxiong388 Jul 13, 2017

Member

This exceeds 80 characters max line length.

@hxiong388

hxiong388 Jul 13, 2017

Member

This exceeds 80 characters max line length.

lib/googleauth/signet.rb
+ begin
+ yield
+ rescue => e
+ raise e if e.is_a?(Signet::AuthorizationError) || e.is_a?(Signet::ParseError)

This comment has been minimized.

@hxiong388

hxiong388 Jul 13, 2017

Member

This exceeds 80 characters max line length.

@hxiong388

hxiong388 Jul 13, 2017

Member

This exceeds 80 characters max line length.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jul 13, 2017

Coverage Status

Coverage increased (+0.03%) to 98.443% when pulling 227ba09 on serihiro:retry-request-with-unexpected-error into 0f7fe1e on google:master.

coveralls commented Jul 13, 2017

Coverage Status

Coverage increased (+0.03%) to 98.443% when pulling 227ba09 on serihiro:retry-request-with-unexpected-error into 0f7fe1e on google:master.

@serihiro

This comment has been minimized.

Show comment
Hide comment
@serihiro

serihiro Jul 13, 2017

Contributor

@hxiong388
I fixed rubocop violations and rebased master branch.
Could you check this again?

Contributor

serihiro commented Jul 13, 2017

@hxiong388
I fixed rubocop violations and rebased master branch.
Could you check this again?

@hxiong388 hxiong388 merged commit 8665196 into google:master Jul 13, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hxiong388

This comment has been minimized.

Show comment
Hide comment
@hxiong388

hxiong388 Jul 13, 2017

Member

@serihiro Thanks! Looks great.

Member

hxiong388 commented Jul 13, 2017

@serihiro Thanks! Looks great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment