Timeout parameter doesn't work when retry is configured

[geigerj]

Originally reported by @frankyn

What

Setting CallOptions timeout does nothing when the RPC is configured to retry.

To reproduce

video_service_client = Google::Cloud::VideoIntelligence::V1beta1::VideoIntelligenceServiceClient.new
features             = [Google::Cloud::Videointelligence::V1beta1::Feature::LABEL_DETECTION]                                                                                           
options = Google::Gax::CallOptions.new(timeout: 60)
path = "gs://cloudmleap/video/next/volleyball_court.mp4"
                                                                                                                  
operation = video_service_client.annotate_video(path, features, options: options) do |op|
  ...
end

Expected behavior

The call retries with an initial timeout of 60s.

Observed behavior

The call fails with "INVALID_ARGUMENT" due to an initial timeout of 19s; the configured timeout is ignored. It is possible to change the timeout only by manually configuring the full backoff settings.

[evaogbe]

I'm getting a different error:

GaxError Exception occurred in retry method that was not classified as transient, caused by
7:Google Cloud Video Intelligence API has not been used in project usable-auth-library before or it is
disabled. Enable it by visiting
https://console.developers.google.com/apis/api/videointelligence.googleapis.com/overview?project=usable-auth-library
then retry. If you enabled this API recently, wait a few minutes for the action to propagate to our
systems and retry. (Google::Gax::RetryError)

[geigerj]

@eoogbe Try using the project gapic-test, which should have the API enabled already.

[evaogbe]

It's maxxed on keys. I can't create a new one.

[evaogbe]

Thank you for your bug report. Based on its current description, we are unable to reproduce it. If you are still experiencing this issue and are reasonably certain that it is a bug, please feel free to re-open this issue with additional information to help us reproduce it.

[geigerj]

@eoogbe we've updated the timeout configuration since the original bug report; are you sure you're not just seeing that? I'd be surprised if the root cause was fixed in GAX because AFAIK no work has been done there. Happy to sync offline in more detail.

[geigerj]

I've confirmed the bug still exists with a print debug geigerj@248eab9, even though the configuration has changed for the specific video-intelligence case. I am fairly certain the retry settings are overriding the timeout parameter, even though the expect behavior is for the method parameters to override the configuration.

@eoogbe PTAL.

[evaogbe]

The docs say:

# @param timeout [Numeric] The client-side timeout for API calls. This
#   parameter is ignored for retrying calls.

I don't think the timeout parameter is supposed to affect retry. I was able to test setting the retry_options keyword, which does change the timeout for retry.

Closing as works as intended.

[geigerj]

This is a pretty big usability issue as is -- I don't think we can reasonably expect anyone to pass a full retry configuration as a parameter to a retrying call just to change the initial timeout parameter, and this is not an advanced use case. Let's change the behavior so that the timeout param is not ignored for retrying calls, but instead overrides the initial_rpc_timeout (and the max_rpc_timeout/total_timeout if either of those values is less than the explicitly specified timeout param).

[geigerj]

Also, FWIW, the type of the options parameter is CallOptions, not CallSettings, which does not have the documentation you quoted. I am less concerned about CallSettings, because we don't expect most users to interact with it directly.

[jiren]

This is an issue with retry_codes with empty array. If retry_codes is empty array and timeout param is passed using call option then it is executing retry instead of timeout

i.e here non_idempotent codes is empty so it should try with timeout options not using retryable.

gax-ruby/lib/google/gax/settings.rb

Lines 85 to 87 in ed62589

	def retry_codes?
	@retry_options && @retry_options.retry_codes
	end

gax-ruby/lib/google/gax/api_callable.rb

Line 252 in ed62589

api_call = if settings.retry_codes?

gax-ruby/lib/google/gax/settings.rb

Lines 85 to 87 in ed62589

	def retry_codes?
	@retry_options && @retry_options.retry_codes
	end

Need to check empty

def retry_codes?
   @retry_options && @retry_options.retry_codes && !@retry_options.retry_codes.empty?
end

[lukesneeringer]

This still seems to be in progress.
(Replying here to bring the issue back into compliance.)

[alex-lange]

Hi, Has there been any updates to this issue re: retry_codes being an empty array? The comments specifically say that the RetryOptions#retry_codes is an Array, so I believe this is an actual bug that is treating an empty array like nil (!!nil is false but !![] is true).

I think something like this should fix it:

def retry_codes?
   @retry_options && @retry_options.retry_codes.any?
end

Not sure how this repository works -- I am happy to open a PR for this!

[jbolinger]

Hi @alex-lange we'd be happy to review a PR if you'd like to take a shot a fixing this. Let us know if we can help.