Skip to content
This repository was archived by the owner on Sep 8, 2024. It is now read-only.

Conversation

forslund
Copy link
Collaborator

@forslund forslund commented Aug 26, 2020

Description

self._loaded_status was accessed from a message handler, if a message is
§received at the precisely wrong time an exception may occur. This fixes
the init order.

This was reported by @j1nx with the following stack trace:

2020-02-07 15:51:09.257 | ERROR    |   337 | concurrent.futures | exception calling callback for <Future at 0x7f96356d00 state=finished raised AttributeError>
Traceback (most recent call last):
  File "/usr/lib/python3.8/concurrent/futures/_base.py", line 408, in add_done_callback
    fn(self)
  File "/usr/lib/python3.8/site-packages/pyee/_executor.py", line 60, in _callback
    self.emit('error', exc)
  File "/usr/lib/python3.8/site-packages/pyee/_base.py", line 111, in emit
    self._emit_handle_potential_error(event, args[0] if args else None)
  File "/usr/lib/python3.8/site-packages/pyee/_base.py", line 83, 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 "/usr/lib/python3.8/site-packages/mycroft/skills/skill_manager.py", line 361, in is_all_loaded
    status = {'status': self._loaded_status}
AttributeError: 'SkillManager' object has no attribute '_loaded_status'

In this case it was likely a message from mycroft gui that arrived at precisely the wrong time.

The issue is likely resolved in #2648, but if that PR isn't included this is a quick fix for the issue.

Contributor license agreement signed?

CLA [ Yes ]

@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Aug 26, 2020
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@krisgesling krisgesling added the Merge after next release For large changes that look good, but we want to keep in Dev a little longer label Sep 3, 2020
@krisgesling
Copy link
Contributor

Looks good, not already included in #2648 so will merge after the 20.08 release.

@forslund
Copy link
Collaborator Author

forslund commented Sep 3, 2020

It is included in #2648 since the new ProcessStatus class declares the state before registering the handlers (which replaces these handlers)

self._loaded_status was accessed from a message handler, if a message is
received at the precisely wrong time an exception may occur. This fixes
the init order.

Also moves the skill_updater creation to be fore the handlers are
registered.
@forslund forslund force-pushed the bugfix/all_loaded-order branch from c9a8093 to b71bedd Compare September 7, 2020 04:35
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund
Copy link
Collaborator Author

forslund commented Sep 7, 2020

I've updated this with a possible fix for another issue flagged by @j1nx, it now creates the skill_updater object before registering the bus handlers.

I'm not quite sure in which scenario this would make a difference but should be more correct.

@krisgesling
Copy link
Contributor

Just saw that I missed merging this after the 20.08 release. Thanks again :)

@krisgesling krisgesling merged commit 31fd991 into MycroftAI:dev Sep 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Merge after next release For large changes that look good, but we want to keep in Dev a little longer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants