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 <brett.smith@curii.com>

diff --git a/sdk/python/arvados/api.py b/sdk/python/arvados/api.py
index 13dd8e9af8a9b14e1bdf655f3cd4b18b0651417c..19154f3e8b368f5b0dbeb631e4290ac7f32d101f 100644 (file)
--- 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 0513c7023f54c61e91bb976b6465f63065d869bd..e9dde196254b311bbe7387567a4080d853c7a589 100644 (file)
--- 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 c249f46d3c8d8b2352d444a8be915534bd5c2316..03af8ce5ee181e8bc95cddd724ef7c65c734131f 100644 (file)
--- 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):