Feature/skill manager error checks (#1981)

* Attempt to create skill directory if not existing

* Handle missing priority skills

* Minor update of comments

* Handle skill load exception

Make sure an exception while trying to load/reload skill doesn't shutdown thread.

* Handle MsmException during SkillManager creation

If SkillManager can't be created due to an MsmException wait for network connection and retry.

* Update immediately if skill install file is missing

Missing skill install file indicates that this is a new venv and the requirements of the skills will need to be reinstalled.

* Add basic test for skill_manager

Basically only creating the skill_manager but it ensures that msm can be used on all supported python versions

mycroft/skills/__main__.py

@@ -25,8 +25,9 @@ from mycroft.util import (
     connected, wait_while_speaking, reset_sigint_handler,
     create_echo_function, create_daemon, wait_for_exit_signal
 )
+from mycroft.util.log import LOG
 
-from .skill_manager import SkillManager
+from .skill_manager import SkillManager, MsmException
 from .core import FallbackSkill
 from .event_scheduler import EventScheduler
 from .intent_service import IntentService
@@ -68,9 +69,20 @@ def _starting_up():
     event_scheduler = EventScheduler(bus)
 
     # Create a thread that monitors the loaded skills, looking for updates
-    skill_manager = SkillManager(bus)
+    try:
+        skill_manager = SkillManager(bus)
+    except MsmException:
+        # skill manager couldn't be created, wait for network connection and
+        # retry
+        LOG.info('Msm is uninitialized and requires network connection',
+                 'to fetch skill information\n'
+                 'Waiting for network connection...')
+        while not connected():
+            time.sleep(30)
+        skill_manager = SkillManager(bus)
+
     skill_manager.daemon = True
-    # Wait until skills have been loaded once before starting to check
+    # Wait until priority skills have been loaded before checking
     # network connection
     skill_manager.load_priority()
     skill_manager.start()

mycroft/skills/skill_manager.py

@@ -101,7 +101,9 @@ class SkillManager(Thread):
         self.update_interval = Configuration.get()['skills']['update_interval']
         self.update_interval = int(self.update_interval * 60 * MINUTES)
         self.dot_msm = join(self.msm.skills_dir, '.msm')
-        if exists(self.dot_msm):
+        # Update immediately if the .msm or installed skills file is missing
+        # otherwise according to timestamp on .msm
+        if exists(self.dot_msm) and exists(self.installed_skills_file):
             self.next_download = os.path.getmtime(self.dot_msm) + \
                                  self.update_interval
         else:
@@ -135,6 +137,10 @@ class SkillManager(Thread):
         repo_config = msm_config['repo']
         data_dir = expanduser(config['data_dir'])
         skills_dir = join(data_dir, msm_config['directory'])
+        # Try to create the skills directory if it doesn't exist
+        if not exists(skills_dir):
+            os.makedirs(skills_dir)
+
         repo_cache = join(data_dir, repo_config['cache'])
         platform = config['enclosure'].get('platform', 'default')
         return MycroftSkillsManager(
@@ -245,12 +251,15 @@ class SkillManager(Thread):
             data = {'utterance': dialog.get("skills updated")}
             self.bus.emit(Message("speak", data))
 
+        # Schedule retry in 5 minutes on failure, after 10 shorter periods
+        # Go back to 60 minutes wait
         if default_skill_errored and self.num_install_retries < 10:
             self.num_install_retries += 1
             self.next_download = time.time() + 5 * MINUTES
             return False
         self.num_install_retries = 0
 
+        # Update timestamp on .msm file to be used when system is restarted
         with open(self.dot_msm, 'a'):
             os.utime(self.dot_msm, None)
         self.next_download = time.time() + self.update_interval
@@ -285,10 +294,7 @@ class SkillManager(Thread):
         """
         skill_path = skill_path.rstrip('/')
         skill = self.loaded_skills.setdefault(skill_path, {})
-        skill.update({
+        skill.update({"id": basename(skill_path), "path": skill_path})
-            "id": basename(skill_path),
-            "path": skill_path
-        })
 
         # check if folder is a skill (must have __init__.py)
         if not MainModule + ".py" in os.listdir(skill_path):
@@ -354,15 +360,19 @@ class SkillManager(Thread):
     def load_priority(self):
         skills = {skill.name: skill for skill in self.msm.list()}
         for skill_name in PRIORITY_SKILLS:
-            skill = skills[skill_name]
+            skill = skills.get(skill_name)
-            if not skill.is_local:
+            if skill:
-                try:
+                if not skill.is_local:
-                    skill.install()
+                    try:
-                except Exception:
+                        skill.install()
-                    LOG.exception('Downloading priority skill:' + skill.name)
+                    except Exception:
-                    if not skill.is_local:
+                        LOG.exception('Downloading priority skill: '
-                        continue
+                                      '{} failed'.format(skill.name))
-            self._load_or_reload_skill(skill.path)
+                        if not skill.is_local:
+                            continue
+                self._load_or_reload_skill(skill.path)
+            else:
+                LOG.error('Priority skill {} can\'t be found')
 
     def remove_git_locks(self):
         """If git gets killed from an abrupt shutdown it leaves lock files"""
@@ -392,10 +402,14 @@ class SkillManager(Thread):
             skill_paths = glob(join(self.msm.skills_dir, '*/'))
             still_loading = False
             for skill_path in skill_paths:
-                still_loading = (
+                try:
-                        self._load_or_reload_skill(skill_path) or
+                    still_loading = (
-                        still_loading
+                            self._load_or_reload_skill(skill_path) or
-                )
+                            still_loading
+                    )
+                except Exception as e:
+                    LOG.error('(Re)loading of {} failed ({})'.format(
+                        skill_path, repr(e)))
             if not has_loaded and not still_loading and len(skill_paths) > 0:
                 has_loaded = True
                 self.bus.emit(Message('mycroft.skills.initialized'))

test/unittests/skills/test_skill_manager.py

@@ -0,0 +1,69 @@
+import unittest
+import mock
+import tempfile
+from os.path import exists, join
+from shutil import rmtree
+from test.util import base_config
+
+from mycroft.configuration import Configuration
+from mycroft.skills.skill_manager import SkillManager
+
+BASE_CONF = base_config()
+BASE_CONF['data_dir'] = tempfile.mkdtemp()
+BASE_CONF['skills'] = {
+    'msm': {
+        'directory': 'skills',
+        'versioned': True,
+        'repo': {
+            'cache': '.skills-repo',
+            'url': 'https://github.com/MycroftAI/mycroft-skills',
+            'branch': '18.08'
+        }
+    },
+    'update_interval': 3600,
+    'auto_update': False,
+    'blacklisted_skills': [],
+    'priority_skills': ["mycroft-pairing"]
+}
+
+
+class MockEmitter(object):
+    def __init__(self):
+        self.reset()
+
+    def emit(self, message):
+        self.types.append(message.type)
+        self.results.append(message.data)
+
+    def get_types(self):
+        return self.types
+
+    def get_results(self):
+        return self.results
+
+    def on(self, event, f):
+        pass
+
+    def reset(self):
+        self.types = []
+        self.results = []
+
+
+class MycroftSkillTest(unittest.TestCase):
+    emitter = MockEmitter()
+
+    def setUp(self):
+        self.emitter.reset()
+        self.temp_dir = tempfile.mkdtemp()
+
+    @mock.patch.dict(Configuration._Configuration__config, BASE_CONF)
+    def test_create_manager(self):
+        """ Verify that the skill manager and msm loads as expected and
+            that the skills dir is created as needed.
+        """
+        SkillManager(self.emitter)
+        self.assertTrue(exists(join(BASE_CONF['data_dir'], 'skills')))
+
+    @classmethod
+    def tearDownClass(cls):
+        rmtree(BASE_CONF['data_dir'])