-
Notifications
You must be signed in to change notification settings - Fork 14
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
Signal clients on friend presence state changes #252
Conversation
using (var db = databaseFactory.GetInstance()) | ||
{ | ||
foreach (var friend in await db.GetUserFriends(state.UserId)) | ||
await Groups.AddToGroupAsync(Context.ConnectionId, friend_presence_watchers(friend.zebra_id)); |
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.
Hmm, as discussed IRL there's two other ways we could go about this, and I'm not sure which is best.
- The way this is implemented, 1000 users connecting with 1000 friends each will be creating 1M channels. The overhead of broadcast friend presence updates is low, but the initial connect overhead is high.
- An alternative would be doing the inverse – connecting user registers against a "notify-me" channel and when a friend connects, a ping would be sent to each friend. This could reduce the channel overhead to 1000 in the above scenario, but on connect we would need to iterate all friends and send messages out (slightly higher db overhead, but probably file). If we use this method, broadcasting further updates (ie. if we want to convey activity state changes) would not be possible using the new channel subscriptions.
- The way bancho did this was to send the friends list to the client, and then broadcast all online/offline changes and leave everything to the client. I'd lean towards this method – because users are wanting to see the dashboard show all online users like stable does – but I'm not sure about the performance overheads of having such a large number of SignalR broadcasts in the background (especially since it won't stop during gameplay).
I think I'm leaning towards what you have here as an initial implementation since it's simple to get out and if it's going to fall over, we'll know pretty quickly (server-spectator
will probably hit memory or performance issues on a sizable number of users connecting).
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.
@bdach this one is still in early discussion but you may have something to say here.
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.
I think I'd start with the same structure as this is doing. The 1M figure is scary but it is a worst-case figure in no less than two aspects: it assumes that everyone has 1k friends, which they won't, and it also assumes that everyone's set of friends is completely disjoint, which I also don't think it will be (although maybe not to a degree that would matter). That said this 100% will be the heaviest usage of groups we have done to date so it'll be interesting to see if things hold up. The primary indicator of doom would be runaway memory usage.
In terms of observability, a ddog gauge on the number of active groups could prove useful.
Also, something like https://stackoverflow.com/questions/12651169/signalr-and-large-number-of-groups (not sure how up-to-date it is) may prove to be an annoying snag.
because users are wanting to see the dashboard show all online users like stable does
I'm confused by this part however, because we already have the dashboard working for all online users, and it works because users implicitly opt into the broadcasts by opening the dashboard (and opt out of them by closing it).
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.
I'm confused by this part however, because we already have the dashboard working for all online users, and it works because users implicitly opt into the broadcasts by opening the dashboard
in stable, users are shown regardless of whether they are playing a beatmap or not. we're eventually going to want this to be a thing here too.
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.
in stable, users are shown regardless of whether they are playing a beatmap or not
I am further confused because again, to the best of my knowledge, that's what we already have on lazer...?
foreach (var userState in GetAllStates()) | |
{ | |
if (userState.Value.UserStatus != UserStatus.Offline) | |
await Clients.Caller.UserPresenceUpdated(userState.Value.UserId, userState.Value.ToUserPresence()); | |
} |
GetAllStates()
here is going to list all users connected to the metadata hub, which has nothing to do with whether they're playing beatmaps or not.
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.
Wow, I guess it is. I was sure it was still only for actively playing.
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.
In terms of observability, a ddog gauge on the number of active groups could prove useful.
This doesn't look easy to do, because SignalR itself doesn't expose any tracking variable. It looks like you can override Hub.Groups
with a custom IGroupManager
which could implement our own tracking, but it sounds difficult to do properly - have to consider SignalR's automatic cleanup on disconnection, SignalR not telling us the groups which a user's in, concurrency, etc...
Also, something like https://stackoverflow.com/questions/12651169/signalr-and-large-number-of-groups (not sure how up-to-date it is) may prove to be an annoying snag.
This looks to be more users with many groups, rather than groups with many users or many groups in general. While not super applicable to us anyway, I'm crossing my fingers it won't ever become an issue - the only reference I can find is this connection.GroupNames
member that doesn't look to be serialised anywhere.
e1fb75f
to
7b80ada
Compare
Superseded by #255 |
Prereqs: