BrokenPipeError (?) when idle for a long time about python-mpd2 HOT 20 CLOSED

Hello TomTom0815

I use this library for my Project FlushFM - a webradio on my toilet that starts playing every time the light goes on.

It's working fine for a more than a year now, so chances are that my pull request will work for you too.

I wish you (and your daughter) the best :)

from python-mpd2.

Hi @speendo, I saw it. It might work for your case, but it does not fix the broken disconnect method and the python version handling seems overly complicated.

Let's assume the connection is broken and disconnect is called. Then we end up with a borked MPDClient as before.

In my opinion, disconnect has to be changed as I proposed (or in a similar way). Then a broken connection is indicated by an exception and one can recover from this state by disconnecting and connecting again. Clean, straight forward and it works in either case. One may discuss that a unification of mpd.ConnectionError and ConnectionError makes sense, so that there is only one type of exception to be caught, which indicates a problem with the underlying connection.

from python-mpd2.

merged in #92

from python-mpd2.

Duplicate of #31

from python-mpd2.

Hello Jörg!

I'm aware of #31 and ConnectionErrors.

Prove me wrong, but I don't see how this issue is related to that.

My script should catch all ConnectionErrors with

try:
    client.play()
except MPDConnectionError:
    logging.info("Lost MPD connection")
    client.connect()
    client.play()

Please find the whole script here: https://gist.github.com/speendo/0d9c1a028d045de3f7a6

After a couple of hours of idle time I get a (generic) BrokenPipeError which - as far as I know - is not related to the mpd-specific ConnectionError.

I would be glad if you could clarify why this is a duplicate or reopen the issue.

Thank you!

from python-mpd2.

ok. take a look at it.

from python-mpd2.

Thank you!

from python-mpd2.

The problem here is, that mpd does not mask any exception thrown at all, which are generated by the layers below. Sorry this was not my invention. The alternative is exception chaining (catching the error and rethrow an MPDError inheriting the original one). Python 2 does not support this. However it is possible to a custom reference in the exeption to its root cause.

from python-mpd2.

Okay, thank you. Took me some time but I think I understand the problem now.

I use Python3 anyway but I understand that this is not a solution you would want to offer only for Python3 users...

I would like to avoid catching all Exceptions and do something like

try:
    client.play()
except Exception:
    logging.info("Some random exception occured")
    client.connect()
    client.play()

as this is considered as bad style... Is there a way to be more specific - maybe using the custom reference?

from python-mpd2.

Wouldn't this be a possible solution?
http://stackoverflow.com/questions/1350671/inner-exception-with-traceback-in-python

[edit]
more to find here:
http://www.ianbicking.org/blog/2007/09/re-raising-exceptions.html

from python-mpd2.

Just for testing purposes, I tried to catch every exception and reconnect to the server, if I catch one. Like this:

try:
    client.play()
except Exception:
    logging.info("Some random exception occured")
    client.connect()
    client.play()

(find a full example here: https://gist.github.com/speendo/91de6511443e75f18fed)

This also didn't work because the client still thinks it is connected. I get this error:

Traceback (most recent call last):
  File "test_mpd.py", line 63, in <module>
    client.play()
  File "/usr/local/lib/python3.4/dist-packages/mpd.py", line 600, in decorator
    return wrapper(self, name, args, bound_decorator(self, returnValue))
  File "/usr/local/lib/python3.4/dist-packages/mpd.py", line 236, in _execute
    self._write_command(command, args)
  File "/usr/local/lib/python3.4/dist-packages/mpd.py", line 263, in _write_command
    self._write_line(" ".join(parts))
  File "/usr/local/lib/python3.4/dist-packages/mpd.py", line 243, in _write_line
    self._wfile.flush()
  File "/usr/lib/python3.4/socket.py", line 391, in write
    return self._sock.send(b)
BrokenPipeError: [Errno 32] Broken pipe

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "test_mpd.py", line 66, in <module>
     client_connect()
  File "test_mpd.py", line 25, in client_connect
    client.connect("localhost", 6600)
  File "/usr/local/lib/python3.4/dist-packages/mpd.py", line 507, in connect
    raise ConnectionError("Already connected")
mpd.ConnectionError: Already connected

As far as I understand this, this means I would have to close the (broken) connection first before I could reestablish the client's connection, right?

This would be a bit akward. I am only a hobby programmer and not an expert, but from my point of view I think that python-mpd2 should catch the BrokenPipeError and do something about it - either set the connection closed or reestablish the connection.

If the connection is not reestablished for me it would feel more natural if my script would fire a MPD.ConnectionError instead of a "generic" BrokenPipeError.

However, do you know of a way to get the BrokenPipeError more quickly? Because every test I make takes me 4 hours which is quite a lot for someone who can only code in his free time ;-)

By the way:

Schöne Weihnachten!

from python-mpd2.

To repro the BrokenPipeError, assuming you have a mpd server running on localhost on port 6600:

from mpd import MPDClient

