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

Panic does not exit #226

Closed
maufl opened this issue May 18, 2018 · 13 comments · Fixed by #823
Closed

Panic does not exit #226

maufl opened this issue May 18, 2018 · 13 comments · Fixed by #823
Labels

Comments

@maufl
Copy link

maufl commented May 18, 2018

I noticed that when ever a panic is thrown, librespot stops working completely but does not exit.
This is unfortunate, as it would be restarted by systemd in my setup if it exits.

@plietar
Copy link
Contributor

plietar commented May 18, 2018

Librespot has a few threads, and a panic on one does not always cause others to stop.

You should be able to make all panics into process aborts by adding to Cargo.toml (assuming you're building with --release):

[profile.release]
panic = "abort"

Let me know if that works. This seems like a reasonable thing to do, so I could add it here.

@maufl
Copy link
Author

maufl commented May 18, 2018

Doesn't that also mean that the stack is not unwound and we won't get any stack traces if it panics? I actually added

[profile.release]
debug = true

just to get nice stack traces. (Debug build is not performant enough to smoothly play music)

@maufl
Copy link
Author

maufl commented May 29, 2018

Ok, I tried it. Strangely, now when librespot crashes, it seems to hang with 100% CPU utilization. Also, I don't get a stack trace.

@roderickvd
Copy link
Member

This behaviour is known to me e.g. when the player thread panics. @JasonLG1979 is putting quite a lot of effort in error handling in #823, let's see how that goes.

@roderickvd
Copy link
Member

Can you test and report back now that #823 has been merged?

@uSpike
Copy link

uSpike commented Nov 17, 2021

I find that with v0.3.1 librespot will exit after a crash.

thread 'main' panicked at 'index out of bounds: the len is 0 but the index is 0', connect/src/spirc.rs:1155:29
...
[2021-11-17T14:28:17Z DEBUG librespot_connect::spirc] drop Spirc[0]
[2021-11-17T14:28:17Z DEBUG librespot_playback::player] Shutting down player thread ...
[2021-11-17T14:28:17Z DEBUG librespot_playback::player] drop PlayerInternal[0]
[2021-11-17T14:28:17Z DEBUG librespot_playback::player] PlayerInternal thread finished.
[2021-11-17T14:28:17Z DEBUG librespot_core::session] drop Session[0]
[2021-11-17T14:28:17Z DEBUG librespot_core::mercury] drop MercuryManager
[2021-11-17T14:28:17Z DEBUG librespot_core::session] drop Dispatch
$

@JasonLG1979
Copy link
Contributor

I find that with v0.3.1 librespot will exit after a crash.

That would be happening here:

https://github.com/librespot-org/librespot/blob/dev/connect/src/spirc.rs#L1155

@roderickvd
Copy link
Member

So that means we can close this issue?

@JasonLG1979
Copy link
Contributor

So that means we can close this issue?

No. That means we need to make sure this isn't zero:

https://github.com/librespot-org/librespot/blob/dev/connect/src/spirc.rs#L1141

@JasonLG1979
Copy link
Contributor

We're trying to grab the 1st thing (index 0) from a thing with a length of 0.

@JasonLG1979
Copy link
Contributor

JasonLG1979 commented Nov 17, 2021

I'm not exactly sure of the context or what exactly the thing is but the return type is an option. Provided whatever is asking for it actually checks it and doesn't just unwrap we can just return None if the len is 0.

@uSpike
Copy link

uSpike commented Nov 17, 2021

So that means we can close this issue?

Yes, in my opinion this issue has been solved (not exiting on panic).

So that means we can close this issue?

No. That means we need to make sure this isn't zero:

https://github.com/librespot-org/librespot/blob/dev/connect/src/spirc.rs#L1141

I'd suggest that be opened as a separate issue.

@roderickvd
Copy link
Member

I agree. Thanks to @JasonLG1979 so that we can now close this issue, and @uSpike for reporting back on it. Do file that new issue with steps how to reproduce that.

@roderickvd roderickvd linked a pull request Nov 17, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants