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

ruckus_unleashed: Detected blocking call to sleep inside the event loop #84550

Closed
Cougar opened this issue Dec 25, 2022 · 14 comments
Closed

ruckus_unleashed: Detected blocking call to sleep inside the event loop #84550

Cougar opened this issue Dec 25, 2022 · 14 comments

Comments

@Cougar
Copy link
Contributor

Cougar commented Dec 25, 2022

The problem

Sometimes (actually too often) my Ruckus Unleashed crashes and looks like Home Assistant doesn't handle this error well:

2022-12-25 15:12:42.819 ERROR (MainThread) [homeassistant] Error doing job: Exception in callback _UnixReadPipeTransport._call_connection_lost(OSError(5, 'I/O error'))
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/asyncio/events.py", line 80, in _run
    self._context.run(self._callback, *self._args)
  File "/usr/local/lib/python3.10/asyncio/unix_events.py", line 581, in _call_connection_lost
    self._pipe.close()
  File "/usr/local/lib/python3.10/site-packages/pexpect/pty_spawn.py", line 327, in close
    self.ptyproc.close(force=force)
  File "/usr/local/lib/python3.10/site-packages/ptyprocess/ptyprocess.py", line 403, in close
    time.sleep(self.delayafterclose)
  File "/usr/src/homeassistant/homeassistant/util/async_.py", line 180, in protected_loop_func
    check_loop(func, strict=strict)
  File "/usr/src/homeassistant/homeassistant/util/async_.py", line 141, in check_loop
    raise RuntimeError(
RuntimeError: Detected blocking call to sleep inside the event loop. Use `await hass.async_add_executor_job()`; This is causing stability issues. Please report issue
2022-12-25 15:12:42.826 ERROR (MainThread) [homeassistant.components.ruckus_unleashed] Error fetching ruckus_unleashed data: Could not establish connection to host

What version of Home Assistant Core has the issue?

core-2022.12.8

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant OS

Integration causing the issue

Ruckus Unleashed

Link to integration documentation on our website

https://www.home-assistant.io/integrations/ruckus_unleashed

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

@home-assistant
Copy link

Hey there @gabe565, mind taking a look at this issue as it has been labeled with an integration (ruckus_unleashed) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of ruckus_unleashed can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign ruckus_unleashed Removes the current integration label and assignees on the issue, add the integration domain after the command.

(message by CodeOwnersMention)


ruckus_unleashed documentation
ruckus_unleashed source
(message by IssueLinks)

@elupus
Copy link
Contributor

elupus commented Dec 26, 2022

This is an annoying problem in upstream code: pexpect/ptyprocess#2

It needs some way to close down async.

@elupus
Copy link
Contributor

elupus commented Dec 26, 2022

I dont remember if we check the sleep argument in ha. A sleep of zero is no issue.

@gabe565
Copy link
Contributor

gabe565 commented Dec 29, 2022

@elupus is correct. I've tried to eliminate as many sleep calls as possible, but unfortunately can't avoid all of them when using pexpect.

@ms264556
Copy link
Contributor

The Unleashed SSH client isn't ideal for this sort of integration. Apart from having to insert sleeps etc, this integration will make it very difficult to use the CLI for anything else, since 'enable' is single-user so any CLI user will be forcibly de-privileged every few minutes.

@gabe565 - If you're interested, I forked your pyruckus and migrated it to use HTTPS AJAX, which should be more reliable.
The AJAX results obviously look different than your scraped-from-SSH version, but I left legacy methods which I think massage things enough for your test_data.py and home-assistant usage.

It's a major bit of brain surgery, and I'm not much of a Python programmer, so I didn't PR the changes back to your repo.
But if you find the concept OK, then I could do that.

@ms264556
Copy link
Contributor

ms264556 commented Feb 28, 2023

I added pyruckus methods to block & unblock devices, in case you wanted to add this functionality in Home Assistant.
I've also added extra logic to support ZoneDirector & older releases of Unleashed.

Edit: I added methods to turn an AP's status LEDs on and off, in case you want to e.g. turn them off at night.
Edit: I added methods to enable/disable an SSID.

@gabe565
Copy link
Contributor

gabe565 commented Mar 1, 2023

@ms264556 Nice work! I haven't gotten a chance to take a look at your fork yet, but I love the idea of getting that merged upstream. An HTTP API would be more standard and would fix the blocking call to sleep error.

@ms264556
Copy link
Contributor

In the interest of not breaking existing uses of pyruckus, I pushed a new aioruckus library to PyPi for this.

I had a first go at swapping this in to HA, here, and it seems to work fine.

I didn't look at the tests yet. I'll do this and then I guess I can try to PR.

@ms264556
Copy link
Contributor

ms264556 commented Apr 1, 2023

I've modified the tests.

@gabe565
I'm not sure what the process is for non-owners of integrations to contribute large changes.
If I just do a PR will it come to you for evaluation/remediation?

I'm obviously a bit worried, since this integration seems to be used by ~100 users.
I'd hate to have 100 users receive a lightly tested change which breaks something for them.

@gabe565
Copy link
Contributor

gabe565 commented Apr 1, 2023

Hey! Sorry I've been a bit slow to respond.

If you create a PR, I think it will automatically add me for approval! Although, I think using the same package for this would make sense instead of creating a new one. The package wasn't meant for only SSH so I'd definitely like to merge your PR and add support for both.

Otherwise, it would be fine if this was a breaking change and users have to swap from SSH to HTTP. It would be worth it for the benefits of using a JSON API instead of string parsing

@gabe565
Copy link
Contributor

gabe565 commented Apr 1, 2023

I also forgot to mention this but I don't have access to a physical Ruckus device anymore, so I can't test your change unfortunately. I made a package that mocks Ruckus SSH, but not HTTP

@ms264556
Copy link
Contributor

ms264556 commented Apr 1, 2023

Although, I think using the same package for this would make sense instead of creating a new one. The package wasn't meant for only SSH so I'd definitely like to merge your PR and add support for both.

I added a bunch of pyruckus compatibility shims to my library and poked it into a pyruckus fork, because I thought this was the quickest way to get it into HA. But the pyruckus PR was pending for quite a while, so I ended up just removing the shims and modifying HA to directly use my library.

Feel free to just copy-paste whatever you want from aioruckus into pyruckus (my code is 0BSD licensed, so requires no attribution) then retarget the HA PR.

(I just noticed your comment mentions not having a ruckus device anymore. I'd find it really hard to do bugfixes/enhancements just using a mock).

@gabe565
Copy link
Contributor

gabe565 commented Apr 3, 2023

@ms264556 Yeah that's been the main reason I've been slow to review your PR. It's hard to accept big changes like this without a real device, even though I'm really excited about them.

You're library is looking great, though! I just don't want this to cause fragmentation if we can avoid it. Honestly, at this point I don't really want to keep SSH. Your changes retain all of the same functionality and HTTP is pretty much always preferable imo. I'd like to reopen that PR and get it merged in!

Also, do you want me to add your as a maintainer on gabe565/pyruckus?

Edit: I can't reopen the PR in pyruckus anymore since the history has diverged. Let me know what you want to do, I don't mind working with you either way!

@issue-triage-workflows
Copy link

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates.
Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍
This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

@issue-triage-workflows issue-triage-workflows bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 9, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Aug 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants