From 3be861cee265590cae9fccc23d3ef889974c8325 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85ke=20Forslund?= Date: Fri, 25 Aug 2017 10:38:14 +0200 Subject: [PATCH 1/2] Fix trailing references causing memory leak ==== Fixed Issues ==== ==== Tech Notes ==== This PR corrects a couple of small issues led to skills being left in memory when. - Handler for `stop.mycroft` weren't removed from event emitter when skill shut down. Now is added using `self.add_event()` - registered intent list `self.events` created a circular reference that python couldn't resolve a live so this is now deleted at shutdown - Timers in scheduled skills weren't terminated properly. Now if the timer is alive it will be joined ==== Documentation Notes ==== Registring event handlers should use `self.add_event` instead of `self.emitter.on()` To make sure they are cleaned up when skill is terminated. ==== Localization Notes ==== NONE - point to new strings, language specific functions, etc. ==== Environment Notes ==== NONE - new package requirements, new files being written to disk, etc. ==== Protocol Notes ==== NONE - message types added or changed, new signals, APIs, etc. --- mycroft/skills/core.py | 5 +++-- mycroft/skills/scheduled_skills.py | 4 ++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/mycroft/skills/core.py b/mycroft/skills/core.py index 18bd5a33a778..dc27141c95b1 100644 --- a/mycroft/skills/core.py +++ b/mycroft/skills/core.py @@ -252,7 +252,7 @@ def __register_stop(self): self.stop_time = time.time() self.stop_threshold = self.config_core.get("skills").get( 'stop_threshold') - self.emitter.on('mycroft.stop', self.__handle_stop) + self.add_event('mycroft.stop', self.__handle_stop, False) def detach(self): for (name, intent) in self.registered_intents: @@ -304,7 +304,7 @@ def _register_decorated(self): _intent_list = [] _intent_file_list = [] - def add_event(self, name, handler, need_self): + def add_event(self, name, handler, need_self=False): """ Create event handler for executing intent @@ -518,6 +518,7 @@ def shutdown(self): # removing events for e, f in self.events: self.emitter.remove(e, f) + self.events = None # Remove reference to wrappers self.emitter.emit( Message("detach_skill", {"skill_id": str(self.skill_id) + ":"})) diff --git a/mycroft/skills/scheduled_skills.py b/mycroft/skills/scheduled_skills.py index 87c07cc51668..7d56db515d08 100644 --- a/mycroft/skills/scheduled_skills.py +++ b/mycroft/skills/scheduled_skills.py @@ -116,7 +116,11 @@ def notify(self, timestamp): def shutdown(self): super(ScheduledSkill, self).shutdown() + # if timer method is running wait for it to complete self.cancel() + if self.timer and self.timer.isAlive(): + self.timer.join() + self.timer = None class ScheduledCRUDSkill(ScheduledSkill): From 34a527b1308a42be06417ed784936b461a7ecee3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=85ke=20Forslund?= Date: Fri, 25 Aug 2017 11:11:19 +0200 Subject: [PATCH 2/2] Add warning if skill isn't properly removed --- mycroft/skills/main.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/mycroft/skills/main.py b/mycroft/skills/main.py index 23b36f3b7a65..cdaa6eea2434 100644 --- a/mycroft/skills/main.py +++ b/mycroft/skills/main.py @@ -240,6 +240,14 @@ def _watch_skills(): logger.debug("Reloading Skill: " + skill_folder) # removing listeners and stopping threads skill["instance"].shutdown() + + # - 2 since there are two local references that are known + refs = sys.getrefcount(skill["instance"]) - 2 + if refs > 0: + logger.warn("After shutdown of {} there are still " + "{} references remaining. The skill " + "won't be cleaned from memory." + .format(skill['instance'].name, refs)) del skill["instance"] skill["loaded"] = True skill["instance"] = load_skill(