i3status-rust icon indicating copy to clipboard operation
i3status-rust copied to clipboard

Reconnection to pulseaudio

Open l4l opened this issue 5 years ago • 6 comments

How to reproduce: run i3status-rust with the following config and then run pulseaudio -k.

[[block]]
block = "sound"

Pulseaudio is restarted (since it a normal behavior for a daemon), but no reconnection is made. I don't really know, is it a significant issue, since it can be fixed with an i3 restarting. But still no notes that something went wrong.

l4l avatar Feb 23 '19 10:02 l4l

The pulse-audio connection is already pretty decoupled from the UI, so it shouldn't be hard to make it reconnect on error.

I am unfortunately still in the baby-step stage with rust, so I was not able to come to an agreement with the borrow checker with regards to my first attempt...

kennylevinsen avatar Feb 27 '19 12:02 kennylevinsen

This seems like a good feature to have. @kennylevinsen can you give me an idea of the errors you are encountering?

Also, cc'ing @SWW13, the original author of that block.

atheriel avatar Mar 05 '19 21:03 atheriel

I had a short look into it, we currently neither catch a broken pulseaudio connection nor have a functional reconnect.

The failed connection is only check once during connection in: https://github.com/greshake/i3status-rust/blob/b3146bcbdbcca79fd1daba675205324284f90cbd/src/blocks/sound.rs#L242-L250 Maybe a check could be added in the iteration: https://github.com/greshake/i3status-rust/blob/b3146bcbdbcca79fd1daba675205324284f90cbd/src/blocks/sound.rs#L258-L269 A new connection probably needs a new PulseAudioClient, so this could be a bit tricky.

If someone wants to work on it, the initial PR https://github.com/greshake/i3status-rust/pull/245 has some information on the pulseaudio implementation and I can also be highlighted here.

SWW13 avatar Mar 06 '19 01:03 SWW13

@atheriel As PulseAudioClient simply passes messages between its users and a PulseAudioConnection using channels, my idea was simply to make PulseAudioClient recreate PulseAudioConnections as they die.

I tried two naïve approaches:

  1. A simple loop that starts out by establishing the connection, followed by servicing it until it breaks. This gave complaints about mutable borrows (setting up the connection at the start of the loop requires calling mut &self methods on the connection) clash with the borrows from the previous loop iteration (servicing the connection). This was quite the "wtf" moment.
  2. A PulseAudioConnMgr with a "get" method that either retrieves the current conn if functional, or creates a new one. However, this design ends up needing to return a mutable reference to the inner value of an option struct field (current conn state), which the burrow checker certainly dislikes (unwrap does not live long enough).

After this, I simply agreed to disagree with the borrow-checker and went on to do other, more agreeable things. I wouldn't mind a solution, though.

kennylevinsen avatar Mar 06 '19 08:03 kennylevinsen

@kennylevinsen I haven't dig into the code much, but you particular issue might be solved with a runtime borrowing via RefCell wrapper. Though, you should be careful with it, since in multi-threaded environment it may panic. So if you cannot guarantee a proper borrowing you should use RwLock, or better Mutex.

l4l avatar Mar 06 '19 19:03 l4l

@l4l @kennylevinsen We indeed use the (very tricky, very un-Rust-like) PulseAudio API in multi-threaded environment, which is probably why the compiler is complaining. It is likely correct. We will probably need to do some amount of copying (via .clone()) to make this work properly.

Unfortunately, after taking a look at the code myself, I'm inclined to agree with @SWW13 -- implementing this change will probably be a hard and painful experience. This is certainly one of (if not the most) complex parts of the codebase. To whomever wants to take this on... I commend you.

atheriel avatar Mar 06 '19 19:03 atheriel