Use non-persistent connection for MPD #94507

thijsputman · 2023-06-12T23:20:04Z

Proposed change

Instead of relying on the (persistent) connection handling of python-mpd2 (which is not working optimally – see for example Mic92/python-mpd2#31), explicitly connect and disconnect for every command send to MPD.

This resolves #57633 (comment) (and probably some related issues) where the MPD integration would get stuck in "limbo": Disconnected and unable to reconnect at the same time. Outside of a full restart of Home Assistant there was no way of recovering from this.
With the three MPD players in my network, this issue would occur once per week; a slight hiccup in the Wi-Fi was often already enough to require a full restart of HA.

This change introduces a bit of overhead, but as the integration is local-polling (ie, nothing actually gets pushed back to us over the persistent connection) I'd say the impact is minimal. The MPD protocol doesn't really appear to be geared towards persistent connections any way.

The best solution would probably be a fix in python-mpd2 itself, but I'm afraid that's a bit out of scope for me at the moment.

I've tried to keep changes to a minimum (didn't totally succeed 😇) and use an elegant/modern Python approach. I'm not very familiar with Python, so there might be some oddities here and there...

I've been running this code locally for about two months (with three MPD players on the network) and it's been rock solid: Disconnects happen from time to time, the integration always recovers and never gets stuck in "limbo".

In case anyone is wondering: I did initially try to simply brute-force some additional connection attempts into the integration – that didn't work at all: Once python-mpd2 looses the connection, there doesn't appear to an (obvious) way to get it to reconnect again.

Type of change

Dependency upgrade
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: MPD too noisy (after disconnection) #57633 (comment)
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.

In other words, don't rely on the connection management provided by "python-mpd2". Instead of keeping the connection to MPD open, we explicitly connect before and disconnect after each command. There is probably a bit of overhead to this, but as the integration uses a local-polling approach to begin with, no functionality is lost or degraded. This change greatly hardens the MPD integration against both network issues and problems with the daemon itself. All connection-related failure modes have effectively been removed.

Only "async_get_media_image" attempts to connect, all others are either called from there, or from the main "async_update" method (see previous commit) which also attempts to connect. So, this commit mainly revolves around gracefully handling situations where no connection is available when trying to retrieve MPD state. Finally, note the removal of "self._commands". This property is only used at the start of "_async_get_file_image_response" and was thus changed into a local variable.

These all need to explicitly connect to MPD as part of their flow.

emontnemery

I think the change is fine since the implementations is polling, with an interval of 120s.

A few comments though

homeassistant/components/mpd/media_player.py

emontnemery · 2023-06-16T08:03:42Z

homeassistant/components/mpd/media_player.py

-            # Cleanly disconnect in case connection is not in valid state
-            _LOGGER.debug("Error updating status: %s", error)
-            self._disconnect()
+        return self._is_available not in [None, False]


Why do we need do differentiate between None and False, is it just to control debug logs?

Also, I'd suggest to simplify:

return bool(self._is_available)

Or maybe:

return self._is_available is True

Why do we need do differentiate between None and False, is it just to control debug logs?

Correct.

Whenever a device initially goes offline (or is offline when HA starts), a single warning is logged. If the device remains offline, all subsequent failed connection attempts raise a debug message instead.

Personal preference of keeping the logs clear of duplicate messages/clutter... I guess the grouping HA does in its log viewer UI more or less achieves the same – if this unwanted / against conventions let me know.

The requested simplification (return self._is_available is True) was applied.

The distinction between False and None (to control debug logging) remains – pending my remark/question:

Personal preference of keeping the logs clear of duplicate messages/clutter... I guess the grouping HA does in its log viewer UI more or less achieves the same – if this unwanted / against conventions let me know.

I guess it's more a question for @emontnemery - personally I think logging a WARN on the first failure and logging a DEBUG on subsequent failures makes sense to me. Noisy logging is unhelpful, and the code looks like it'll behave as designed.

Yes, warn on first failure and debug after that is fine, it was just not clear to me that's what was happening.

home-assistant · 2023-06-16T08:08:38Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

homeassistant/components/mpd/media_player.py

Rudd-O · 2023-08-03T00:55:39Z

Why not fix the mpd integration so that when there is an exception, it reconnects? This is what I have been doing in my MPRIS integration, with the added safety of a heartbeat plus disconnect and retry when the heartbeat isn't received in the expected time window.

thijsputman · 2023-08-03T08:32:00Z

Why not fix the mpd integration so that when there is an exception, it reconnects? This is what I have been doing in my MPRIS integration, with the added safety of a heartbeat plus disconnect and retry when the heartbeat isn't received in the expected time window.

That's the first thing I tried; never managed to get that to work though: Once the MPD integration and its underlying library start to disagree about the connection state, nothing seems to work any more on my end.

