Skip to content

Cleanly stop tradfri on shutdown#17114

Merged
pvizeli merged 8 commits intodevfrom
tradfri_shutdown
Oct 5, 2018
Merged

Cleanly stop tradfri on shutdown#17114
pvizeli merged 8 commits intodevfrom
tradfri_shutdown

Conversation

@lwis
Copy link
Copy Markdown
Member

@lwis lwis commented Oct 3, 2018

Description:

Shutdown tradfri on HA shutdown to avoid event loop errors on open observations.

Library syntax bump to 3.5.

Handle aiocoap OSErrors errors upstream.

Related issue (if applicable): fixes #???

Example entry for configuration.yaml (if applicable):

discover:

@ghost ghost assigned lwis Oct 3, 2018
@ghost ghost added the in progress label Oct 3, 2018
"""Close connection when hass stops."""
hass.async_add_job(factory.shutdown())

self.hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, on_hass_stop)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

undefined name 'self'
undefined name 'EVENT_HOMEASSISTANT_STOP'

"""Close connection when hass stops."""
hass.async_add_job(factory.shutdown())

self.hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, on_hass_stop)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

undefined name 'self'
undefined name 'EVENT_HOMEASSISTANT_STOP'


def on_hass_stop(event):
"""Close connection when hass stops."""
hass.async_add_job(factory.shutdown())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't use this function anymore. If it a coro use async_create_task. You need also decorate that as callback.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@pvizeli hmm ok, I lifted the example from elsewhere in the code after searching for async_listen_once(EVENT_HOMEASSISTANT_STOP.

"""Close connection when hass stops."""
hass.async_add_job(factory.shutdown())

hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, on_hass_stop)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this shutting down the same observation as the one created when the config entry is created? If not, why do we need to wait until home assistant stops? If it is the same, we should only need to shut it down once, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's any observations that pytradfri holds that have been created since loading, if there's a disconnect and reconnect, that'll be a new observation. It's a global shutdown for any connections pytradfri is holding.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So then we only need one event listener?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry should have worded that better - any factory instances hold observations, I put this next to factory creation to ensure each factory is shutdown.

Looking at the code below this, you're right - we only need to call it once as the api isn't used after. I'll change later.

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good!

@pvizeli pvizeli merged commit 4218efd into dev Oct 5, 2018
@pvizeli pvizeli deleted the tradfri_shutdown branch October 5, 2018 07:59
@ghost ghost removed the in progress label Oct 5, 2018
@balloob balloob mentioned this pull request Oct 12, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants