search

MycroftAI / mycroft-core

Mycroft Core, the Mycroft Artificial Intelligence platform.
https://mycroft.ai
Apache License 2.0
6.53k stars 1.27k forks source link

stop handling #2916

Closed JarbasAl closed 2 months ago

JarbasAl commented 3 years ago

in MycroftSkill the following assumption is made # The self.stop() call took more than 100ms, assume it handled Stop

I personally disagree with this, either stop was handled or it wasnt, no unsafe assumptions, HolmesV removed the timeout, but maybe it would be better to actually raise an exception on timeout?

additionally when an exception is raised in initialize the shutdown/stop methods are called, and may throw yet another exception, since the skill did not finish loading it is expected these calls will often fail

proposed fix https://github.com/HelloChatterbox/HolmesV/pull/51/files

krisgesling commented 3 years ago

It doesn't seem right does it...

Looks like it got introduced here: https://github.com/MycroftAI/mycroft-core/commit/17aab53fae44aa36111a4e1b592734821cac7dd1 and given the commit message, I assume it was to handle some kind of issue when using the Mark 1 button to stop things eg an expired timer. Have you done any testing for that particular scenario?

forslund commented 2 months ago

Closing Issue since we're archiving the repo