-
Notifications
You must be signed in to change notification settings - Fork 296
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add optional non blocking refresh for sync auth code #1368
Merged
Merged
Changes from all commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
a07551f
feat: Add optional non blocking refresh for sync auth code
clundin25 20f8ec4
Rename refresh_window to is_stale.
clundin25 377e19c
Add API to get background refresh errors.
clundin25 31e2f33
Apply error handling feedback.
clundin25 aab77bb
chore: Refresh system test creds.
clundin25 1848c2a
Fix linter.
clundin25 8d741d5
No cover issue.
clundin25 20941ed
Fix coverage test.
clundin25 3ca4b97
Fix linter.
clundin25 c7b11f8
chore: Refresh system test creds.
clundin25 0417312
Fixed linter error.
clundin25 64e1c7c
Merge branch 'main' into pre-emptive-refresh
BigTailWolf b94d6e1
Fix coverage test.
clundin25 88c00e1
Add no cover.
clundin25 e2623f4
Merge remote-tracking branch 'origin/pre-emptive-refresh' into pre-em…
clundin25 b5281bd
chore: Refresh system test creds.
clundin25 5d30915
One more cover pragma and run linter.
clundin25 4b146c4
Remove unnecessary code change.
clundin25 5476a8f
Merge remote-tracking branch 'upstream/main' into pre-emptive-refresh
clundin25 77ff534
chore: Refresh system test creds.
clundin25 a19fe0d
PR feedback.
clundin25 0e35c67
Merge remote-tracking branch 'upstream/main' into pre-emptive-refresh
clundin25 f0b218e
chore: Refresh system test creds.
clundin25 a6b1815
Fix lint
clundin25 df7220c
Use field to store error in RefreshThread.
clundin25 89ceeeb
Remove some left over constants. Combine locks.
clundin25 41042c2
PR feedback.
clundin25 f557447
Remove exception deadlock
clundin25 4cecc4a
Don't try to refresh stale tokens that have an active error.
clundin25 be2e2da
Fix cover check and logic error on `has_error()`
clundin25 d941394
PR feedback.
clundin25 f5e77e5
Deep copy request object when starting background refresh.
clundin25 be5f27e
PR feedback.
clundin25 fcf1f8a
Merge remote-tracking branch 'upstream/main' into pre-emptive-refresh
clundin25 File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
# Copyright 2023 Google LLC | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import copy | ||
import logging | ||
import threading | ||
|
||
import google.auth.exceptions as e | ||
|
||
_LOGGER = logging.getLogger(__name__) | ||
|
||
|
||
class RefreshThreadManager: | ||
""" | ||
Organizes exactly one background job that refresh a token. | ||
""" | ||
|
||
def __init__(self): | ||
"""Initializes the manager.""" | ||
|
||
self._worker = None | ||
self._lock = threading.Lock() # protects access to worker threads. | ||
|
||
def start_refresh(self, cred, request): | ||
"""Starts a refresh thread for the given credentials. | ||
The credentials are refreshed using the request parameter. | ||
request and cred MUST not be None | ||
|
||
Returns True if a background refresh was kicked off. False otherwise. | ||
|
||
Args: | ||
cred: A credentials object. | ||
request: A request object. | ||
Returns: | ||
bool | ||
""" | ||
if cred is None or request is None: | ||
raise e.InvalidValue( | ||
"Unable to start refresh. cred and request must be valid and instantiated objects." | ||
) | ||
|
||
clundin25 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
with self._lock: | ||
if self._worker is not None and self._worker._error_info is not None: | ||
return False | ||
|
||
if self._worker is None or not self._worker.is_alive(): # pragma: NO COVER | ||
self._worker = RefreshThread(cred=cred, request=copy.deepcopy(request)) | ||
self._worker.start() | ||
return True | ||
|
||
def clear_error(self): | ||
""" | ||
Removes any errors that were stored from previous background refreshes. | ||
""" | ||
with self._lock: | ||
if self._worker: | ||
self._worker._error_info = None | ||
|
||
|
||
class RefreshThread(threading.Thread): | ||
""" | ||
Thread that refreshes credentials. | ||
""" | ||
|
||
def __init__(self, cred, request, **kwargs): | ||
"""Initializes the thread. | ||
|
||
Args: | ||
cred: A Credential object to refresh. | ||
request: A Request object used to perform a credential refresh. | ||
**kwargs: Additional keyword arguments. | ||
""" | ||
|
||
super().__init__(**kwargs) | ||
self._cred = cred | ||
self._request = request | ||
self._error_info = None | ||
|
||
def run(self): | ||
""" | ||
Perform the credential refresh. | ||
""" | ||
try: | ||
self._cred.refresh(self._request) | ||
except Exception as err: # pragma: NO COVER | ||
_LOGGER.error(f"Background refresh failed due to: {err}") | ||
self._error_info = err |
This file contains 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
This file contains 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
This file contains 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
Binary file not shown.
This file contains 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
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is
request
immutable or can we create a copy here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.