Sonos setup fails with unhandled exceptions on discovery messages #90648

PeteRager · 2023-04-01T17:49:35Z

Proposed change

During setup of manually configured Sonos devices, if the devices are offline setup of the integration may fail. There were several issues in the code that needed to be resolved.

Exceptions were not being handled when the device was unable to be pinged and were flowing back to HASS during setup. Now the exceptions are handled and logged, setup will complete, and the devices will be periodically retried.
During initialization, a blocking call to soco.uuid was occurring within the HASS event loop, this issue is mitigated by not attempting the ping if the prior call to get the uuid via async_add_executor_job failed.
If an exception occurred when pinging the device, the call to async_call_later (to retry it) would not occur and hence the device would never come back on-line without a HASS restart.
In the attached issues asyncio.TimeoutError was unhandled, during my testing I saw RequestException occur. Added these to the existing exception handler to make them consistent.

Type of change

Dependency upgrade
[X ] Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes Sonos integration fails upon HA restart, if any device is powered off #90226
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

home-assistant · 2023-04-01T17:49:43Z

Hey there @cgtobi, @jjlawren, mind taking a look at this pull request as it has been labeled with an integration (sonos) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of sonos can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign sonos Removes the current integration label and assignees on the pull request, add the integration domain after the command.

homeassistant/components/sonos/__init__.py

1. Speaker activity messages were being sent for speakers whose pings fail, resulting in the speaker remaining available in HASS. Now the activity message is only sent after a successful ping. 2. Remove RequestException from except handler as its a derived class of OSError() 3. If the second host in a list of two hosts was unavailable, then a blocking call would occur in the event loop. Now, fully process the lists of hosts in the first loop; and only process hosts there are not in error in the second loop.

PeteRager · 2023-04-10T22:01:31Z

@jjlawren changes are complete, please review when you have a couple

A question you may have is why I moved the async_dispatcher_send after the ping. What I found is when I unplugged a speaker that message would get sent and that was keeping the speaker alive beyond the ~270 second timeout so it would not get marked unavailable. Moving it after a successful ping eliminates that problem at which point that message makes the speaker become available.

jjlawren

Thanks for testing thoroughly. Getting the failure scenarios for Sonos devices is tricky as network layouts and behaviors vary wildly. This manual polling code should only be necessary when the various discovery methods are not operational. I have tested many similar scenarios by forcibly breaking discovery on my network but it's not a setup I enjoy re-testing.

homeassistant/components/sonos/__init__.py

jjlawren

I can see how this helps during startup. Should be a good improvement. Thanks again for testing and the PR 👍

PeteRager · 2023-04-22T11:54:24Z

Thank you for the review. While I don't expect any issues as we now have much better unit tests. If for some reason an issue is reported on this piece of code, tag me and I'll get right on it.

tests/components/sonos/test_init.py

bdraco · 2023-05-01T17:23:38Z

Second opinion request is for #90648 (comment)

tests/components/sonos/test_init.py

Don't use event wait for Simplify waiting on multiple events

bdraco · 2023-05-21T22:20:01Z

tests/components/sonos/test_init.py