Any command issued from the integration (e.g. "ping", "disconnect") simply results in a "cannot execute this command on a disconnected MPD instance"-response from the library...

If you've got some example code that works properly on your end, feel free to share! I'd love to give it a try here.

Rudd-O · 2023-08-16T23:55:39Z

BTW, you must change uses of asyncio_timeout to asyncio.timeout. The code base is currently migrating to that.

homeassistant/components/mpd/media_player.py

Used to be initialised as None, which caused "NoneType is not iterable" type of issues in case of an unexpected disconnect (at which point status gets set to None again). Instead of guarding against None everywhere, using an empty dictionary seemed more prudent... Furthermore, more cautiously access its members to prevent potential KeyError-s in similar cases.

This method doesn't need a connection; it calls into two other methods that actually connect to MPD – attempting to connect from here resulted in a livelock.

emontnemery

LGTM, thanks @thijsputman 👍

If you want to change how the log level is decided, please open a follow-up PR.

emontnemery · 2023-11-27T17:13:55Z

(Close + Open to rerun CI against the current tip of dev)

thijsputman added 3 commits June 13, 2023 00:32

Update media-player control methods

1c2577c

These all need to explicitly connect to MPD as part of their flow.

home-assistant bot added bugfix cla-signed integration: mpd Quality Scale: No score labels Jun 12, 2023

Correct ruff failure (auto-fixed)

7a422c4

emontnemery requested changes Jun 16, 2023

View reviewed changes

home-assistant bot marked this pull request as draft June 16, 2023 08:08

emontnemery reviewed Jun 16, 2023

View reviewed changes

homeassistant/components/mpd/media_player.py Show resolved Hide resolved

thijsputman added 2 commits June 16, 2023 10:57

Use "async_timeout.timeout" context manager

9d4a6df

Minor changes

5fbfe38

thijsputman marked this pull request as ready for review June 16, 2023 09:11

home-assistant bot requested a review from emontnemery June 16, 2023 09:11

This comment was marked as outdated.

Sign in to view

Merge branch 'dev' into mpd-non-persistent-connection

2e1b30c

Merge branch 'dev' into mpd-non-persistent-connection

4683ac4

Replace "async_timeout" with "asyncio.timeout"

3877241

Rudd-O reviewed Aug 18, 2023

View reviewed changes

homeassistant/components/mpd/media_player.py Show resolved Hide resolved

thijsputman added 3 commits August 24, 2023 12:05

Merge branch 'dev' into mpd-non-persistent-connection

47c46a0

Fix livelock in "async_mute_volume()"

3901e92

This method doesn't need a connection; it calls into two other methods that actually connect to MPD – attempting to connect from here resulted in a livelock.

This comment was marked as off-topic.

Sign in to view

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

This comment was marked as off-topic.

Sign in to view

emontnemery closed this Nov 27, 2023

emontnemery reopened this Nov 27, 2023

emontnemery approved these changes Nov 27, 2023

View reviewed changes

emontnemery merged commit 360ef89 into home-assistant:dev Nov 27, 2023
32 checks passed

thijsputman deleted the mpd-non-persistent-connection branch November 27, 2023 17:47

github-actions bot locked and limited conversation to collaborators Nov 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use non-persistent connection for MPD #94507

Use non-persistent connection for MPD #94507

thijsputman commented Jun 12, 2023

emontnemery left a comment

emontnemery Jun 16, 2023

thijsputman Jun 16, 2023

This comment was marked as off-topic.

thijsputman Aug 15, 2023

kaitlinsm Aug 16, 2023

This comment was marked as off-topic.

emontnemery Nov 27, 2023

home-assistant bot commented Jun 16, 2023

This comment was marked as outdated.

Rudd-O commented Aug 3, 2023

thijsputman commented Aug 3, 2023

Rudd-O commented Aug 16, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

emontnemery left a comment

emontnemery commented Nov 27, 2023

Use non-persistent connection for MPD #94507

Use non-persistent connection for MPD #94507

Conversation

thijsputman commented Jun 12, 2023

Proposed change

Type of change

Additional information

Checklist

emontnemery left a comment

Choose a reason for hiding this comment

emontnemery Jun 16, 2023

Choose a reason for hiding this comment

thijsputman Jun 16, 2023

Choose a reason for hiding this comment

This comment was marked as off-topic.

thijsputman Aug 15, 2023

Choose a reason for hiding this comment

kaitlinsm Aug 16, 2023

Choose a reason for hiding this comment

This comment was marked as off-topic.

emontnemery Nov 27, 2023

Choose a reason for hiding this comment

home-assistant bot commented Jun 16, 2023

This comment was marked as outdated.

Rudd-O commented Aug 3, 2023

thijsputman commented Aug 3, 2023

Rudd-O commented Aug 16, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

emontnemery left a comment

Choose a reason for hiding this comment

emontnemery commented Nov 27, 2023