4br3mm0rd / mpyg321

mpg321 wrapper for python - command line mp3 player
MIT License
22 stars 5 forks source link

Handle pexpect.exceptions.EOF #25

Closed stefets closed 5 months ago

stefets commented 2 years ago

Hi @4br3mm0rd !

If I create an instance of the player and my USB sound card is not connected I crash because of pexpect. I suggest to handle EOF in consts.py in the same way of TIMEOUT. If not, this problem will have to be handled in every class that inherit the BasePlayer

pexpect.exceptions.EOF: End Of File (EOF). Exception style platform. <pexpect.pty_spawn.spawn object at 0x75f89870> command: /usr/local/bin/mpg123 args: ['/usr/local/bin/mpg123', '--remote'] buffer (last 100 chars): b'' before (last 100 chars): b'in/mpg123: symbol lookup error: /usr/local/bin/mpg123: undefined symbol: mpg123_getpar2\r\nVOLUME 10\r\n'

UPDATE 2023 : The mpg123_getpar2 issue is when mpg123 has not the correct reference to .so object.

4br3mm0rd commented 2 years ago

Hey @stefets

Are you running on MacOS? This sounds like this issue and indeed we can fix it by adding pexpect.EOF to the consts. I'll do it in the upcoming days :)

stefets commented 2 years ago

Hi! Not in MacOS, it happen on Linux on the Raspbian OS.

But if we add pexpect.EOF this mean that the player is in an inoperate state and I think we should throw a custom error, as a suggestion. Please, wait a little before working on this issue. I want to investigate a little more and check a couple of things.

Thanks!

stefets commented 2 years ago

I was unable to reproduce - probably a bad usage on my side; I close the issue for now but will reopen if occurs again and if I can reproduce the issue.

4br3mm0rd commented 2 years ago

@stefets I think it could actually be a cool feature: EOF means in general that mpg123 crashed somehow. It could be cool to have an event "onMpgExit" that would be triggered when pexpect meets an EOF. For example, we could choose to run a new mpg123 process inside the same Mpg123Player object. We could even check the player's status, and, if the player was playing, we would replay the song :)

stefets commented 2 years ago

Hi @4br3mm0rd,

Yes, it will be an advantage to add the pexpect.EOF to the consts file and handling the EOF gracefully. We have to reproduce the EOF crash first I think. At this moment, I don't know how will be the mpg123 wrapper's behavior if it is dead.

Suggestion : If EOF occurs, try to restart the player only once and call a function for instance pause; if EOF occurs again, throw an exception or set the player in an 'shutdown state' and unusable. It's important to take care that If the player is dead, I don't want to see a bunch of errors when I call volume, pause, play and other functions.

What do you think ?

4br3mm0rd commented 2 years ago

Suggestion : If EOF occurs, try to restart the player only once and call a function for instance pause; if EOF occurs again, throw an exception or set the player in an 'shutdown state' and unusable. It's important to take care that If the player is dead, I don't want to see a bunch of errors when I call volume, pause, play and other functions.

I'm not sure that EOF can happen twice, since EOF means that we reached the end of stdout (I don't know any other case than the end of the program for this to happen). But I agree that if we cannot restart we should put the object in a crashed mode.

stefets commented 1 year ago

Hi, I'm reopening the issue.

At the beginning, the EOF occurred because my mpg123 instance was corrupted after a system update.

The EOF that occurs now is because of the --audiodevice parameter.

My application now support an infinite number of USB audio device and if I have one card not connected I have the EOF error.

This crash is normal but it is not handled in the wrapper.

My strategy is to catch the pexpect.EOF and set the problematic instance in a 'safe do nothing zombie mode' until the process is disposed by it's host. I hope push a PR in a couple of days so if you have suggestions let me know!

stefets commented 1 year ago

Use case of calling mpg123 with an unavailable audiodevice, exit code = 255

(python3.11) pi@rpi4b:~/src/live-config/src/plugins $ mpg123 --audiodevice GT10B -R ; echo "exit code= $?"
Cannot connect to server socket err = No such file or directory
Cannot connect to server request channel
jack server is not running or cannot be started
JackShmReadWritePtr::~JackShmReadWritePtr - Init not done for -1, skipping unlock
JackShmReadWritePtr::~JackShmReadWritePtr - Init not done for -1, skipping unlock
main: [src/mpg123.c:check_fatal_output():334] error: out123 error 3: failure loading driver module
exit code=255
4br3mm0rd commented 1 year ago

Hey @stefets thanks for reporting!

Is there a better behavior that we could think of? Like showing the error message ("driver not loaded"), asking to press enter when plugged and then reopening the process?

This could be good for Ux but we might not want to force this behavior (as a library we don't want to interrupt the Ux), so we could set this as an option when creating the object.

What do you think?

stefets commented 1 year ago

Hi @4br3mm0rd !

When pexpect launches the EOF it means that the mpg123 process does not exist. The wrapper is tightly coupled to the mpg123 process and if the latter does not exist I believe it must throw a fatal and unrecoverable error.

It's currently not possible to handle this in the UX layer because exceptions are thrown in the output_processor Thread, even the main Thread doesn't handle and know an exception is happening, and I think this part should be refactored to make it easier how to escalate the error to the UX layer. Should exceptions be thrown outside the output_processor Thread?

I am testing in my fork and you can review my change in branch pexpect-eof-issue-25

I await your comments thank you!

4br3mm0rd commented 1 year ago

Sorry for the late answer I was on vacation.

I believe you are right. I actually didn't like so much the idea of interacting with the Ux from the beginning so the best is to just throw an exception :-)

stefets commented 1 year ago

Hey no problem! I think so. But I'm checking on how to communicate with sub-thread. I have a couple of ideas in mind.

4br3mm0rd commented 1 year ago

Hi @stefets how are you? Any news on this topic? Do you want to do a PR to include it?

stefets commented 1 year ago

Hi @4br3mm0rd! I feel great!, hope you too?

I'm still thinking and reading for the simplest solution to handle this issue.

Next step is to try something like the code in this article.

https://www.geeksforgeeks.org/handling-a-threads-exception-in-the-caller-thread-in-python/

I'll be back with the results!

4br3mm0rd commented 1 year ago

I don't think that the exception comes from the child process. The exception is thrown by pexpect because the child process reached EOF. So I believe that you could easily solve this by putting the pexpect.spawn inside a try/except. I'll try something like that in the coming days (although not sure I can reproduce the eof bug)

stefets commented 1 year ago

Hi, sorry for the delay, the try except is something I had tested; I tried again today and I pushed a change to my test branch with a try except and I never enter the except for EOF; it drives me crazy.

  File "/home/pi/.venv/python3.11/lib/python3.11/site-packages/pexpect/expect.py", line 122, in eof
    raise exc
pexpect.exceptions.EOF: End Of File (EOF). Exception style platform.
<pexpect.pty_spawn.spawn object at 0x7f8c00f110>
command: /usr/local/bin/mpg123
args: ['/usr/local/bin/mpg123', '--audiodevice', 'SD90', '-R', 'mpyg']
stefets commented 5 months ago

Never found any solution on my test; Hoping that an event could be emit if #42 is implemented