mib1185 / py-synologydsm-api

MIT License
16 stars 11 forks source link

Revert "Gather update methods to update faster" #325

Closed bdraco closed 6 months ago

bdraco commented 6 months ago

Reverts mib1185/py-synologydsm-api#301

https://github.com/home-assistant/core/issues/116755

It turns out that some of the slower devices cannot respond within 10s to all the requests because the API is too slow.

While this will make updates much slower for faster devices, we still need to support the older slower ones.

mib1185 commented 6 months ago

can we increase the timeout to let's say 30s instead and keep the batch processing?

bdraco commented 6 months ago

Changing the default timeout to 30s would probably work as well, however it won't fix anyone with a slow system who has manually set the timeout lower. We could do a one time options change to increase to 30s in the integration if its less than 30s, but we usually try not to alter user's settings. However its not common or a usual design to allow for a user configurable timeout in the first place, and would likely be rejected as an option for any new integration in code review as the timeout value is something developers should pick.

bdraco commented 6 months ago

Also I think there is another issue going on here https://github.com/home-assistant/core/issues/116755#issuecomment-2094209912

mib1185 commented 6 months ago

i meant so change the timeout just for this "bulk fetch" task - something like

async with asyncio.timeout(30)
    await asyncio.gather(*update_methods)
mib1185 commented 6 months ago

would likely be rejected as an option for any new integration in code review as the timeout value is something developers should pick.

i agree and already thought about removing both, the timeout and the scan interval option from the integration

bdraco commented 6 months ago

i meant so change the timeout just for this "bulk fetch" task - something like

async with asyncio.timeout(30)
    await asyncio.gather(*update_methods)

Usually we want aiohttp to handle the timeout https://github.com/mib1185/py-synologydsm-api/blob/737fef672bb2c659862448c3ce2bd1e4bce5aee2/src/synology_dsm/synology_dsm.py#L77 since if a timeout hits inside the asyncio.timeout context manager it will cancel all the tasks inside which sometimes leads to races in tearing down the connections which can make subsequent requests fail. That really shouldn't happen but it does in practice as we have to be sure everything inside an asyncio.timeout block is always cancellation safe. In theory aiohttp should be cancellation safe but we still see bug reports in the aiohttp issue queue where its not.

Another option might be to use gather_with_limited_concurrency https://github.com/home-assistant/core/blob/985fd499094297fc352e0c530e99264c0b742686/homeassistant/util/async_.py#L96 to reduce the number of requests.

I think before we know what to do we need to see if increasing the timeout works for the user

bdraco commented 6 months ago

Let's hold off on this until we know if increasing the timeout to 30s solves the issue https://github.com/home-assistant/core/issues/116755#issuecomment-2094281502

mib1185 commented 6 months ago

I think increasing the timeout to 30s should be sufficient - we have a quiet old and slow ds213+ https://github.com/home-assistant/core/issues/116755#issuecomment-2094366092 which is fine with 30s

bdraco commented 6 months ago

Let's go with adjusting the default value in HA instead.