From 759ec42dbdc010bca25b652dd8be87f80d4624a9 Mon Sep 17 00:00:00 2001 From: Daniel McKnight Date: Wed, 12 Jul 2023 09:43:47 -0700 Subject: [PATCH] Add locking around skill settings changes Add test case to reproduce/resolve #93 Possibly related to #91 --- ovos_workshop/skills/base.py | 32 ++++++++++-------- test/unittests/skills/test_base.py | 52 ++++++++++++++++++++++++++++-- 2 files changed, 68 insertions(+), 16 deletions(-) diff --git a/ovos_workshop/skills/base.py b/ovos_workshop/skills/base.py index 91d0079e..7d83050b 100644 --- a/ovos_workshop/skills/base.py +++ b/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() diff --git a/test/unittests/skills/test_base.py b/test/unittests/skills/test_base.py index 38f19d64..06a61b41 100644 --- a/test/unittests/skills/test_base.py +++ b/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