librespot icon indicating copy to clipboard operation
librespot copied to clipboard

discovery::server: fix activeUser field of getInfo

Open dsheets opened this issue 1 year ago • 6 comments
trafficstars

The current active user was almost persisted and reported but wasn't. The handler for the discovery server is under an Arc so I introduced a Mutex to protect the username field and made setting the username and sending the credentials atomic so that every valid credential send corresponds to an active user update.

There doesn't appear currently to be any way for this activeUser field to be reset back to the empty string when a user disconnects so one could argue that it's actually more broken now than before as it will keep reporting the last user to connect indefinitely instead of the user with a currently active session.

If you're interested in this fix, I may take a look at clearing the value in the future.

This issue was reported by @TimotheeGerber in https://github.com/TimotheeGerber/spotify-connect/pull/4#discussion_r1428834453.

dsheets avatar Dec 17 '23 22:12 dsheets

Thanks! Yes this is a known shortcoming, nice to see something being done about it.

If you're interested in this fix, I may take a look at clearing the value in the future.

That would be nice. Do you want to handle that under this PR or a separate one?

roderickvd avatar Dec 20 '23 20:12 roderickvd

bump you think you can take a shot at updating the user on disconnect / reconnect?

roderickvd avatar Jan 03 '24 19:01 roderickvd

Hi, sorry for the delay. I might have some time to take a look at this next week.

dsheets avatar Jan 05 '24 13:01 dsheets

Don't use async Mutexes for short-lived locks like this. Use a normal std mutex instead. See this section of the tokio book for more information.

Finomnis avatar Feb 08 '24 21:02 Finomnis

bump @Finomnis is right. Could you change it accordingly?

roderickvd avatar Mar 31 '24 09:03 roderickvd

Nudging @dsheets - this would be a very worthwhile addition and only requires a bit of work... could you muster it and add to the changelog? Then we can merge!

roderickvd avatar Aug 21 '24 19:08 roderickvd