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

Enhancement, BREAKING CHANGE - await for the unsubscribe()'s reply before closing the connection #196

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lmagyar
Copy link
Contributor

@lmagyar lmagyar commented Jun 17, 2024

Without this, an unsubscribe() followed by a disconnect() always causes an exception: unsubscribe() can't even send the unsubscribe message, OK it's not critical before a close/disconnect to send it (and even wait for the reply), but not causing an unnecessary exception I think is better:

DEBUG:hass_client:Closing client connection
DEBUG:hass_client:Listen completed. Cleaning up
ERROR:asyncio:Task exception was never retrieved
future: <Task finished name='Task-6' coro=<HomeAssistantClient._send_json_message() done, defined at Z:\xxx\.venv\Lib\site-packages\hass_client\client.py:390> exception=NotConnected()>
Traceback (most recent call last):
  File "Z:\xxx\.venv\Lib\site-packages\hass_client\client.py", line 396, in _send_json_message
    raise NotConnected
hass_client.exceptions.NotConnected

without this unsubscribe + disconnect causes an exception
'''
DEBUG:hass_client:Closing client connection
DEBUG:hass_client:Listen completed. Cleaning up
ERROR:asyncio:Task exception was never retrieved
future: <Task finished name='Task-6' coro=<HomeAssistantClient._send_json_message() done, defined at Z:\xxx\.venv\Lib\site-packages\hass_client\client.py:390> exception=NotConnected()>
Traceback (most recent call last):
  File "Z:\xxx\.venv\Lib\site-packages\hass_client\client.py", line 396, in _send_json_message
    raise NotConnected
hass_client.exceptions.NotConnected
'''
@marcelveldt
Copy link
Member

I'm not a fan of needing a coroutine to unsubscribe, especially because in 99% of the cases this is done when we want to cleanup. At the same time I wonder if the unsubscribe is even needed because HA already cleans up the sub when the client disconnects.

I'd really like to keep this a simple callback method and not a couroutine

@marcelveldt marcelveldt reopened this Jun 17, 2024
@lmagyar
Copy link
Contributor Author

lmagyar commented Jun 18, 2024

It's your lib. It was just an idea, even myselft wasn't sure it is really needed.

I use unsubscribe, because currently I need only to capture few event's But even in my case when I close the connection, the unsubscribe is already happened.

So you can close or merge, your decision. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants