Merge pull request #5294 from jestabro/configtree-thread-safe-init

T9015: fix thread safety of configtree read/write_cache

2 files changed, 52 insertions, 3 deletions

diff --git a/python/vyos/configtree.py b/python/vyos/configtree.py
index 24b3252fd..a70aac095 100644
--- a/python/vyos/configtree.py
+++ b/python/vyos/configtree.py
@@ -72,7 +72,7 @@ class ConfigTreeError(Exception):
 
 
 class ConfigTree:
-    # pylint: disable=too-many-instance-attributes,too-many-arguments,too-many-statements,too-many-public-methods
+    # pylint: disable=too-many-instance-attributes,too-many-arguments,too-many-statements,too-many-public-methods,raise-missing-from
     def __init__(
         self,
         config_string=None,
@@ -232,7 +232,11 @@ class ConfigTree:
                 self.__config = address
                 self.__version = ''
             case ('internal', internal):
-                config = self.__read_internal(internal.encode())
+                try:
+                    cache_string = Path(internal).read_bytes()
+                except OSError as e:
+                    raise ValueError(f'Failed to read internal cache file: {e}')
+                config = self.__read_internal_string(cache_string)
                 if config is None:
                     msg = self.__get_error().decode()
                     raise ValueError(
@@ -299,7 +303,11 @@ class ConfigTree:
         return self.__version
 
     def write_cache(self, file_name):
-        self.__write_internal(self.get_tree(), file_name.encode())
+        cache_string = self.__write_internal_string(self.get_tree())
+        try:
+            Path(file_name).write_bytes(cache_string)
+        except OSError as e:
+            raise ValueError(f'Failed to write internal cache file: {e}')
 
     def write_internal_string(self) -> str:
         res = self.__write_internal_string(self.get_tree())
diff --git a/src/tests/test_config_tree.py b/src/tests/test_config_tree.py
index d6339c570..ba25537ed 100644
--- a/src/tests/test_config_tree.py
+++ b/src/tests/test_config_tree.py
@@ -14,7 +14,10 @@
 #
 #
 
+import os
 import json
+import time
+import threading
 import unittest
 from unittest import TestCase
 
@@ -29,6 +32,20 @@ class TestInitialSetup(TestCase):
             config_str = f.read()
             self.ct = ConfigTree(config_str)
 
+        self.thread_exception = None
+        self.orig_excepthook = threading.excepthook
+
+        def custom_hook(args):
+            self.thread_exception = args.exc_value
+
+        threading.excepthook = custom_hook
+
+    def tearDown(self):
+        threading.excepthook = self.orig_excepthook
+
+        if self.thread_exception:
+            raise self.thread_exception
+
     def test_subtree_from_partial(self):
         reftree = ReferenceTree(cache_file='data/reftree.cache')
 
@@ -43,6 +60,30 @@ class TestInitialSetup(TestCase):
 
         self.assertEqual(self.ct, reassemble)
 
+    def test_cache_io(self):
+        config_cache = '/tmp/config.cache'
+
+        self.ct.write_cache(config_cache)
+        ct_cached = ConfigTree(internal=config_cache)
+        os.unlink(config_cache)
+
+        self.assertEqual(self.ct, ct_cached)
+
+    def test_cache_io_thread(self):
+        config_cache = '/tmp/config.cache'
+
+        def task():
+            time.sleep(0.1)
+            self.ct.write_cache(config_cache)
+            ct_cached = ConfigTree(internal=config_cache)
+            os.unlink(config_cache)
+
+            self.assertEqual(self.ct, ct_cached)
+
+        t = threading.Thread(target=task)
+        t.start()
+        t.join()
+
 
 if __name__ == '__main__':
     unittest.main()