Skip to content

fix: resolve the issue where rpc timeout of 0 is used when timeout expires #776

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 16, 2025
Merged

fix: resolve the issue where rpc timeout of 0 is used when timeout expires #776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
95cde20
fix: resolve the issue where rpc timeout of 0 is used when timeout ex…
parthea Jan 12, 2025
b7ea26e
Merge branch 'main' into fix-incorrect-rpc-timeout
parthea Jan 14, 2025
5b9a2e8
address offline feedback
parthea Jan 16, 2025
30ce320
formatting
parthea Jan 16, 2025
03b27f0
update comment
parthea Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions google/api_core/timeout.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,7 @@ def __call__(self, func):
def func_with_timeout(*args, **kwargs):
"""Wrapped function that adds timeout."""

remaining_timeout = self._timeout
if remaining_timeout is not None:
if self._timeout is not None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this self._timeout is meant to be treated as the total timeout and not a timeout for each retry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some clean up that needs to happen as part of a longer term fix (Googlers see b/388247478) but for now I'd recommend to keep the definitions unchanged.

# All calculations are in seconds
now_timestamp = self._clock().timestamp()

Expand All @@ -114,8 +113,19 @@ def func_with_timeout(*args, **kwargs):
now_timestamp = first_attempt_timestamp

time_since_first_attempt = now_timestamp - first_attempt_timestamp
# Avoid setting negative timeout
kwargs["timeout"] = max(0, self._timeout - time_since_first_attempt)
remaining_timeout = self._timeout - time_since_first_attempt

# Although the `deadline` parameter in `google.api_core.retry.Retry`
# is deprecated, and should be treated the same as the `timeout`,
# it is still possible for the `deadline` argument in
# `google.api_core.retry.Retry` to be larger than the `timeout`.
Comment on lines +120 to +121
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not explain how the deadline value enters here since there's no deadline in this function. It's confusing. Please clarify.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deadline enters via wrap_method linked below, and is used in the calculation for an overall timeout. The fact that there is a separate deadline that can be confirmed is unexpected behavior which will be addressed in a follow up PR.

python-api-core/google/api_core/gapic_v1/method.py

Line 134 in b1fae31

def wrap_method(

# See https://github.com/googleapis/python-api-core/issues/654
# Only positive non-zero timeouts are supported.
# Revert back to the initial timeout for negative or 0 timeout values.
if remaining_timeout < 1:
remaining_timeout = self._timeout
Comment on lines +125 to +126
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this. This seems to be saying that if the next server-RPC timeout is ~0, we should reset it to be the GAPIC-surface level timeout, which seems wrong. I assume we're checking the GAPIC-surface level timeout elsewhere? Where?

If we are indeed checking it elsewhere, clarify that here, and explain how the scenario leading up to this situation means it should be reset to self._timeout instead of, say, 5.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right @vchudnov-g , there is existing incorrect behavior and this simply avoids the specific failure of sending requests with an rpc timeout of 0, which fail immediately. More clarity is needed to document the current incorrect behavior and the workaround in this PR, as well as the recommended long term fix. As per offline discussion, we'll merge this as is and follow up with another PR to update the comments to provide more clarity.


kwargs["timeout"] = remaining_timeout

return func(*args, **kwargs)

Expand Down
4 changes: 2 additions & 2 deletions tests/unit/test_timeout.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,9 @@ def _clock():
wrapped()
target.assert_called_with(timeout=3.0)
wrapped()
target.assert_called_with(timeout=0.0)
target.assert_called_with(timeout=42.0)
wrapped()
target.assert_called_with(timeout=0.0)
target.assert_called_with(timeout=42.0)

def test_apply_no_timeout(self):
target = mock.Mock(spec=["__call__", "__name__"], __name__="target")
Expand Down
Loading