Improve collection of Zone Group State for large Sonos systems (partial fix for #518 / #935)

[pwt]

No description provided.

[jjlawren]

I'm a little hesitant about this approach:

1. Not every network can handle subscription callbacks properly.
2. This opens/closes a listening socket every time the ZGS is requested, which could cause other side effects.
3. What happens if a different event handler has already been loaded, such as the async implementation?

[pwt]

Thanks for the comments, @jjlawren.

1. Not every network can handle subscription callbacks properly.

The events-based approach is only invoked if a normal query fails. It's a last resort, so if it fails due to network limitations it's still better than not trying.

2. This opens/closes a listening socket every time the ZGS is requested, which could cause other side effects.

The cacheing logic limits ZGS refreshes to at most once per 5 seconds by default (using POLLING_CACHE_TIMEOUT). That seems reasonable to me.

3. What happens if a different event handler has already been loaded, such as the async implementation?

The logic currently only handles the standard events module. I'll try adding support for the others, although I could apply the same 'it's better than nothing' rationale as for (1).

Of course, this is all predicated on this approach actually working for large speaker populations! Reading through the issues (e.g., #518 (comment)) makes me optimistic that it might. I'm looking forward to seeing some results from the use of your test script on suitably large systems.

[pwt]

Hi @jjlawren:

I've now added support for events_asyncio which appears to work but, due to my inexperience with asyncio development, clearly has room for improvement.

If you get a moment, perhaps you could take a look at ~ lines 181-207 and point out the right way to approach this? In particular I couldn't figure out how to retrieve the ZGS from the callback without resorting to a global.

[jjlawren]

I'd really like to have confirmation that this is the actual underlying issue first.

In my downstream implementation that uses soco, I think I'd actually prefer to know that the polling call failed (at least optionally) so I can handle the issue myself by creating my own long-lived subscriptions. Perhaps with a special exception that better describes why this is failing, like SonosNotSupported to indicate it's a Sonos-side limitation. The automatic fallback fixes here are complex and are not risk-free.

[pwt]

I'd really like to have confirmation that this is the actual underlying issue first.

Agree.

In my downstream implementation that uses soco, I think I'd actually prefer to know that the polling call failed (at least optionally) so I can handle the issue myself by creating my own long-lived subscriptions. Perhaps with a special exception that better describes why this is failing, like SonosNotSupported to indicate it's a Sonos-side limitation. The automatic fallback fixes here are complex and are not risk-free.

I think I'd like a default that works without plunging users into having to understand how to subscribe to and process ZGT events, etc. I take your point, though, so we should provide a config option to opt out of the fallback behaviour.

[pwt]

In its current state this PR is a sticking plaster that falls back to using a ZoneGroupTopology event to determine the ZoneGroupState if a direct GetZoneGroupState() lookup fails.

• The fallback behaviour can be disabled by setting config.ZGT_EVENT_FALLBACK to False, in which case the SoCoUPnPException can be handled elsewhere by the caller
• The fallback only works when using the standard soco.events module, not soco.events_twisted or soco.events_asyncio

It's likely that if GetZoneGroupState() fails once then it will continue to fail. Hence, an improvement after the first failure would be to avoid the expense of the initial GetZoneGroupState() call and use the event fallback to begin with. This should probably be coupled with a periodic reset to confirm that GetZoneGroupState() is still failing.

[pwt]

Hi @jjlawren: From my perspective this is ready to merge now, and your review would be welcome.

There's support for falling back to using a ZGT event under the default events module and when using events_asyncio. Support for events_twisted can be added later by someone with the required expertise; the code scaffolding is there for it.

Hi @KennethNielsen FYI, in case you want to take a look.

I'd like to land this in v0.29, to be released within the next week or so.

[KennethNielsen]

I have a few questions, mostly related to async I probably just don't understand.

[pwt]

Thanks @jjlawren and @KennethNielsen for taking the time to review this PR, and thanks also to @jjlawren for the insight that led to creating this workaround.

It still lacks support for events_twisted (I couldn't get this to work), but I suspect the Venn diagram intersection of users with very large Sonos systems and events_twisted users is rather small.

I'll merge this in the next day or two, and release SoCo v0.29.