fredrike / pydaikin

GNU General Public License v3.0
8 stars 9 forks source link

I/O blocking in the event loop #3

Open tomlut opened 4 months ago

tomlut commented 4 months ago

I have been ashed to post this here. See https://github.com/home-assistant/core/issues/114345

As per private discussion with bdraco about relatively long Daikin integration start up times, he discovered that the Daikin integration does I/O blocking in the event loop:

... it does blocking I/O in the event loop

https://bitbucket.org/mustang51/pydaikin/src/45f5ead619d861727626a7ab0522a0c18f00e0ba/pydaikin/daikin_base.py#lines-59

these imports do blocking I/O in the event loop

creating an ssl context does blocking I/O in the event loop https://bitbucket.org/mustang51/pydaikin/src/45f5ead619d861727626a7ab0522a0c18f00e0ba/pydaikin/daikin_brp072c.py#lines-24 (edited)

fredrike commented 4 months ago

@tomlut do you have a suggestion how to fix this? I'm more than happy to accept PRs but have limited time to fit things myself.

@icovada perhaps you can help with this.

tomlut commented 4 months ago

Unfortunately not. I'm a user, not a developer.

@bdraco has been the driving push behind this, he may have some pointers for you.

bdraco commented 4 months ago

The best way to fix it would be to break apart the lib a bit more to avoid the circular imports and make those top level imports so its all loaded at import time which automatically runs in the executor

cremor commented 1 month ago

@kingy444 Has this issue been fixed by your PR #6?

kingy444 commented 1 month ago

I believe it should have - not sure how to test / confirm to close this one out

tomlut commented 1 month ago

Well something has changed, not necessarily for the better though:

Logger: homeassistant.helpers.entity
Source: helpers/entity.py:1266
First occurred: 5 August 2024 at 03:09:29 (174 occurrences)
Last logged: 07:09:11

Update of climate.downstairs is taking over 10 seconds
Update of sensor.downstairs_inside_temperature is taking over 10 seconds

Logger: homeassistant.components.daikin
Source: components/daikin/__init__.py:121
integration: Daikin AC (documentation, issues)
First occurred: 5 August 2024 at 16:11:45 (18 occurrences)
Last logged: 05:09:32

Connection failed for 10.1.1.18

Logger: homeassistant.components.sensor
Source: helpers/entity_platform.py:1031
integration: Sensor (documentation, issues)
First occurred: 5 August 2024 at 16:43:56 (20 occurrences)
Last logged: 05:09:31

Updating daikin sensor took longer than the scheduled update interval 0:00:30

Logger: homeassistant.components.climate
Source: helpers/entity_platform.py:1031
integration: Climate (documentation, issues)
First occurred: 01:27:25 (1 occurrences)
Last logged: 01:27:25

Updating daikin climate took longer than the scheduled update interval 0:01:00
kingy444 commented 1 month ago

Not seeing anything like that in my logs and I have 3 instances running against my controller (dev/test/prod)

There are some inconsistencies between how the different models work so I can only test with the Airbase models

Slow updates doesn't answer if IO Blocking and circular imports are still an issue though - how did you initially identify that as an issue?

tomlut commented 1 month ago

I didn't. bdraco did (see first post).

cremor commented 1 month ago

Well something has changed, not necessarily for the better though:

@tomlut Which version of Home Assistant did you use to test? AFAIK the changes which have been made for this issue will only be released with Home Assistant 2024.8

tomlut commented 1 month ago

Ah, I see. I'm still on 2024.7. Was going to have a go at the beta but did not get time.

tomlut commented 1 month ago

Just updated to 2024.8. Now it fails to connect:

