Repository: dwavesystems/dwave-cloud-client
Pull request #642 (2024-07-24 21:53:17)

Improve `@cached.ondisk` resilience to forking/high concurrency
Test/improve forking/concurrency resilience from https://github.com/dwavesystems/dwave-cloud-client/issues/641.
Mitigate race condition from https://github.com/dwavesystems/homebase/issues/37.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/dwavesystems/dwave-cloud-client/pull/642?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dwavesystems) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 88.43%. Comparing base [(`cc88c81`)](https://app.codecov.io/gh/dwavesystems/dwave-cloud-client/commit/cc88c81ec7e262664c879cf5d1a85c44616da005?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dwavesystems) to head [(`68cbff9`)](https://app.codecov.io/gh/dwavesystems/dwave-cloud-client/commit/68cbff92ae4bba24b22618c8c53f253549c5f58f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dwavesystems).
> Report is 7 commits behind head on master.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #642      +/-   ##
==========================================
- Coverage   88.64%   88.43%   -0.22%     
==========================================
  Files          42       42              
  Lines        4580     4583       +3     
==========================================
- Hits         4060     4053       -7     
- Misses        520      530      +10     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/dwavesystems/dwave-cloud-client/pull/642?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dwavesystems).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dwavesystems).


Patch:
diff --git a/dwave/cloud/config/loaders.py b/dwave/cloud/config/loaders.py
index 1947d93..3ae2349 100644
--- a/dwave/cloud/config/loaders.py
+++ b/dwave/cloud/config/loaders.py
@@ -238,13 +238,19 @@ def get_default_configfile_path(**kwargs) -> str:
     return get_configfile_paths(**kwargs)[0]
 
 
-def get_cache_dir() -> str:
+def get_cache_dir(create: bool = False) -> str:
     """Return a directory path convenient for storing user-local,
     package-local and version-specific cache data.
     """
-    return homebase.user_cache_dir(
+    path = homebase.user_cache_dir(
         app_name=__packagename__, app_author=CONF_AUTHOR,
-        version=__version__, use_virtualenv=False, create=True)
+        version=__version__, use_virtualenv=False, create=False)
+
+    # avoid possible race condition on create (https://github.com/dwavesystems/homebase/issues/37)
+    if create:
+        os.makedirs(path, exist_ok=True)
+
+    return path
 
 
 def load_config_from_files(filenames=None):
diff --git a/dwave/cloud/solver.py b/dwave/cloud/solver.py
index f2e2264..8240b92 100644
--- a/dwave/cloud/solver.py
+++ b/dwave/cloud/solver.py
@@ -381,7 +381,7 @@ class BaseUnstructuredSolver(BaseSolver):
         raise NotImplementedError
 
     def upload_problem(self, problem, **kwargs):
-        """Encode and upload the problem.
+        r"""Encode and upload the problem.
 
         Args:
             problem (model-like):
@@ -663,7 +663,7 @@ class DQMSolver(BaseUnstructuredSolver):
         return self.sample_problem(dqm, label=label, **params)
 
     def upload_dqm(self, dqm):
-        """Upload the specified :term:`DQM` to SAPI, returning a Problem ID
+        r"""Upload the specified :term:`DQM` to SAPI, returning a Problem ID
         that can be used to submit the DQM to this solver (i.e. call the
         `.sample_dqm` method).
 
@@ -751,7 +751,7 @@ class CQMSolver(BaseUnstructuredSolver):
         return self.sample_problem(cqm, label=label, **params)
 
     def upload_cqm(self, cqm):
-        """Upload the specified :term:`CQM` to SAPI, returning a Problem ID
+        r"""Upload the specified :term:`CQM` to SAPI, returning a Problem ID
         that can be used to submit the CQM to this solver (i.e. call the
         `.sample_cqm` method).
 
@@ -892,7 +892,7 @@ class NLSolver(BaseUnstructuredSolver):
                    model: typing.Union['dwave.optimization.Model', io.IOBase, bytes],
                    **upload_params,
                    ) -> concurrent.futures.Future:
-        """Upload the specified :term:`NL model` to SAPI, returning a Problem ID
+        r"""Upload the specified :term:`NL model` to SAPI, returning a Problem ID
         that can be used to submit the NL model to this solver (i.e. call the
         :meth:`.sample_nlm` method).
 
diff --git a/dwave/cloud/utils/decorators.py b/dwave/cloud/utils/decorators.py
index 7181e1a..d65ae5b 100644
--- a/dwave/cloud/utils/decorators.py
+++ b/dwave/cloud/utils/decorators.py
@@ -27,7 +27,7 @@ import warnings
 
 from functools import wraps
 from secrets import token_hex
-from typing import Any, Callable, Dict, List, Mapping, Optional, Sequence
+from typing import Any, Callable, Dict, List, Mapping, Optional, Sequence, Union
 from unittest import mock
 
 from dwave.cloud.utils.time import epochnow
@@ -224,7 +224,7 @@ class cached:
 
     def __init__(self, *,
                  maxage: Optional[float] = None,
-                 store: Optional[Mapping] = None,
+                 store: Union[Mapping, Callable[[], Mapping], None] = None,
                  key: Optional[str] = None,
                  bucket: Optional[str] = None):
 
@@ -232,6 +232,8 @@ class cached:
 
         if store is None:
             store = {}
+        if callable(store):
+            store = store()
         self.store = store
 
         self.key = key
@@ -282,15 +284,21 @@ class cached:
     def ondisk(cls, **kwargs):
         """@cached backed by an on-disk sqlite3-based cache."""
 
-        # lazy import for performance
-        import diskcache
+        # defer to break circular imports
         from dwave.cloud.config import get_cache_dir
 
         directory = kwargs.pop('directory', get_cache_dir())
         compression_level = kwargs.pop('compression_level', 6)
-        cache = diskcache.Cache(disk=diskcache.JSONDisk, directory=directory,
-                                disk_compress_level=compression_level)
-        return cls(store=cache, **kwargs)
+
+        # defer diskcache construction (sqlite db access!) until actually needed.
+        # also, by postponing sqlite3 connect we enable easier post-fork re-init
+        # (see https://www.sqlite.org/faq.html#q6)
+        def store_initializer():
+            import diskcache
+            return diskcache.Cache(disk=diskcache.JSONDisk, directory=directory,
+                                   disk_compress_level=compression_level)
+
+        return cls(store=store_initializer, **kwargs)
 
     class disabled:
         """Context manager and decorator that disables the cache within the
diff --git a/releasenotes/notes/lazy-disk-cache-init-e400c535079a8aed.yaml b/releasenotes/notes/lazy-disk-cache-init-e400c535079a8aed.yaml
new file mode 100644
index 0000000..d6e4f66
--- /dev/null
+++ b/releasenotes/notes/lazy-disk-cache-init-e400c535079a8aed.yaml
@@ -0,0 +1,12 @@
+---
+fixes:
+  - |
+    Defer SQLite connect in ``@cached.ondisk()`` until actually needed. Also,
+    verify cache thread/process-safety and forking support.
+  - |
+    Fix ``get_cache_dir()`` to not create the cache directory by default. Creation
+    is now optional and controlled with ``create`` argument. This makes it consistent
+    with other config path functions.
+  - |
+    Fix possible race condition during cache directory create.
+    See `homebase#37 <https://github.com/dwavesystems/homebase/issues/37>`_.


Test patch:
diff --git a/tests/test_config.py b/tests/test_config.py
index 397509d..5f4f644 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -459,7 +459,7 @@ class TestConfigUtils(unittest.TestCase):
             parse_boolean('x')
 
     def test_cache_dir(self):
-        path = get_cache_dir()
+        path = get_cache_dir(create=True)
         self.assertTrue(os.path.isdir(path))
         self.assertIn(__packagename__, path)
         self.assertIn(__version__, path)
diff --git a/tests/test_utils.py b/tests/test_utils.py
index 4529ec5..746f8a7 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -17,7 +17,11 @@ import copy
 import io
 import json
 import logging
+import os
+import sqlite3
+import subprocess
 import tempfile
+import textwrap
 import time
 import unittest
 import uuid
@@ -512,6 +516,65 @@ class TestCachedOnDiskDecorator(TestCachedCommon):
         self.assertEqual(f2(), 0)
 
 
+@unittest.skipUnless(hasattr(os, 'fork'), "os.fork() not available on this platform")
+class TestCachedForking(unittest.TestCase):
+
+    @unittest.skipUnless(sqlite3.threadsafety == 3,
+                         "sqlite3 is not compiled with serialized threading mode support")
+    def test_cache_connection_safety_post_forking(self):
+        # note: this convoluted subprocess call approach seems to be simpler
+        # than trying to kill the forked unittest suite after a forking test case.
+        program = textwrap.dedent("""
+            import os
+            from dwave.cloud.utils.decorators import cached
+
+            @cached.ondisk()
+            def f():
+                return 42
+
+            # make sure db connection is opened
+            f()
+
+            os.fork()
+
+            # make sure cache works in both parent and child
+            if f() != 42:
+                print("FAIL")
+        """)
+
+        run = subprocess.run(f"echo '{program}' | python", shell=True, capture_output=True)
+
+        self.assertEqual(run.returncode, 0)
+        self.assertEqual(run.stdout.strip(), b'')
+        self.assertEqual(run.stderr.strip(), b'')
+
+    def test_import_only_survives_forking(self):
+        # note: this convoluted subprocess call approach seems to be simpler
+        # than trying to kill the forked unittest suite after a forking test case.
+        program = textwrap.dedent("""
+            import os
+            import tempfile
+            from dwave.cloud.utils.decorators import cached
+
+            # isolate cache to avoid https://github.com/grantjenks/python-diskcache/issues/325
+            @cached.ondisk(directory=tempfile.mkdtemp())
+            def f():
+                return 42
+
+            os.fork()
+
+            # make sure cache works in both parent and child
+            if f() != 42:
+                print("FAIL")
+        """)
+
+        run = subprocess.run(f"echo '{program}' | python", shell=True, capture_output=True)
+
+        self.assertEqual(run.returncode, 0)
+        self.assertEqual(run.stdout.strip(), b'')
+        self.assertEqual(run.stderr.strip(), b'')
+
+
 class TestRetriedDecorator(unittest.TestCase):
 
     def test_func_called(self):