-
Notifications
You must be signed in to change notification settings - Fork 550
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
discovery::server: fix activeUser field of getInfo #1235
base: dev
Are you sure you want to change the base?
discovery::server: fix activeUser field of getInfo #1235
Conversation
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.
Thanks! Yes this is a known shortcoming, nice to see something being done about it.
That would be nice. Do you want to handle that under this PR or a separate one? |
bump you think you can take a shot at updating the user on disconnect / reconnect? |
Hi, sorry for the delay. I might have some time to take a look at this next week. |
Don't use async Mutexes for short-lived locks like this. Use a normal |
bump @Finomnis is right. Could you change it accordingly? |
@@ -12,7 +12,7 @@ use aes::cipher::{KeyIvInit, StreamCipher}; | |||
use base64::engine::general_purpose::STANDARD as BASE64; | |||
use base64::engine::Engine as _; | |||
use futures_core::Stream; | |||
use futures_util::{FutureExt, TryFutureExt}; | |||
use futures_util::{lock::Mutex, FutureExt, TryFutureExt}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Mutex
from std
instead of async
muteness.
active_user = username.to_string(); | ||
} | ||
let maybe_username = self.username.lock().await; | ||
let active_user = maybe_username.as_deref().unwrap_or(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you comment why the unwrap_or
is "safe" here? Should it always succeed?
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! |
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 TimotheeGerber/spotify-connect#4 (comment).