Add locking around skill settings changes

Add test case to reproduce/resolve #93
Possibly related to #91

ovos_workshop/skills/base.py

@@ -23,7 +23,7 @@
 from inspect import signature
 from itertools import chain
 from os.path import join, abspath, dirname, basename, isfile
-from threading import Event
+from threading import Event, RLock
 from typing import List, Optional, Dict, Callable, Union
 
 from ovos_bus_client import MessageBusClient
@@ -182,6 +182,7 @@ def __init__(self, name: Optional[str] = None,
         self._settings = None
         self._initial_settings = settings or dict()
         self._settings_watchdog = None
+        self._settings_lock = RLock()
 
         # Override to register a callback method that will be called every time
         # the skill's settings are updated. The referenced method should
@@ -319,9 +320,10 @@ def settings(self, val: dict):
         if self._settings is None:
             self._initial_settings = val
             return
-        # ensure self._settings remains a JsonDatabase
-        self._settings.clear()  # clear data
-        self._settings.merge(val, skip_empty=False)  # merge new data
+        with self._settings_lock:
+            # ensure self._settings remains a JsonDatabase
+            self._settings.clear()  # clear data
+            self._settings.merge(val, skip_empty=False)  # merge new data
 
     # not a property in mycroft-core
     @property
@@ -616,15 +618,16 @@ def _init_settings(self):
         # NOTE: lock is disabled due to usage of deepcopy and to allow json
         # serialization
         self._settings = JsonStorage(self._settings_path, disable_lock=True)
-        if self._initial_settings and not self._is_fully_initialized:
-            self.log.warning("Copying default settings values defined in "
-                             "__init__ \nto correct this add kwargs "
-                             "__init__(bus=None, skill_id='') "
-                             f"to skill class {self.__class__.__name__}")
-            for k, v in self._initial_settings.items():
-                if k not in self._settings:
-                    self._settings[k] = v
-        self._initial_settings = copy(self.settings)
+        with self._settings_lock:
+            if self._initial_settings and not self._is_fully_initialized:
+                self.log.warning("Copying default settings values defined in "
+                                 "__init__ \nto correct this add kwargs "
+                                 "__init__(bus=None, skill_id='') "
+                                 f"to skill class {self.__class__.__name__}")
+                for k, v in self._initial_settings.items():
+                    if k not in self._settings:
+                        self._settings[k] = v
+            self._initial_settings = copy(self.settings)
 
         self._start_filewatcher()
 
@@ -677,7 +680,8 @@ def _handle_settings_file_change(self, path: str):
         @param path: Modified file path
         """
         if self._settings:
-            self._settings.reload()
+            with self._settings_lock:
+                self._settings.reload()
         if self.settings_change_callback:
             try:
                 self.settings_change_callback()

test/unittests/skills/test_base.py

@@ -1,6 +1,10 @@
+import os
+import shutil
 import unittest
 
 from logging import Logger
+from threading import Event, Thread
+from time import time
 from unittest.mock import Mock, patch
 from os.path import join, dirname, isdir
 
@@ -19,11 +23,18 @@ def test_simple_trace(self):
 
 
 class TestBaseSkill(unittest.TestCase):
+    test_config_path = join(dirname(__file__), "temp_config")
+    os.environ["XDG_CONFIG_HOME"] = test_config_path
     from ovos_workshop.skills.base import BaseSkill
     bus = FakeBus()
     skill_id = "test_base_skill"
     skill = BaseSkill(bus=bus, skill_id=skill_id)
 
+    @classmethod
+    def tearDownClass(cls) -> None:
+        os.environ.pop("XDG_CONFIG_HOME")
+        shutil.rmtree(cls.test_config_path)
+
     def test_00_skill_init(self):
         from ovos_workshop.settings import SkillSettingsManager
         from ovos_workshop.skills.base import SkillGUI
@@ -90,8 +101,45 @@ def test_startup(self):
         pass
 
     def test_init_settings(self):
-        # TODO
-        pass
+        # Test initial settings defined and not fully initialized
+        test_settings = {"init": True}
+        self.skill._initial_settings = test_settings
+        self.skill._settings["init"] = False
+        self.skill._settings["test"] = "value"
+        self.skill._init_event.clear()
+        self.skill._init_settings()
+        self.assertEqual(dict(self.skill.settings),
+                         {**test_settings,
+                          **{"__mycroft_skill_firstrun": False}})
+        self.assertEqual(dict(self.skill._initial_settings),
+                         dict(self.skill.settings))
+
+        # Test settings changed during init
+        stop_event = Event()
+        setting_event = Event()
+
+        def _update_skill_settings():
+            while not stop_event.is_set():
+                self.skill.settings["test_val"] = time()
+                setting_event.set()
+
+        # Test this a few times since this handles a race condition
+        for i in range(8):
+            setting_event.clear()
+            stop_event.clear()
+            thread = Thread(target=_update_skill_settings, daemon=True)
+            thread.start()
+            setting_event.wait()  # settings have some value
+            self.skill._init_settings()
+            setting_event.clear()
+            setting_event.wait()  # settings updated since init
+            stop_time = time()
+            stop_event.set()
+            thread.join()
+            self.assertAlmostEquals(self.skill.settings["test_val"], stop_time,
+                                    0)
+            self.assertNotEqual(self.skill.settings["test_val"],
+                                self.skill._initial_settings["test_val"])
 
     def test_init_skill_gui(self):
         # TODO