FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: resolve the issue where rpc timeout of 0 is used when timeout expires by parthea · Pull Request #776 · googleapis/python-api-core · GitHub

This repository was archived by the owner on Feb 23, 2026. It is now read-only.
/ python-api-core Public archive

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

Merged
parthea merged 5 commits into from
Jan 16, 2025
Conversation 6 Commits 5 Checks 0 Files changed
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 .py  (2) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
18 changes: 14 additions & 4 deletions google/api_core/timeout.py
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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:

ohmayr Jan 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

parthea Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

vchudnov-g Jan 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

parthea Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

vchudnov-g Jan 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

parthea Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Back | FazBrowse Home | New Git URL