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

Close order revert #394

Merged
merged 8 commits into from
Nov 22, 2024
Merged

Close order revert #394

merged 8 commits into from
Nov 22, 2024

Conversation

Lurker00
Copy link

It is essential first to close the sub-devices, to make their _is_closing==True, and only then disconnect the gateway. There was my comment that you've removed. Now I revert your change and put back the comment.

@Lurker00
Copy link
Author

Also calls subdevice_state() -> subdevice_state_updated()!

@Lurker00
Copy link
Author

I decided to push my next small touch to this PR: it prevents sending commands out of order, when commands are sent in bulk.

@@ -347,9 +347,11 @@ async def close(self):

tasks = [self._task_shutdown_entities, self._task_reconnect, self._task_connect]
pending_tasks = [task for task in tasks if task and task.cancel()]
pending_tasks += [self.abort_connect()]
pending_tasks += []
Copy link
Owner

Choose a reason for hiding this comment

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

This does affect parent devices, why not move sub_devices close block above tasks cancel?

Copy link
Author

Choose a reason for hiding this comment

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

The primary goal of the previous PR was to shutdown the whole LocalTuya fully and efficiently.

The primary goal of this PR is to fix the problem: calling abort_connect() before closing the sub-devices causes
full featured disconnected() of sub-devices, including launching their _task_reconnect tasks, which is a waste of CPU time.

Now back to your question:

Doing close of sub-device before stopping those 3 tasks allows the 3 tasks to continue running during awaits for sub-devices close() calls. This way the 3 tasks do redundant jobs, which is inefficient.

What may slightly improve the preformance is to put the sub-devices close loop between cancelling the 3 tasks and asyncio.gather call for them. But it's a subtle improvement. Also, the current order has more predictable result.

@Lurker00
Copy link
Author

Comment to the last commit:

Idents don't really matter here, because task switch may happen at await statements only. Meaning, before the "Idents :(" commit, the task that just released the lock, updates self._last_command_sent before the next task has an opportunity to acquire the lock.

@Lurker00
Copy link
Author

Comment to the last commit:

The cancellation was broken by adding handlers of asyncio.CancelledError into _make_connection: if _make_connection called from await self.async_connect() call from _async_reconnect receives this exception, it gracefully returns to the caller, and then _async_reconnect falls into the sleep for 5 or 10 seconds.

Note exactly 5 seconds delay between closing mains powered and then battery powered WiFi devices:

2024-11-22 10:38:51.838 INFO (MainThread) [custom_components.localtuya.coordinator] [bf0...vcr - ВС: USB Smart Adapter] Closed connection
2024-11-22 10:38:56.839 INFO (MainThread) [custom_components.localtuya.coordinator] [bf9...qv2 - ГС: T&H Сенсор] Closed connection

No a need to check for _is_closing after sleeps, because the sleeps should be interrupted by asyncio.CancelledError. That's why I just moved the check down.

@xZetsubou xZetsubou merged commit 3c2b926 into xZetsubou:master Nov 22, 2024
2 checks passed
@Lurker00 Lurker00 deleted the close_order_revert branch November 22, 2024 08:53
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