Conversation
Added connection test and connection retry loop.
Fixed absence of login during connection retries.
|
Performed a few more improvements and in my tests this approach totally fixed the issue. The server side socket disconnects are now transparent to home assistant (unless home assistant permanently fails to access the vlc server, in which case it will be marked as unavailable until it is up again). |
|
Hey @teixeluis, hoping to get to this soon. In the meantime, I saw that Home Assistant is actually replacing the Python VLC interface (home-assistant/core#57513) to aiovlc, which seems like it might obsolete |
Thanks for the input, @dmcc Ok..seems like a nice touch, using asyncio in this new client code. From a quick look at the implementation, it isn't clear if and how it handles the server side closing of the socket. It doesn't look like it attempts to connect again, if the socket closes. Not sure if it is a bug or feature, but regardless if we are sending commands (e.g. "status" ) or not, the vlc telnet module always closes the session 90 seconds after it having been established. So it doesn't really matter if home assistant is performing periodic (i.e. every 5 seconds) status queries, the socket will always close on the server side after that fixed amount of time (I haven't tested but maybe if it is playing content this might be different...). |
dmcc
left a comment
dmcc
left a comment
There was a problem hiding this comment.
Thanks Luis! Left some small nitpicks.
| def run_command(self, command): | ||
| return self.do_run_command(command, self.retries) | ||
|
|
||
| def do_send_string(self, string): |
There was a problem hiding this comment.
Nit: "string" is also the name of a Python module -- maybe "message" is a better name here?
| # Return the split output of the command | ||
| return command_output | ||
|
|
||
| def do_run_command(self, command, retries): |
There was a problem hiding this comment.
Nit: Add a short doc string?
| except Exception: | ||
| return False | ||
|
|
||
| def run_command(self, command): |
There was a problem hiding this comment.
Nit: Could we combine run_command and do_run_command? I think we can wrap much of do_run_command in for _ in range(self.retries): ...
Alternatively, maybe_reconnect_and_run_command might be a clearer name for do_run_command?
| self.disconnect() | ||
| time.sleep(1) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Do we want to log the exception here? (or at least that there was one?) If this is a thing that happens commonly, maybe we can tighten it up to the specific Exception that happens here.
(same for line 153)
@MartinHjelmare Do you know if it would be possible to add reconnection support to |
|
I'd suggest to make sure we raise an exception in the library when the connection is not ok, and let the library user handle the reconnection. The library user calls connect so we should let them handle reconnecting too. |
3 participants
This hotfix is intended to fix the issue reported in:
home-assistant/core#29184
The approach consists of performing a connection test by sending a CRLF to the vlc server and expecting the prompt, prior to attempting to send the command itself.
Whenever the test fails, the connection is retried, using the appropriate method.
I have tested the fix in my Home Assistant Core instance (core-2021.9.7) with success against another device running the VLC daemon (VLC media player 3.0.11) with telnet enabled.
I can now call the media player repeatedly without issues. At regular intervals the media player does become unavailable (I'm guessing due to the VLC closing the session - this can be observed when connecting via the command line), but the client will automatically reconnect. Before the fix, once the BrokenPipe error would occur, the session would never be recovered.