Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/issue 1014 #1018

Merged
merged 2 commits into from
Aug 31, 2017
Merged

Bugfix/issue 1014 #1018

merged 2 commits into from
Aug 31, 2017

Conversation

forslund
Copy link
Collaborator

Fix trailing references causing memory leak

==== Fixed Issues ====
#1014 If not resolved atleast vastly improved

==== 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 ====
Registering event handlers should use self.add_event instead of
self.emitter.on() To make sure they are cleaned up when skill is
terminated.

==== 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.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 38.877% when pulling 34a527b on forslund:bugfix/issue-1014 into 2e479dd on MycroftAI:dev.

@forslund forslund added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Aug 31, 2017
Copy link
Contributor

@LearnedVector LearnedVector left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LearnedVector LearnedVector merged commit 995d67e into MycroftAI:dev Aug 31, 2017
@forslund forslund deleted the bugfix/issue-1014 branch January 31, 2018 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants