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

Log error instead of panicking when child fails to start #684

Merged
merged 2 commits into from
May 1, 2021

Conversation

roderickvd
Copy link
Member

librespot currently panics when onevent programs fail to start, which seems a bit harsh -- there's nothing in the way of resuming music streaming. This PR handles this gracefully.

@sashahilton00
Copy link
Member

I believe we decided to make it panic originally as we assumed a valid, functioning program as a prerequisite, hence the panic meant that any error caused an immediate failure as opposed to continuing regardless. I'm indifferent as to whether it logs or panics, I can see arguments for both.

@Johannesd3
Copy link
Contributor

IMHO, Rust programs should never panic. Panics are actually meant for logic errors, i.e. programmer's mistakes. The proper way is to propagate the error down to the main function, and terminate the program printing an error message.

@sashahilton00
Copy link
Member

That's fair enough. Given that it spits out an error if the script fails, I think we can probably change this behaviour as I agree, panicking does feel heavy handed.

@sashahilton00 sashahilton00 merged commit 8973d29 into librespot-org:dev May 1, 2021
@roderickvd roderickvd deleted the fix-onevent-panic branch May 1, 2021 10:07
@roderickvd
Copy link
Member Author

IMHO, Rust programs should never panic. Panics are actually meant for logic errors, i.e. programmer's mistakes. The proper way is to propagate the error down to the main function, and terminate the program printing an error message.

Is this we want to improve in general? There is a lot of .expect() and .unwrap() going on for operations that may well fail (e.g. incorrect command line parameters, failure to open an audio backend) that could be made nicer. It's not high priority but I'd be willing to invest some time in it.

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 this pull request may close these issues.

3 participants