@@ -84,7 +99,9 @@ async def test_async_poll_manual_hosts_warnings(
    manager.hosts.add("10.10.10.10")
    with caplog.at_level(logging.DEBUG), patch.object(
        manager, "_async_handle_discovery_message"
-    ), patch("homeassistant.components.sonos.async_call_later"), patch(
+    ), patch(
+        "homeassistant.components.sonos.async_call_later"


We probably shouldn’t be patching async_call_later here but that’s an existing issue

Agreed, that's from a prior test. I'll get that fixed in a new pull request.

PeteRager · 2023-05-26T11:45:59Z

@jjlawren please review when you have 5.

jjlawren

This codepath should only be needed when network discovery is failing and manually configured hosts are required. Tests are a bit more complex now, but the previous limitation of a single mocked device is good to relax. Thanks for the thorough testing here 👍

bdraco · 2023-05-30T14:41:58Z

Pushed to my production to verify that it didn't cause any side effects. All good 👍

bdraco · 2023-05-30T14:42:10Z

Merging in dev to get a clean CI run before merging

bdraco · 2023-05-30T15:09:00Z

Thanks @PeteRager

Sonos setup fails with unhandled exceptions on discovery messages

2fb13d5

PeteRager requested review from cgtobi and jjlawren as code owners April 1, 2023 17:49

home-assistant bot added bugfix cla-signed has-tests integration: sonos labels Apr 1, 2023

home-assistant bot assigned cgtobi and jjlawren Apr 1, 2023

home-assistant bot added the Quality Scale: No score label Apr 1, 2023

PeteRager marked this pull request as draft April 1, 2023 17:49

PeteRager marked this pull request as ready for review April 1, 2023 17:54

PeteRager marked this pull request as draft April 2, 2023 13:56

jjlawren reviewed Apr 5, 2023

View reviewed changes

homeassistant/components/sonos/__init__.py Outdated Show resolved Hide resolved

homeassistant/components/sonos/__init__.py Outdated Show resolved Hide resolved

homeassistant/components/sonos/__init__.py Outdated Show resolved Hide resolved

PeteRager added 2 commits April 7, 2023 09:42

Update test_init.py

edb6066

PeteRager marked this pull request as ready for review April 7, 2023 13:56

PeteRager requested a review from jjlawren April 7, 2023 13:57

PeteRager added 4 commits April 15, 2023 18:37

Merge branch 'dev' into sonos-timeout-error

2e82be8

Merge issues

dccb60e

Add helper function to get hostname

9c849fc

Fix doc string

633f1d3

jjlawren reviewed Apr 18, 2023

View reviewed changes

homeassistant/components/sonos/__init__.py Show resolved Hide resolved

jjlawren approved these changes Apr 22, 2023

View reviewed changes

frenck added the smash Indicator this PR is close to finish for merging or closing label Apr 22, 2023

bdraco reviewed Apr 23, 2023

View reviewed changes

tests/components/sonos/test_init.py Outdated Show resolved Hide resolved

Remove patching of hass system object

70ab97e

PeteRager added 5 commits May 1, 2023 08:37

Updates

60a3fcc

Add delay

744c162

Fix tests errors caused by merging issue

1ad6445

Update conftest.py

427607b

Move subscription timeout patch to conftest

30bd851

bdraco reviewed May 1, 2023

View reviewed changes

tests/components/sonos/test_init.py Outdated Show resolved Hide resolved

bdraco added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label May 1, 2023

PeteRager added 2 commits May 21, 2023 10:52

remove test access to hass.data

b9910fc

Update test_init.py

f08158e

bdraco reviewed May 21, 2023

View reviewed changes

tests/components/sonos/test_init.py Outdated Show resolved Hide resolved

bdraco removed the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label May 21, 2023

PeteRager added 3 commits May 21, 2023 17:04

Merge branch 'dev' into sonos-timeout-error

17ca8da

Fix regression error, remove unneeded change

26d0a0b

Wait on events rather than using a sleep

28d46b0

bdraco reviewed May 21, 2023

View reviewed changes

tests/components/sonos/test_init.py Outdated Show resolved Hide resolved

Updates

621c388

Don't use event wait for Simplify waiting on multiple events

bdraco reviewed May 21, 2023

View reviewed changes

bdraco approved these changes May 21, 2023

View reviewed changes

bdraco requested a review from jjlawren May 21, 2023 22:57

jjlawren mentioned this pull request May 28, 2023

Sonos - Detected blocking call to putrequest inside the event loop: soco.uid, ip_addr, "manual zone scan" #93715

Closed

jjlawren approved these changes May 30, 2023

View reviewed changes

Merge branch 'dev' into sonos-timeout-error

2e19cf1

bdraco approved these changes May 30, 2023

View reviewed changes

bdraco merged commit 6a8d18a into home-assistant:dev May 30, 2023
20 checks passed

github-actions bot locked and limited conversation to collaborators May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sonos setup fails with unhandled exceptions on discovery messages #90648

Sonos setup fails with unhandled exceptions on discovery messages #90648

PeteRager commented Apr 1, 2023 •

edited

home-assistant bot commented Apr 1, 2023

PeteRager commented Apr 10, 2023 •

edited

jjlawren left a comment

jjlawren left a comment

PeteRager commented Apr 22, 2023

bdraco commented May 1, 2023

bdraco May 21, 2023

PeteRager May 21, 2023

PeteRager commented May 26, 2023

jjlawren left a comment

bdraco commented May 30, 2023

bdraco commented May 30, 2023

bdraco commented May 30, 2023

Sonos setup fails with unhandled exceptions on discovery messages #90648

Sonos setup fails with unhandled exceptions on discovery messages #90648

Conversation

PeteRager commented Apr 1, 2023 • edited

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Apr 1, 2023

PeteRager commented Apr 10, 2023 • edited

jjlawren left a comment

Choose a reason for hiding this comment

jjlawren left a comment

Choose a reason for hiding this comment

PeteRager commented Apr 22, 2023

bdraco commented May 1, 2023

bdraco May 21, 2023

Choose a reason for hiding this comment

PeteRager May 21, 2023

Choose a reason for hiding this comment

PeteRager commented May 26, 2023

jjlawren left a comment

Choose a reason for hiding this comment

bdraco commented May 30, 2023

bdraco commented May 30, 2023

bdraco commented May 30, 2023

PeteRager commented Apr 1, 2023 •

edited

PeteRager commented Apr 10, 2023 •

edited