From 2668643b7570db96651466250e7a496184f6ef0a Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Tue, 29 Nov 2022 15:01:43 -0500 Subject: [PATCH] 19686: api constructor returns ThreadSafeApiCache This is an API-compatible wrapper object that provides thread safety. Returning this from api() helps keep users out of trouble. The changes to ThreadSafeApiCache are required to keep it API-compatible with the original and keep tests passing. Assignments to the request_id attribute need to be used for all future requests. It's a little unclear if this is an intended API or just test scaffolding, but it's not too difficult to keep working so I just did that. Arvados-DCO-1.1-Signed-off-by: Brett Smith --- sdk/python/arvados/api.py | 33 +++++++++++++++++++++------------ sdk/python/arvados/safeapi.py | 12 +++++++++--- sdk/python/tests/test_api.py | 21 +++++++++++++++++++++ 3 files changed, 51 insertions(+), 15 deletions(-) diff --git a/sdk/python/arvados/api.py b/sdk/python/arvados/api.py index 13dd8e9af8..19154f3e8b 100644 --- a/sdk/python/arvados/api.py +++ b/sdk/python/arvados/api.py @@ -374,6 +374,11 @@ def api(version=None, cache=True, host=None, token=None, insecure=False, like you would write in user configuration; or pass additional arguments for lower-level control over the client. + This function returns a `arvados.safeapi.ThreadSafeApiCache`, an + API-compatible wrapper around `googleapiclient.discovery.Resource`. If + you're handling concurrency yourself and/or your application is very + performance-sensitive, consider calling `api_client` directly. + Arguments: version: str | None @@ -398,21 +403,20 @@ def api(version=None, cache=True, host=None, token=None, insecure=False, Other arguments are passed directly to `api_client`. See that function's docstring for more information about their meaning. """ - if discoveryServiceUrl or host or token: - # We pass `insecure` here for symmetry with `api_kwargs_from_config`. - client_kwargs = normalize_api_kwargs( - version, discoveryServiceUrl, host, token, - insecure=insecure, - ) - else: - client_kwargs = api_kwargs_from_config(version) - return api_client( - **client_kwargs, + kwargs.update( cache=cache, + insecure=insecure, request_id=request_id, timeout=timeout, - **kwargs, ) + if discoveryServiceUrl or host or token: + kwargs.update(normalize_api_kwargs(version, discoveryServiceUrl, host, token)) + else: + kwargs.update(api_kwargs_from_config(version)) + version = kwargs.pop('version') + # We do the import here to avoid a circular import at the top level. + from .safeapi import ThreadSafeApiCache + return ThreadSafeApiCache({}, {}, kwargs, version) def api_from_config(version=None, apiconfig=None, **kwargs): """Build an Arvados API client from a configuration mapping @@ -421,6 +425,11 @@ def api_from_config(version=None, apiconfig=None, **kwargs): configuration. It accepts that mapping as an argument, so you can use a configuration that's different from what the user has set up. + This function returns a `arvados.safeapi.ThreadSafeApiCache`, an + API-compatible wrapper around `googleapiclient.discovery.Resource`. If + you're handling concurrency yourself and/or your application is very + performance-sensitive, consider calling `api_client` directly. + Arguments: version: str | None @@ -435,4 +444,4 @@ def api_from_config(version=None, apiconfig=None, **kwargs): Other arguments are passed directly to `api_client`. See that function's docstring for more information about their meaning. """ - return api_client(**api_kwargs_from_config(version, apiconfig, **kwargs)) + return api(**api_kwargs_from_config(version, apiconfig, **kwargs)) diff --git a/sdk/python/arvados/safeapi.py b/sdk/python/arvados/safeapi.py index 0513c7023f..e9dde19625 100644 --- a/sdk/python/arvados/safeapi.py +++ b/sdk/python/arvados/safeapi.py @@ -15,6 +15,7 @@ import threading from . import api from . import config from . import keep +from . import util class ThreadSafeApiCache(object): """Thread-safe wrapper for an Arvados API client @@ -53,13 +54,18 @@ class ThreadSafeApiCache(object): else: self._api_kwargs = api.normalize_api_kwargs(version, **api_params) self.api_token = self._api_kwargs['token'] + self.request_id = self._api_kwargs.get('request_id') self.local = threading.local() self.keep = keep.KeepClient(api_client=self, **keep_params) def localapi(self): - if 'api' not in self.local.__dict__: - self.local.api = api.api_client(**self._api_kwargs) - return self.local.api + try: + client = self.local.api + except AttributeError: + client = api.api_client(**self._api_kwargs) + client._http._request_id = lambda: self.request_id or util.new_request_id() + self.local.api = client + return client def __getattr__(self, name): # Proxy nonexistent attributes to the thread-local API client. diff --git a/sdk/python/tests/test_api.py b/sdk/python/tests/test_api.py index c249f46d3c..03af8ce5ee 100644 --- a/sdk/python/tests/test_api.py +++ b/sdk/python/tests/test_api.py @@ -139,6 +139,27 @@ class ArvadosApiTest(run_test_server.TestCaseWithServers): result = api.humans().get(uuid='test').execute() self.assertEqual(string.hexdigits, ''.join(list(result.keys()))) + def test_api_is_threadsafe(self): + api_kwargs = { + 'host': os.environ['ARVADOS_API_HOST'], + 'token': os.environ['ARVADOS_API_TOKEN'], + 'insecure': True, + } + config_kwargs = {'apiconfig': os.environ} + for api_constructor, kwargs in [ + (arvados.api, {}), + (arvados.api, api_kwargs), + (arvados.api_from_config, {}), + (arvados.api_from_config, config_kwargs), + ]: + sub_kwargs = "kwargs" if kwargs else "no kwargs" + with self.subTest(f"{api_constructor.__name__} with {sub_kwargs}"): + api_client = api_constructor('v1', **kwargs) + self.assertTrue(hasattr(api_client, 'localapi'), + f"client missing localapi method") + self.assertTrue(hasattr(api_client, 'keep'), + f"client missing keep attribute") + class RetryREST(unittest.TestCase): def setUp(self): -- 2.39.5