client = MPDClient()
client.connect("localhost", 6600)
client.close()
client.ping() # first ping returns with no error, but second one...
client.ping()
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "/home/user/.virtualenvs/app/lib/python3.6/site-packages/mpd.py", line 629, in decorator
    return wrapper(self, name, args, bound_decorator(self, returnValue))
  File "/home/user/.virtualenvs/app/lib/python3.6/site-packages/mpd.py", line 252, in _execute
    self._write_command(command, args)
  File "/home/user/.virtualenvs/app/lib/python3.6/site-packages/mpd.py", line 279, in _write_command
    self._write_line(" ".join(parts))
  File "/home/user/.virtualenvs/app/lib/python3.6/site-packages/mpd.py", line 259, in _write_line
    self._wfile.flush()
  File "/usr/lib64/python3.6/socket.py", line 604, in write
    return self._sock.send(b)
BrokenPipeError: [Errno 32] Broken pipe

As stated above, one cannot call a client.connect(...) to remedy the situation as the client._sock object exists, and mpd thus believes it is still connected.

To handle this case I am catching BrokenPipeError and then calling a client._reset(), which sets the _sock object to None. From here I can attempt to reconnect using client.connect(...)

from python-mpd2.

Hi.

I'm running into this issue as well. After some hours, the connection get dropped and get a socket errors all over the place. It only happens after some hours.

Sad story: I build a device to play music for my little daughter (4 years) - it worked great for many months until I switched to python-mpd2. She goes to bed and listens to music and as soon she wakes up she like to hear her favorite audio book. So, now she wakes me up at 5am and I need to reboot the device. :(

I reworked the connection/re-connection and expectation handling many times (takes one day turn-around for each test) but always ended up with a new exception or fail.
I think the merge request #65 from Speendo solves the issue. Protect the write() and flush() to the sockets, catch socket.error and do a reset() on the socket will solve the issues with the re-connect when I discover that the mpd command didn’t succeeded. Right now, the re-connect fails with “Already connected” or more socket errors.

I’m going to test this merge request locally in the next day. But could you please revive the discussions around this merge request and drive that forward. I would really appreciate a fix for this.

from python-mpd2.

I had the same problem and solved/worked around it by calling _reset() on BrokenPipeError and then reconnect.

I think disconnect() should be modified to be able to deal with dead, already closed sockets correctly.

from python-mpd2.

Maybe something like:

    def disconnect(self):
        logger.info("Calling MPD disconnect()")
        try:
            if (self._rfile is not None
                    and not isinstance(self._rfile, _NotConnected)):
                self._rfile.close()
            if (self._wfile is not None
                    and not isinstance(self._wfile, _NotConnected)):
                self._wfile.close()
            if self._sock is not None:
                self._sock.close()
        finally:
            self._reset()

So it will still raise its exception, but afterwards the mpd instance is able to reconnect.

from python-mpd2.

I find it quite astonishing, that the library does not handle the BrokenPipeError correctly, because it is something very common, when dealing with sockets. The default connection_timeout of mpd is 60 seconds. This means, after 60s mpd closes the connection, when no data was sent. No need to wait for 4 hours. It's 100% reproducible. Connect, wait more than 60s, and send something. You get a BrokenPipeError and an unusable MPDClient.

The problem is, that disconnect tries to close _wfile. This forces a flush of its buffers, which fails, because the socket was closed on the other end. The raised exception prevents _sock.close() and _reset to be called and leaves a borked MPDClient.

I worked around this deficiency by wrapping MPDClient like this

from mpd import MPDClient
from mpd import ConnectionError as MPDConnectionError

class MPDClientWrapper(object):
    def __init__(self, *args, **kwargs):
        self.__dict__['_mpd'] = MPDClient(*args, **kwargs)

    def __getattr__(self, name):
        a = self._mpd.__getattribute__(name)
        if not callable(a): return a

        def b(*args, **kwargs):
            try:
                return a(*args, **kwargs)
            except (MPDConnectionError, ConnectionError) as e:
                cargs, ckwargs = self.__dict__['_connect_args']
                self.connect(*cargs, **ckwargs)
                return a(*args, **kwargs)

        return b

    def __setattr__(self, name, value):
        self._mpd.__setattr__(name, value)

    def connect(self, *args, **kwargs):
        self.__dict__['_connect_args'] = args, kwargs
        self.disconnect()
        self._mpd.connect(*args, **kwargs)

    def disconnect(self):
        try:
            self._mpd.close()
            self._mpd.disconnect()
        except (MPDConnectionError, ConnectionError) as e:
            pass
        finally:
            self._mpd._reset()

1. This makes disconnect never raise a ConnectionError and leaves MPDClient in a clean state.
2. It allows to connect to another server without the need to disconnect explicitly.
3. It automatically reconnects, if the connection died for what ever reason
4. You can easily extend this to and custom methods, i.e. readd volume

from python-mpd2.

Thank you for looking at it!

Did you also see my merge request? At least for me it work very robust and it tries to keep the logic of the original library.

from python-mpd2.

Nothing?

from python-mpd2.

Well, @quantenschaum, as far as I get it, I think you simply follow a different approach. While my solution keeps the logic of the original library (not questioning if it makes sense or not), you solve the problem by changing its behavior.

When using (my version of) python-mpd2, I do also use a wrapper, because I want my mpd client to stay connected. But this wrapper is implemented in my project not in the library.

Nevertheless, your approach also solves the issue and for that I find it much better than the current state of this library.

... however, I guess you wanted @Mic92 to answer your comment, not me...

from python-mpd2.

Yes, of course, the current behavior is leaving a broken MPDClient when the remote end closes the socket. This is a bug and needs to be fixed = changed. One might make the auto reconnect feature optional, but fixing the broken disconnect method is essential.

from python-mpd2.