librespot icon indicating copy to clipboard operation
librespot copied to clipboard

refactor: remove parking_lot dependency and refine feature selections

Open roderickvd opened this issue 4 months ago • 3 comments

  • Trim Cargo.toml feature selections
  • Remove parking_lot dependency across the entire workspace in favor of standard library synchronization primitives

Trimming feature selections

I went through the dependency tree to apply default-features = false and trimming feature selections where possible.

Removing parking_lot

Some years ago I moved from std::sync::Mutex and friends to parking_lot when I was purging all use of unwrap and expect from the code base. Since then two things happened:

  1. Performance of std synchronization primitives got a lot better;
  2. I got wiser and now understand that when mutexes get poisoned, it's a good thing to propagate that error and panic the calling thread too. This is why we now use expect again here.

Fixes #1382

roderickvd avatar Aug 20 '25 10:08 roderickvd

I didn't look fully through it, yet. But what is the behavior when the Mutex gets poisoned, did you test/encounter that, yet?

photovoltex avatar Aug 20 '25 11:08 photovoltex

I didn’t test it. But you could trigger it by panicking while holding a lock. Which shouldn’t happen.

Edit: what will happen is that the thread trying to acquire the lock will then also panic. Purpose being to indeed have librespot exit rather than continue in a poisoned state.

roderickvd avatar Aug 20 '25 11:08 roderickvd

So just to be sure if I understand it correct. If we encounter a poisoned lock, we don't want to continue as it can cause undefined behavior. This will probably not happen unless some access occurs to the lock while the thread of the active lock panics, in which case we already have a panic and just want to stop the program as a result. So the panic of the lock would just be a result of another error/panic, right?

Exactly.

roderickvd avatar Aug 20 '25 13:08 roderickvd

So whats the state of things here @roderickvd? Could you resolve the conflicts and the review comment if everything is ready to merge?

photovoltex avatar Sep 13 '25 19:09 photovoltex

Yeah if you guys are OK with it then I can do that.

roderickvd avatar Sep 13 '25 21:09 roderickvd

I'm not sure if this fully fixes #1382, but besides that this sounds seems like a good thing to do. @kingosticks anything from your side that speaks against these changes?

photovoltex avatar Sep 14 '25 20:09 photovoltex

Looks good to me.

kingosticks avatar Sep 15 '25 22:09 kingosticks