Failed setup, will retry: Cannot connect to host 10.1.1.18:443 ssl:default [[SSL: UNSAFE_LEGACY_RENEGOTIATION_DISABLED] unsafe legacy renegotiation disabled (_ssl.c:1000

Can connect fine in the Daikin phone app.

Tried removing the integration and it was discovered before I got around to adding it again, however when I click the configure button and submit:

Screenshot 2024-08-08 at 13-01-06 Settings – Home Assistant

bdraco commented 1 month ago

Failed setup, will retry: Cannot connect to host 10.1.1.18:443 ssl:default [[SSL: UNSAFE_LEGACY_RENEGOTIATION_DISABLED] unsafe legacy renegotiation disabled (_ssl.c:1000

Similar fix for another lib assuming you need to turn on legacy: https://github.com/gwww/elkm1/pull/69

tomlut commented 1 month ago

It's like it is trying to connect using SSL when it is not a secure connection:

Logger: pydaikin.daikin_base Source: components/daikin/config_flow.py:87 First occurred: 13:08:35 (1 occurrences) Last logged: 13:08:35

Exception in TaskGroup: Cannot connect to host 10.1.1.18:80 ssl:default [Connect call failed ('10.1.1.18', 80)]

DaveTSG commented 1 month ago

Just updated to 2024.8. Now it fails to connect:

Failed setup, will retry: Cannot connect to host 10.1.1.18:443 ssl:default [[SSL: UNSAFE_LEGACY_RENEGOTIATION_DISABLED] unsafe legacy renegotiation disabled (_ssl.c:1000

Can connect fine in the Daikin phone app.

For what it's worth, I'm seeing the same issue, sadly.

cremor commented 1 month ago

Could you please create a new issue for that, maybe both here and in the Home Assistant repository? That new issue has nothing to do with this one here.

edit: There is already an issue in the Home Assistant repository: https://github.com/home-assistant/core/issues/123160

kingy444 commented 1 month ago

Failed setup, will retry: Cannot connect to host 10.1.1.18:443 ssl:default [[SSL: UNSAFE_LEGACY_RENEGOTIATION_DISABLED] unsafe legacy renegotiation disabled (_ssl.c:1000

Similar fix for another lib assuming you need to turn on legacy: gwww/elkm1#69

@fredrike can we implement this and release a new version that supports fallback to http

TheDJVG commented 1 month ago

FWIW I see that some work has been done to add ssl context that sets some things in the past. https://github.com/fredrike/pydaikin/blob/cda9a0fe884e6e2b02b9243f7cc840d92429069b/pydaikin/daikin_brp072c.py#L24-L28 However I do no see anywhere in the code this context being referenced/used:

# git --no-pager grep _ssl_context 
pydaikin/daikin_brp072c.py:        self._ssl_context = ssl.create_default_context(ssl.Purpose.SERVER_AUTH)
pydaikin/daikin_brp072c.py:        self._ssl_context.options |= 0x4
pydaikin/daikin_brp072c.py:        self._ssl_context.check_hostname = False
pydaikin/daikin_brp072c.py:        self._ssl_context.verify_mode = ssl.CERT_NONE

When patching the HA side of things like this:

    session = async_get_clientsession(hass)
    session._connector._ssl.options |= 0x4
    session._connector._ssl.check_hostname = False
    session._connector._ssl.verify_mode = ssl.CERT_NONE

The connection succeeds again, however I'm now getting a 403 forbidden error. I currently miss to see how these things could be related but people with more experience on the daikin local API might know more about it.

Just for completion the 403 happens when the common/register_terminal path is being called on my BRP072C42.

UPDATE

The 403 is coming from the fact that the X-Daikin-uuid header is not being added, although it seems like it's being set, just not used:

# git --no-pager grep _headers
pydaikin/daikin_brp072c.py:        self._headers = {"X-Daikin-uuid": self._uuid}

Dirty patching the _get_resource method adding the headers if they're set restored functionaility for me:

            async with self.session.get(
                f'{self.base_url}/{path}', params=params, headers=getattr(self, '_headers', None)
            ) as response:

Long story short:

adprom commented 1 month ago

I tried this fix but get the following error in the logs

`Logger: homeassistant.config_entries Source: config_entries.py:604 First occurred: 2:09:29 PM (9 occurrences) Last logged: 2:10:08 PM

Error setting up entry 192.168.1.73 for daikin Error setting up entry 192.168.1.74 for daikin Error setting up entry 192.168.1.75 for daikin Error setting up entry 192.168.1.76 for daikin Error setting up entry 192.168.1.71 for daikin Traceback (most recent call last): File "/usr/src/homeassistant/homeassistant/config_entries.py", line 604, in async_setup result = await component.async_setup_entry(hass, self) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/src/homeassistant/homeassistant/components/daikin/init.py", line 49, in async_setup_entry daikin_api = await daikin_api_setup( ^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/src/homeassistant/homeassistant/components/daikin/init.py", line 88, in daikin_api_setup session._connector._ssl.verify_mode = ssl.CERT_NONE ^^^ NameError: name 'ssl' is not defined. Did you forget to import 'ssl'`

TheDJVG commented 1 month ago

I tried this fix but get the following error in the logs

`Logger: homeassistant.config_entries Source: config_entries.py:604 First occurred: 2:09:29 PM (9 occurrences) Last logged: 2:10:08 PM

Error setting up entry 192.168.1.73 for daikin Error setting up entry 192.168.1.74 for daikin Error setting up entry 192.168.1.75 for daikin Error setting up entry 192.168.1.76 for daikin Error setting up entry 192.168.1.71 for daikin Traceback (most recent call last): File "/usr/src/homeassistant/homeassistant/config_entries.py", line 604, in async_setup result = await component.async_setup_entry(hass, self) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/src/homeassistant/homeassistant/components/daikin/init.py", line 49, in async_setup_entry daikin_api = await daikin_api_setup( ^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/src/homeassistant/homeassistant/components/daikin/init.py", line 88, in daikin_api_setup session._connector._ssl.verify_mode = ssl.CERT_NONE ^^^ NameError: name 'ssl' is not defined. Did you forget to import 'ssl'`

The solution is mentioned in the error, you need to import the ssl module. Put import ssl at the top of the file with the other imports.

adprom commented 1 month ago

Yup, that worked. That 3rd step made everything work.

For anyone trying this, be careful not to change anything else including indentation of other lines.

Not being a python native, it wasn't obvious.

kingy444 commented 1 month ago

If anyone has the knowledge to build the package locally and test the above, then submit a PR for a fix that would be great.

I can patch with the above steps but I don't have this model to test, and won't be near a PC for another couple of days either.

Only @fredrike can package for pip and does not have a unit for testing of these fixes either.

I suspect that the units could also have SSL enabled as a fix as whoever added the forced SSL assumably was using SSL. My device is one that still uses http (airbase)

cremor commented 1 month ago

There is already a PR: #13

fredrike commented 1 month ago

If anyone has the knowledge to build the package locally and test the above, then submit a PR for a fix that would be great.

I can patch with the above steps but I don't have this model to test, and won't be near a PC for another couple of days either.

Only @fredrike can package for pip and does not have a unit for testing of these fixes either.

I suspect that the units could also have SSL enabled as a fix as whoever added the forced SSL assumably was using SSL. My device is one that still uses http (airbase)

That's right I actually don't have any Daikin devices at all anymore. I would be happy to add more maintainers to pydaikin and it would be great with a CI-flow that pushes new versions to pypi.

There is already a PR: #13

I've merged and pushed a new version now.

kingy444 commented 1 month ago

Looks like that bump / PR has broken more devices (including ones that were already working) in 2024.8.1

Haven't looked at the content of the PR - but probably just need to set a default as not all controllers work the same way. The AirBase I have doesn't appear to present headers based on 2024.8.1 breaking connection

EDIT: Happy to take a look at fixing but not near my machine for another 12-18 hours minimum

image

ngolf commented 1 month ago

All my daikin units seemed to have stopped working this morning (I also updated to 2024.8.1, not sure if time coincides exactly though). I can see them connected to my local network, and control them via the daikin app.

andymcmanus commented 1 month ago

Just updated to 2024.8. Now it fails to connect:

Failed setup, will retry: Cannot connect to host 10.1.1.18:443 ssl:default [[SSL: UNSAFE_LEGACY_RENEGOTIATION_DISABLED] unsafe legacy renegotiation disabled (_ssl.c:1000

Can connect fine in the Daikin phone app.

Tried removing the integration and it was discovered before I got around to adding it again, however when I click the configure button and submit:

Screenshot 2024-08-08 at 13-01-06 Settings – Home Assistant

I got the same here. But this started when I went from 2024.8.0 to 2024.8.1 I've just rolled back to 2024.8.0 and it's all working again. :(

ngolf commented 1 month ago

Rolling back to 2024.8.0 fixed it for me too.

kingy444 commented 1 month ago

It all depends on the controller you have, they all have different api requirements.

2024.8.0 saw a dependancy version bump after considerable changes to the upstream library, so any downgrade 2024.7.x will drop the dependency to the old library

tomlut commented 1 month ago

I loaded up the changed integration as a custom component and it has been working well enough for both my ducted and split system. I say "well enough" as while they operate as expected I do see this in the logs:

2024-08-16 06:10:27.290 WARNING (MainThread) [homeassistant.helpers.entity] Update of sensor.downstairs_inside_temperature is taking over 10 seconds
2024-08-16 06:10:47.290 WARNING (MainThread) [homeassistant.components.sensor] Updating daikin sensor took longer than the scheduled update interval 0:00:30
2024-08-16 06:11:17.292 WARNING (MainThread) [homeassistant.components.sensor] Updating daikin sensor took longer than the scheduled update interval 0:00:30
2024-08-16 06:11:27.294 WARNING (MainThread) [homeassistant.helpers.entity] Update of switch.downstairs_streamer is taking over 10 seconds
2024-08-16 06:12:27.298 WARNING (MainThread) [homeassistant.helpers.entity] Update of sensor.downstairs_inside_temperature is taking over 10 seconds
2024-08-16 06:13:27.303 WARNING (MainThread) [homeassistant.helpers.entity] Update of sensor.downstairs_inside_temperature is taking over 10 seconds
2024-08-16 06:15:27.308 WARNING (MainThread) [homeassistant.helpers.entity] Update of sensor.downstairs_inside_temperature is taking over 10 seconds
2024-08-16 06:16:27.311 WARNING (MainThread) [homeassistant.helpers.entity] Update of sensor.downstairs_inside_temperature is taking over 10 seconds
2024-08-16 06:17:27.312 WARNING (MainThread) [homeassistant.helpers.entity] Update of sensor.downstairs_inside_temperature is taking over 10 seconds
2024-08-16 06:17:47.313 WARNING (MainThread) [homeassistant.components.sensor] Updating daikin sensor took longer than the scheduled update interval 0:00:30
2024-08-16 06:19:27.316 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:19:40.956 ERROR (MainThread) [pydaikin.daikin_base] Exception in TaskGroup: Cannot connect to host 10.1.1.18:443 ssl:<ssl.SSLContext object at 0x7fd8cbb9a350> [None]
2024-08-16 06:19:40.957 WARNING (MainThread) [custom_components.daikin] Connection failed for 10.1.1.18
2024-08-16 06:21:27.318 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:23:31.601 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:25:31.607 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:27:31.614 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:30:31.617 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:31:31.616 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:31:55.833 ERROR (MainThread) [pydaikin.daikin_base] Exception in TaskGroup: Cannot connect to host 10.1.1.18:443 ssl:<ssl.SSLContext object at 0x7fd8cbb9a350> [None]
2024-08-16 06:31:55.834 WARNING (MainThread) [custom_components.daikin] Connection failed for 10.1.1.18
2024-08-16 06:33:30.771 ERROR (MainThread) [pydaikin.daikin_base] Exception in TaskGroup: Cannot connect to host 10.1.1.18:443 ssl:<ssl.SSLContext object at 0x7fd8cbb9a350> [[SSL: INVALID_ALERT] invalid alert (_ssl.c:1000)]
2024-08-16 06:34:31.620 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:35:31.622 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:36:31.622 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:41:35.123 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:43:35.126 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:44:35.128 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:45:35.129 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:50:35.134 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:54:38.894 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:55:38.896 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:56:38.901 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:57:38.903 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 06:58:38.905 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 07:03:38.911 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 07:05:42.820 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 07:06:42.831 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 07:08:42.832 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
2024-08-16 07:09:42.831 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.upstairs is taking over 10 seconds
2024-08-16 07:09:42.833 WARNING (MainThread) [homeassistant.helpers.entity] Update of climate.downstairs is taking over 10 seconds
fredrike commented 1 month ago

@tomlut are these errors new or have you seen them before?

This is the changes that seems to have introduced the shit-storm with errors: https://github.com/fredrike/pydaikin/compare/v2.11.1...v2.13.4

tomlut commented 1 month ago

The exception is new. I've had the update taking over... since forever. Though not quite this bad.

fredrike commented 1 month ago

The exception is new. I've had the update taking over... since forever. Though not quite this bad.

Ok, please create a new issue with the exception here and I'll look into it.

Contact me on discord or in the HA-forum so we can test this before releasing another version.