jkristell / async-mpd

3 stars 3 forks source link

Panic on parsing mpd response #5

Closed xa888s closed 4 years ago

xa888s commented 4 years ago

I am currently writing a music client with this library, and it is really nice to use, thanks! I am running into an issue that whenever a specific song starts playing, it crashes. All other songs seem to work fine. The song is Promiscuous by Nelly Furtado. Tags:

xa888s commented 4 years ago

I have isolated it to line 58 in client.rs

xa888s commented 4 years ago

Adding a dbg! reveals empty string (just realized it already prints it :P)

[/home/abyss/projects/async-mpd/src/client.rs:57] &lines = "volume: 50\nrepeat: 1\nrandom: 0\nsingle: 0\nconsume: 0\nplaylist: 2\nplaylistlength: 3\nmixrampdb: 0.000000\nstate: play\nxfade: 5\nsong: 1\nsongid: 2\ntime: 0:242\nelapsed: 0.000\nbitrate: 332\nduration: 242.293\naudio: 44100:16:2\nnextsong: 2\nnextsongid: 3"
[/home/abyss/projects/async-mpd/src/client.rs:57] &lines = "volume: 50\nrepeat: 1\nrandom: 0\nsingle: 0\nconsume: 0\nplaylist: 2\nplaylistlength: 3\nmixrampdb: 0.000000\nstate: play\nxfade: 5\nsong: 2\nsongid: 3\ntime: 0:64\nelapsed: 0.000\nbitrate: 0\nduration: 64.080\naudio: 44100:16:2\nnextsong: 0\nnextsongid: 1"
[/home/abyss/projects/async-mpd/src/client.rs:57] &lines = ""
jkristell commented 4 years ago

Hi and thanks for the response!

It could be that the server closed the connection? The EOF error seems to indicate that. The mpd server is really eager to disconnect if you don't put the connection in idle state or continuously send it some command. I use to have some code in cmd that checked the connection and reconnected if the server had closed the connection, but it was a bit of a hack so I removed it.

So your options right now is either to put the server in Idle state between commands or open a new connection for each command.

I'm not really sure how to solve this. Ideas are very welcome!

xa888s commented 4 years ago

I was thinking you could get a new connection in the .unwrap_or_else() and re run the command? I think you could also return a new error type to show the user something so they can reset their own connection? Maybe like a Result<Status, MpdError> with one of the variants being Disconnected. Then the user can choose what to do if needed. I think it is fine to provide an error but a panic is very drastic for something that seems like it could happen often? If a panic is required than a section in the docs that warns about this would be important to inform the user of their duties. Can't really think of any better solutions right now but let me know what you think.

jkristell commented 4 years ago

The Result<_, MpdError> make sense. Then the user of the library can handle this the way feel is right and don't have to do the Idle NoIdle dance.

Yeah, to panic is really bad, fully agree with you. I can probably implement this during the week or if you want to do feel free to send a pull request! :-)

jkristell commented 4 years ago

Been reading up on error handling library, will implement this tonight!

jkristell commented 4 years ago

Implemented this and added a new example on how to use it.

But I'm leaning on moving the reconnect step back into the library again to make the library nicer to use.