feat: Add optional non blocking refresh for sync auth code #1368

clundin25 · 2023-08-11T21:58:17Z

No description provided.

sai-sunder-s · 2023-09-13T17:45:46Z

google/auth/credentials.py

+        if not self.expiry:
+            return TokenState.FRESH
+
+        refresh_window = _helpers.utcnow() >= (self.expiry - _helpers.REFRESH_THRESHOLD)


rename this variable to something like is_fresh?

sai-sunder-s · 2023-09-13T17:47:30Z

google/auth/credentials.py

+            else:
+                self.refresh(request)
+
+        if self.token_state == TokenState.INVALID:
            self.refresh(request)


Multiple requests may end up doing this refresh right?
Can we have a lock on this sync refresh?

The code is using a bounded queue which should reduce duplicate work. There is a risk that there is a small amount of duplicate work, if two threads queue work at the same time. The queue itself is thread safe + has locks.

I believe using the queue to reduce duplicate work is acceptable, and that multiple threads will perform the refresh occasionally.

There should be no data corruption due to this

…ptive-refresh

google/auth/credentials.py

westarle · 2023-12-13T15:06:59Z

google/auth/credentials.py

@@ -171,11 +188,38 @@ def before_request(self, request, method, url, headers):
        # pylint: disable=unused-argument
        # (Subclasses may use these arguments to ascertain information about
        # the http request.)
-        if not self.valid:
+
+        if self.token_state == TokenState.FRESH:


This section doesn't guard access to refresh worker resources with your feature flag.

Maybe something like:

if _use_non_blocking_refresh(): _non_blocking_refresh() # thread maintenance, stale handling, refresh on invalid else _blocking_refresh() # refresh on stale or invalid

google/auth/credentials.py

westarle · 2023-12-13T15:12:37Z

google/auth/impersonated_credentials.py

@@ -259,7 +259,11 @@ def _update_token(self, request):
        """

        # Refresh our source credentials if it is not valid.
-        if not self._source_credentials.valid:
+        if (
+            not self._source_credentials.valid


can we remove this dependency and juse use TokenState.INVALID?

Good point, I've made this change

google/auth/_refresh_worker.py

westarle · 2023-12-13T16:32:06Z

google/auth/_refresh_worker.py

+            # it can be flaky due to the scheduler.
+            _LOGGER.error(f"Background refresh failed due to: {err}")
+            if not self._error_queue.full():
+                self._error_queue.put_nowait(err)


with the code above, you can just stuff the error into a field.

google/auth/_refresh_worker.py

google/auth/credentials.py

google/auth/_refresh_worker.py

westarle · 2023-12-15T19:28:31Z

google/auth/credentials.py

+    def with_non_blocking_refresh(self):
+        self._use_non_blocking_refresh = True
+
+    def get_background_refresh_error(self):


I think I would prefer to remove this from the public API:

it is not relevant without the feature flag

it isn't clear when you'd want to call it

it's hard to implement safely

westarle · 2023-12-15T19:29:05Z

google/auth/credentials.py

+class TokenState(Enum):
+    """
+    Tracks the state of a token.
+    FRESH: The token is valid. It is not expired or close to expired, or the token has no expiry. To make it mutually exclusive to STALE.


sorry about this: the "To make it mutually exclusive to STALE" part wasn't meant to be part of the comment.

westarle · 2023-12-15T19:40:15Z

google/auth/_refresh_worker.py

