Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

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

Already on GitHub?Sign in to your account

Merged
parthea merged 5 commits intomainfromfix-incorrect-rpc-timeout
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes fromall commits
Commits
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 deletionsgoogle/api_core/timeout.py
View file
Open in desktop
Original file line numberDiff line numberDiff 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 thisself._timeout is meant to be treated as the total timeout and not a timeout for each retry.

Copy link
CollaboratorAuthor

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 thedeadline value enters here since there's nodeadline in this function. It's confusing. Please clarify.

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

The deadline enters viawrap_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.

# 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 toself._timeout instead of, say, 5.

Copy link
CollaboratorAuthor

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 deletionstests/unit/test_timeout.py
View file
Open in desktop
Original file line numberDiff line numberDiff 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
[8]ページ先頭
©2009-2025 Movatter.jp