Tweak retry policy. 40x errors aren't typically recoverable other than 401s in the case of expired access tokens. Even then, 1 retry is enough
This commit is contained in:
		
							parent
							
								
									0c8e067012
								
							
						
					
					
						commit
						bfa5225766
					
				|  | @ -591,9 +591,12 @@ module Google | ||||||
| 
 | 
 | ||||||
|       connection = options[:connection] || self.connection |       connection = options[:connection] || self.connection | ||||||
|       request.authorization = options[:authorization] || self.authorization unless options[:authenticated] == false |       request.authorization = options[:authorization] || self.authorization unless options[:authenticated] == false | ||||||
|  |        | ||||||
|       tries = 1 + (options[:retries] || self.retries) |       tries = 1 + (options[:retries] || self.retries) | ||||||
|  | 
 | ||||||
|       Retriable.retriable :tries => tries,  |       Retriable.retriable :tries => tries,  | ||||||
|                           :on => [TransmissionError], |                           :on => [TransmissionError], | ||||||
|  |                           :on_retry => client_error_handler(request.authorization), | ||||||
|                           :interval => lambda {|attempts| (2 ** attempts) + rand} do |                           :interval => lambda {|attempts| (2 ** attempts) + rand} do | ||||||
|         result = request.send(connection, true) |         result = request.send(connection, true) | ||||||
| 
 | 
 | ||||||
|  | @ -607,14 +610,6 @@ module Google | ||||||
|             })) |             })) | ||||||
|             raise RedirectError.new(result.headers['location'], result) |             raise RedirectError.new(result.headers['location'], result) | ||||||
|           when 400...500 |           when 400...500 | ||||||
|             if result.status == 401 && request.authorization.respond_to?(:refresh_token) && auto_refresh_token |  | ||||||
|               begin |  | ||||||
|                 logger.debug("Attempting refresh of access token & retry of request") |  | ||||||
|                 request.authorization.fetch_access_token! |  | ||||||
|               rescue Signet::AuthorizationError |  | ||||||
|                  # Ignore since we want the original error |  | ||||||
|               end |  | ||||||
|             end |  | ||||||
|             raise ClientError.new(result.error_message || "A client error has occurred", result) |             raise ClientError.new(result.error_message || "A client error has occurred", result) | ||||||
|           when 500...600 |           when 500...600 | ||||||
|             raise ServerError.new(result.error_message || "A server error has occurred", result) |             raise ServerError.new(result.error_message || "A server error has occurred", result) | ||||||
|  | @ -665,6 +660,31 @@ module Google | ||||||
|       return Addressable::Template.new(@base_uri + template).expand(mapping) |       return Addressable::Template.new(@base_uri + template).expand(mapping) | ||||||
|     end |     end | ||||||
|      |      | ||||||
|  | 
 | ||||||
|  |     ## | ||||||
|  |     # Returns on proc for special processing of retries as not all client errors | ||||||
|  |     # are recoverable. Only 401s should be retried and only if the credentials | ||||||
|  |     # are refreshable | ||||||
|  |     # | ||||||
|  |     # @param [#fetch_access_token!] authorization | ||||||
|  |     #   OAuth 2 credentials | ||||||
|  |     # @return [Proc]  | ||||||
|  |     def client_error_handler(authorization)   | ||||||
|  |       can_refresh = authorization.respond_to?(:refresh_token) && auto_refresh_token  | ||||||
|  |       Proc.new do |exception, tries| | ||||||
|  |         next unless exception.kind_of?(ClientError) | ||||||
|  |         if exception.result.status == 401 && can_refresh && tries == 1 | ||||||
|  |           begin | ||||||
|  |             logger.debug("Attempting refresh of access token & retry of request") | ||||||
|  |             authorization.fetch_access_token! | ||||||
|  |             next | ||||||
|  |           rescue Signet::AuthorizationError | ||||||
|  |           end | ||||||
|  |         end | ||||||
|  |         raise exception | ||||||
|  |       end | ||||||
|  |     end | ||||||
|  | 
 | ||||||
|   end |   end | ||||||
| 
 | 
 | ||||||
| end | end | ||||||
|  |  | ||||||
|  | @ -168,7 +168,7 @@ describe Google::APIClient do | ||||||
|      end |      end | ||||||
|   end   |   end   | ||||||
| 
 | 
 | ||||||
|   describe 'when retiries enabled' do |   describe 'when retries enabled' do | ||||||
|     before do |     before do | ||||||
|       client.retries = 2 |       client.retries = 2 | ||||||
|     end |     end | ||||||
|  | @ -213,6 +213,40 @@ describe Google::APIClient do | ||||||
|       ) |       ) | ||||||
|     end |     end | ||||||
| 
 | 
 | ||||||
|  | 
 | ||||||
|  |     it 'should not attempt multiple token refreshes' do | ||||||
|  |       client.authorization.access_token = '12345' | ||||||
|  |       expect(client.authorization).to receive(:fetch_access_token!).once | ||||||
|  | 
 | ||||||
|  |       @connection = stub_connection do |stub| | ||||||
|  |         stub.get('/foo') do |env| | ||||||
|  |           [401, {}, '{}'] | ||||||
|  |         end | ||||||
|  |       end | ||||||
|  | 
 | ||||||
|  |       client.execute(   | ||||||
|  |         :uri => 'https://www.gogole.com/foo', | ||||||
|  |         :connection => @connection | ||||||
|  |       ) | ||||||
|  |     end | ||||||
|  | 
 | ||||||
|  |     it 'should not retry on client errors' do | ||||||
|  |       count = 0 | ||||||
|  |       @connection = stub_connection do |stub| | ||||||
|  |         stub.get('/foo') do |env| | ||||||
|  |           count.should == 0 | ||||||
|  |           count += 1 | ||||||
|  |           [403, {}, '{}'] | ||||||
|  |         end | ||||||
|  |       end | ||||||
|  | 
 | ||||||
|  |       client.execute(   | ||||||
|  |         :uri => 'https://www.gogole.com/foo', | ||||||
|  |         :connection => @connection, | ||||||
|  |         :authenticated => false | ||||||
|  |       ) | ||||||
|  |     end | ||||||
|  | 
 | ||||||
|     it 'should retry on 500 errors' do |     it 'should retry on 500 errors' do | ||||||
|       client.authorization = nil |       client.authorization = nil | ||||||
| 
 | 
 | ||||||
|  |  | ||||||
		Loading…
	
		Reference in New Issue