+                # rety this error.
+                err, self._worker._error_info = self._worker._error_info, None
+
+                raise e.RefreshError(


this has a kind of weird 'every 2nd call fails' pattern that doesn't seem quite right.

I would suggest we try to background refresh once, log and record the error, then do foreground refreshes from then on until we get a new token.

You might return false from start_refresh to cause the caller to call refresh on their own thread. The caller would be responsible for calling "clear error" any time they have a good token from refresh().

Sounds good!

westarle · 2023-12-15T19:41:27Z

google/auth/_refresh_worker.py

+                self._worker = RefreshThread(cred=cred, request=request)
+                self._worker.start()
+
+    def has_error(self):


I would remove this and get_error from the public API since they are subject to the lock and difficult to reason about outside of this class.

Okay that sounds good!

westarle · 2023-12-15T19:54:05Z

google/auth/credentials.py

+            self.refresh(request)
+
+    def _non_blocking_refresh(self, request):
+        if (


following the comment above, this would become:

if self.token_state == TokenState.FRESH: return need_refresh = True if self.token_state == TokenState.STALE: need_refresh = not self.refresh_worker.start_refresh(self, request) if need_refresh: self.refresh(request) self._refresh_worker.clear_error()

Done! I kept the INVALID case, should a refresh was never called in the STALE state.

westarle · 2023-12-15T19:57:41Z

google/auth/_refresh_worker.py

+        self._worker = None
+        self._lock = threading.Lock()  # protects access to worker threads.
+
+    def start_refresh(self, cred, request):


is request immutable or can we create a copy here?

Can you expand on why copy it? I don't think it is necessary, as the request should not be re-used in other places.

I think copying the request would be a breaking change.

I'd have to do some work to understand the implication of deep copying a request object.

google/auth/_refresh_worker.py

google/auth/credentials.py

clundin25 requested review from arithmetic1728 and sai-sunder-s August 11, 2023 21:59

clundin25 force-pushed the pre-emptive-refresh branch from 6dffc03 to b244471 Compare September 12, 2023 17:36

clundin25 marked this pull request as ready for review September 12, 2023 17:36

clundin25 requested review from a team as code owners September 12, 2023 17:36

sai-sunder-s reviewed Sep 13, 2023

View reviewed changes

clundin25 added the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 2, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 2, 2023

clundin25 added 5 commits November 9, 2023 12:35

feat: Add optional non blocking refresh for sync auth code

a07551f

Rename refresh_window to is_stale.

20f8ec4

Add API to get background refresh errors.

377e19c

Apply error handling feedback.

31e2f33

chore: Refresh system test creds.

aab77bb

clundin25 force-pushed the pre-emptive-refresh branch from d99a734 to aab77bb Compare November 9, 2023 20:36

clundin25 added 5 commits November 9, 2023 13:12

Fix linter.

1848c2a

No cover issue.

8d741d5

Fix coverage test.

20941ed

Fix linter.

3ca4b97

chore: Refresh system test creds.

c7b11f8

clundin25 added the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 10, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 10, 2023

Fixed linter error.

0417312

BigTailWolf approved these changes Nov 13, 2023

View reviewed changes

BigTailWolf and others added 5 commits November 13, 2023 10:27

Merge branch 'main' into pre-emptive-refresh

64e1c7c

Fix coverage test.

b94d6e1

Add no cover.

88c00e1

Merge remote-tracking branch 'origin/pre-emptive-refresh' into pre-em…

e2623f4

…ptive-refresh

chore: Refresh system test creds.

b5281bd

westarle reviewed Dec 13, 2023

View reviewed changes

clundin25 added 6 commits December 13, 2023 17:04

PR feedback.

a19fe0d

Merge remote-tracking branch 'upstream/main' into pre-emptive-refresh

0e35c67

chore: Refresh system test creds.

f0b218e

Fix lint

a6b1815

Use field to store error in RefreshThread.

df7220c

Remove some left over constants. Combine locks.

89ceeeb

westarle reviewed Dec 14, 2023

View reviewed changes

google/auth/_refresh_worker.py Outdated Show resolved Hide resolved

google/auth/credentials.py Outdated Show resolved Hide resolved

google/auth/_refresh_worker.py Outdated Show resolved Hide resolved

clundin25 added 4 commits December 14, 2023 10:53

PR feedback.

41042c2

Remove exception deadlock

f557447

Don't try to refresh stale tokens that have an active error.

4cecc4a

Fix cover check and logic error on has_error()

be2e2da

clundin25 added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 14, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 14, 2023

clundin25 requested a review from westarle December 15, 2023 18:00

westarle reviewed Dec 15, 2023

View reviewed changes

PR feedback.

d941394

clundin25 requested a review from westarle December 15, 2023 22:38

Deep copy request object when starting background refresh.

f5e77e5

westarle approved these changes Dec 18, 2023

View reviewed changes

google/auth/_refresh_worker.py Outdated Show resolved Hide resolved

google/auth/credentials.py Show resolved Hide resolved

PR feedback.

be5f27e

clundin25 added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 18, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 18, 2023

Merge remote-tracking branch 'upstream/main' into pre-emptive-refresh

fcf1f8a

clundin25 added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 18, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 18, 2023

clundin25 merged commit a6dc2c3 into googleapis:main Dec 18, 2023
14 checks passed

release-please bot mentioned this pull request Dec 15, 2023

chore(main): release 2.26.0 #1442

Merged

jackwotherspoon mentioned this pull request Mar 24, 2024

Token refresh uses blocking I/O in async function GoogleCloudPlatform/alloydb-python-connector#245

Open

clundin25 mentioned this pull request Apr 19, 2024

Implement pre-emptive credential refresh #1161

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add optional non blocking refresh for sync auth code #1368

feat: Add optional non blocking refresh for sync auth code #1368

clundin25 commented Aug 11, 2023

sai-sunder-s Sep 13, 2023

sai-sunder-s Sep 13, 2023

clundin25 Sep 13, 2023

westarle Dec 13, 2023

westarle Dec 13, 2023

clundin25 Dec 14, 2023

westarle Dec 13, 2023

westarle Dec 15, 2023

westarle Dec 15, 2023

westarle Dec 15, 2023

clundin25 Dec 15, 2023

westarle Dec 15, 2023

clundin25 Dec 15, 2023

westarle Dec 15, 2023

clundin25 Dec 15, 2023

westarle Dec 15, 2023

clundin25 Dec 15, 2023

feat: Add optional non blocking refresh for sync auth code #1368

feat: Add optional non blocking refresh for sync auth code #1368

Conversation

clundin25 commented Aug 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment