MycroftAI / mycroft-core

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

Register bus handlers requiring loop after loop exists #2926

Closed krisgesling closed 1 month ago

krisgesling commented 3 years ago

Description

AttributeError was being thrown by the Speech service.

File "/opt/mycroft/.venv/lib/python3.8/site-packages/pyee/_base.py", line 116, in emit
    self._emit_handle_potential_error(event, args[0] if args else None)
  File "/opt/mycroft/.venv/lib/python3.8/site-packages/pyee/_base.py", line 86, in _emit_handle_potential_error
    raise error
  File "/usr/lib/python3.8/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/opt/mycroft/mycroft/client/speech/__main__.py", line 140, in handle_mic_get_status
    data = {'muted': loop.is_muted()}
AttributeError: 'NoneType' object has no attribute 'is_muted'

Cause: The main bus gets established and event handlers registered before the RecognizerLoop bus is established. Hence if something emits a mycroft.mic.get_status message in this short window, the handler will fail as loop is None.

Fix: This change splits the bus events being registered into two groups:

  1. events required for setup - currently only open and based on the existing #TODO this might get removed as a breaking change.
  2. the rest of the events that interact with the RecognizerLoop or expect the speech service to be ready. These are now only registered after the loop has been initialized and run.

How to test

I haven't created a way to reliably reproduce the issue and a unit test for this is proving difficult. Any suggestions welcome.

Would need to emit a message like mycroft.mic.get_status as the service is being established and verify that the handler is not called. Then once the service is established, re-emit the message and assert that the handler is called.

Contributor license agreement signed?

@AIIX - the only thing calling this that I could see is mycroft-gui-mark-2. It's only going to not receive a response, and it's already doing that with the handler failing, but wanted to flag it in case this will impact that package in any way.

devops-mycroft commented 3 years ago

Voight Kampff Integration Test Succeeded (Results)

AIIX commented 3 years ago

It seems to affect the Mute button delegate in the drop down menu in the mycroft-gui-mark-2 package,I believe if mycroft.mic.get_status does not respond on boot we might have no way to know what the actual status is of the microphone in the GUI.

ken-mycroft commented 3 years ago

My confusion arises from the fact that the response to the "mycroft.volume.get" message contains a muted flag and I was wondering why this is not being used?

krisgesling commented 3 years ago

My confusion arises from the fact that the response to the "mycroft.volume.get" message contains a muted flag and I was wondering why this is not being used?

I think we're talking about different things. mycroft.volume.get relates to the audio output whilst mycroft.mic.get_status relates to the audio input.

krisgesling commented 3 years ago

It seems to affect the Mute button delegate in the drop down menu in the mycroft-gui-mark-2 package,I believe if mycroft.mic.get_status does not respond on boot we might have no way to know what the actual status is of the microphone in the GUI.

@AIIX - presently the response wouldn't be emitted as it would just fail with the error above. So this PR shouldn't make that behavior worse.

I'm thinking we probably need to first wait for the Speech Service to be ready before checking the mic_status

forslund commented 1 month ago

Closing PR since we're archiving the repo