Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BrokenPipeError (?) when idle for a long time #64

Closed
speendo opened this issue Dec 15, 2015 · 20 comments
Closed

BrokenPipeError (?) when idle for a long time #64

speendo opened this issue Dec 15, 2015 · 20 comments

Comments

@speendo
Copy link

speendo commented Dec 15, 2015

Hi and thank you for this project!

When mpd is idle (after calling client.stop()) for a couple of hours, I cannot successfully call client.play() again. I am aware of ConnectionErrors that can occur so I catch them in my script. In this case, however, I get a BrokenPipeError (sometimes I don't get an error at all, but nevertheless mpd won't start to play reproducibly).

I wrote a test script (find it here: https://gist.github.com/speendo/0d9c1a028d045de3f7a6) to track the error down.

I ran this script on my raspberry pi with nohup python3 test_mpd.py &.

Here's a traceback of the Error (found in nohup.out):

Traceback (most recent call last):
  File "test_mpd.py", line 62, 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

And here's the log:

2015-12-15 08:35:26,497 INFO: Log started
2015-12-15 08:35:26,500 INFO: Attempt to (re-)connect to mpd server
2015-12-15 08:35:26,503 INFO: Calling MPD connect('localhost', 6600, timeout=None)
2015-12-15 08:35:26,566 INFO: Finished attempt to (re-)connect to mpd server
2015-12-15 08:35:26,568 INFO: <bound method MPDClient.decorator of <mpd.MPDClient object at 0xb6a38590>>
2015-12-15 08:35:26,570 INFO: Add playlist entry
2015-12-15 08:35:26,573 DEBUG: Calling MPD clear()
2015-12-15 08:35:26,578 DEBUG: Calling MPD add('http://uwstream2.somafm.com:9016',)
2015-12-15 08:35:26,582 INFO: Start playing for the first time
2015-12-15 08:35:26,585 DEBUG: Calling MPD play()
2015-12-15 08:35:36,597 INFO: This obviously works. Switching off now for 4 hours.
2015-12-15 08:35:36,604 DEBUG: Calling MPD stop()
2015-12-15 12:35:36,725 INFO: Trying to switch on again now
2015-12-15 12:35:36,727 DEBUG: Calling MPD play()

Can you help me find the source of this problem (or even solve it)?

@Mic92
Copy link
Owner

Mic92 commented Dec 16, 2015

Duplicate of #31

@Mic92 Mic92 closed this as completed Dec 16, 2015
@speendo
Copy link
Author

speendo commented Dec 16, 2015

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!

@Mic92
Copy link
Owner

Mic92 commented Dec 16, 2015

ok. take a look at it.

@Mic92 Mic92 reopened this Dec 16, 2015
@speendo
Copy link
Author

speendo commented Dec 16, 2015

Thank you!

@Mic92
Copy link
Owner

Mic92 commented Dec 16, 2015

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.

@speendo
Copy link
Author

speendo commented Dec 16, 2015

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?

@speendo
Copy link
Author

speendo commented Dec 16, 2015

@speendo
Copy link
Author

speendo commented Dec 24, 2015

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!

@dinofizz
Copy link

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(...)

@TomTom0815
Copy link

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.

@speendo
Copy link
Author

speendo commented Mar 14, 2017

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 :)

@quantenschaum
Copy link

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.

@quantenschaum
Copy link

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.

@quantenschaum
Copy link

quantenschaum commented Sep 24, 2017

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

@speendo
Copy link
Author

speendo commented Sep 24, 2017

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.

@quantenschaum
Copy link

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.

@quantenschaum
Copy link

Nothing?

@speendo
Copy link
Author

speendo commented Oct 20, 2017

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...

@quantenschaum
Copy link

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.

@Mic92
Copy link
Owner

Mic92 commented Feb 20, 2018

merged in #92

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants