[gerrit_util] Cache resolved Authenticator as a class variable.
I noticed that the Authenticator is resolved maybe 5 or 6 times per
git-cl invocation. This should lead to more consistent behavior and
will likely be a bit faster, especially for SSOAuthenticator and
LuciAuthAuthenticator which involve subprocess invocations.
R=ayatane@chromium.org
Bug: 336351842
Change-Id: Id6c2873a6960a171305560acb98afe2c4f397295
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5589865
Auto-Submit: Robbie Iannucci <iannucci@chromium.org>
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
Commit-Queue: Robbie Iannucci <iannucci@chromium.org>
diff --git a/gerrit_util.py b/gerrit_util.py
index aadcb6d..4e806df 100644
--- a/gerrit_util.py
+++ b/gerrit_util.py
@@ -174,6 +174,10 @@
class Authenticator(object):
"""Base authenticator class for authenticator implementations to subclass."""
+ # Cached Authenticator subclass instance, resolved via get().
+ _resolved: Optional[Authenticator] = None
+ _resolved_lock = threading.Lock()
+
def authenticate(self, conn: HttpConn):
"""Adds authentication information to the HttpConn."""
raise NotImplementedError()
@@ -193,47 +197,54 @@
environment."""
raise NotImplementedError()
- @staticmethod
- def get():
+ @classmethod
+ def get(cls):
"""Returns: (Authenticator) The identified Authenticator to use.
Probes the local system and its environment and identifies the
Authenticator instance to use.
+
+ The resolved Authenticator instance is cached as a class variable.
"""
- use_new_auth = newauth.Enabled()
+ with cls._resolved_lock:
+ if ret := cls._resolved:
+ return ret
- # Allow skipping SSOAuthenticator for local testing purposes.
- skip_sso = newauth.SkipSSO()
+ use_new_auth = newauth.Enabled()
- authenticators: List[Type[Authenticator]]
+ # Allow skipping SSOAuthenticator for local testing purposes.
+ skip_sso = newauth.SkipSSO()
- if use_new_auth:
- LOGGER.debug('Authenticator.get: using new auth stack.')
- authenticators = [
- SSOAuthenticator,
- LuciContextAuthenticator,
- GceAuthenticator,
- LuciAuthAuthenticator,
- ]
- if skip_sso:
- LOGGER.debug('Authenticator.get: skipping SSOAuthenticator.')
- authenticators = authenticators[1:]
- else:
- authenticators = [
- LuciContextAuthenticator,
- GceAuthenticator,
- CookiesAuthenticator,
- ]
+ if use_new_auth:
+ LOGGER.debug('Authenticator.get: using new auth stack.')
+ authenticators = [
+ SSOAuthenticator,
+ LuciContextAuthenticator,
+ GceAuthenticator,
+ LuciAuthAuthenticator,
+ ]
+ if skip_sso:
+ LOGGER.debug('Authenticator.get: skipping SSOAuthenticator.')
+ authenticators = authenticators[1:]
+ else:
+ authenticators = [
+ LuciContextAuthenticator,
+ GceAuthenticator,
+ CookiesAuthenticator,
+ ]
- for candidate in authenticators:
- if candidate.is_applicable():
- LOGGER.debug('Authenticator.get: Selected %s.',
- candidate.__name__)
- return candidate()
+ for candidate in authenticators:
+ if candidate.is_applicable():
+ LOGGER.debug('Authenticator.get: Selected %s.',
+ candidate.__name__)
+ ret = candidate()
+ cls._resolved = ret
+ return ret
- auth_names = ', '.join(a.__name__ for a in authenticators)
- raise ValueError(
- f"Could not find suitable authenticator, tried: [{auth_names}].")
+ auth_names = ', '.join(a.__name__ for a in authenticators)
+ raise ValueError(
+ f"Could not find suitable authenticator, tried: [{auth_names}]."
+ )
class SSOAuthenticator(Authenticator):
diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py
index c7c4721..676c9f6 100755
--- a/tests/git_cl_test.py
+++ b/tests/git_cl_test.py
@@ -669,6 +669,7 @@
# It's important to reset settings to not have inter-tests interference.
git_cl.settings = git_cl.Settings()
self.addCleanup(mock.patch.stopall)
+ gerrit_util.Authenticator._resolved = None
def tearDown(self):
try:
@@ -3623,6 +3624,7 @@
self.mockGit = GitMocks()
mock.patch('scm.GIT.GetConfig', self.mockGit.GetConfig).start()
mock.patch('scm.GIT.SetConfig', self.mockGit.SetConfig).start()
+ gerrit_util.Authenticator._resolved = None
def testRunHook(self):
expected_results = {
