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

Avoid multithreading and other fixes #359

Merged
merged 5 commits into from
Oct 7, 2024

Conversation

Lurker00
Copy link

@Lurker00 Lurker00 commented Oct 2, 2024

8304b61:
_shutdown_entities checks several properties and members that could be changed during connection process. So, to avoid possible race conditions, it is better to call it from the main thread loop of LocalTuya, rather than from HASS thread loop. Instead, a device's entities could be disabled right after successful re-connect. Frankly, I never experienced such a misbehavior.

70970c9:
After several unsuccessful re-connect attempts, it is good to decide that the device is offline and increase delay between further attempts. Just like for sub-devices that were reported as offline by their gateways.

d0a9539:
On close event, _shutdown_entities, actually, did nothing for low power and currently connected devices. Actually, this call has rather cosmetic effect, because the entities are removed from HASS in a moment. But, while it is there, better to make it right way.

@Lurker00
Copy link
Author

Lurker00 commented Oct 2, 2024

Forgot to add:

There also can be a problem with periodical calling of TuyaDevice._async_refresh, but I have no a device which requires it. This may lead to concurrent writes to the interface.

@xZetsubou
Copy link
Owner

xZetsubou commented Oct 4, 2024

Can't we find better way to avoid using call_later it's really not good, maybe if we make _shutdown_entities an async function make a await sleep.

    async def _shutdown_entities(self, exc=""):
        """Shutdown device entities"""
        # Delay shutdown.
        if not self._is_closing:
            await asyncio.sleep(3 + self._device_config.sleep_time)

            if self.connected:
                return

        signal = f"localtuya_{self._device_config.id}"
        dispatcher_send(self._hass, signal, None)

        if self._is_closing:
            return

        if self.is_subdevice:
            self.info(f"Sub-device disconnected due to: {exc}")
        elif hasattr(self, "low_power"):
            m, s = divmod((int(time.time()) - self._last_update_time), 60)
            h, m = divmod(m, 60)
            self.info(f"The device is still out of reach since: {h}h:{m}m:{s}s")
        else:
            self.info(f"Disconnected due to: {exc}")

Now whenever we want to call it w/ delay

        self._call_on_close.append(
            asyncio.create_task(self._shutdown_entities(exc=exc)).cancel
        )

^ as an example it will needs a checks / cancelation on connected

@Lurker00
Copy link
Author

Lurker00 commented Oct 5, 2024

Can't we find better way to avoid using call_later

Agree. I've added is_sleep into the condition, because the device might have woke up and disconnected while _shutdown_entities was sleeping with long enough sleep_time.

@Lurker00
Copy link
Author

Lurker00 commented Oct 5, 2024

^ as an example it will needs a checks / cancelation on connected

The _shutdown_entities task was always dying by itself, and in the proposed implementation still will do. So, no addidional checks are required.

There is another related problem, with _call_on_close: it is growing with already completed async tasks. But it is not critical, and I plan to address it later.

@Lurker00
Copy link
Author

Lurker00 commented Oct 5, 2024

FYI, after merging in this set of changes, I'll PR another set, for better handling of unload/shutdown events. It is ready and is running quite well.

@xZetsubou
Copy link
Owner

If we reload the integration the shutdown task will be canceled however if the connection fails after reload we need to shutdown the entities in that case as well.

@Lurker00
Copy link
Author

Lurker00 commented Oct 7, 2024

if the connection fails after reload we need to shutdown the entities in that case as well.

If the connection fails after reload, the entities should stay in their initial states, i.e. disabled and Unknown. At least this is what I've experienced when LocalTuya failed to start after a restart due to my bugs. Also, I saw entities disabled in the past, when connections started in async_setup_entry before the call to hass.config_entries.async_forward_entry_setups.

Anyway, this PR changes nothing for the case you mentioned, compared to the existing behavior: it sould continue working, or not working, exactly as before 😄 So I suggest to finish here, with loops from dirfferent threads, then go to other possible problems, step by step.

@xZetsubou
Copy link
Owner

forgot about that thanks 😄 you are correct it should be disabled.

@xZetsubou xZetsubou merged commit 2ed259f into xZetsubou:master Oct 7, 2024
2 checks passed
@Lurker00 Lurker00 deleted the avoid-multithreading branch October 7, 2024 10:45
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