-
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
Always broadcast user presence to users' friends #255
Merged
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
fdb6cda
Always broadcast presence changes to friends
smoogipoo c53350e
Don't watch restricted users
smoogipoo d7b37a8
Fix test
smoogipoo 8bcc697
Signal initial online state
smoogipoo ff090ab
Add test
smoogipoo 1534716
Refactor test a bit
smoogipoo c122815
Remove unnecessary removal from group on disconnect
smoogipoo bca0907
Don't set/broadcast initial online state
smoogipoo 483a77a
Merge branch 'master' into friend-presence-2
smoogipoo fbdf591
Broadcast initial friend status on connect
smoogipoo a031daa
Fix improper use of entity states
smoogipoo d74dfa8
Broadcast friend presence via separate event
smoogipoo 83f5408
Update packages
peppy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,7 @@ public class MetadataHub : StatefulUserHub<IMetadataClient, MetadataClientState> | |
private readonly IScoreProcessedSubscriber scoreProcessedSubscriber; | ||
|
||
internal const string ONLINE_PRESENCE_WATCHERS_GROUP = "metadata:online-presence-watchers"; | ||
internal static string FRIEND_PRESENCE_WATCHERS_GROUP(int userId) => $"metadata:online-presence-watchers:{userId}"; | ||
|
||
internal static string MultiplayerRoomWatchersGroup(long roomId) => $"metadata:multiplayer-room-watchers:{roomId}"; | ||
|
||
|
@@ -69,6 +70,21 @@ public override async Task OnConnectedAsync() | |
|
||
await logLogin(usage); | ||
await Clients.Caller.DailyChallengeUpdated(dailyChallengeUpdater.Current); | ||
|
||
using (var db = databaseFactory.GetInstance()) | ||
{ | ||
foreach (int friendId in await db.GetUserFriendsAsync(usage.Item.UserId)) | ||
{ | ||
await Groups.AddToGroupAsync(Context.ConnectionId, FRIEND_PRESENCE_WATCHERS_GROUP(friendId)); | ||
peppy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Check if the friend is online, and if they are, broadcast to the connected user. | ||
using (var friendUsage = await TryGetStateFromUser(friendId)) | ||
{ | ||
if (friendUsage?.Item != null && shouldBroadcastPresenceToOtherUsers(friendUsage.Item)) | ||
await Clients.Caller.FriendPresenceUpdated(friendId, friendUsage.Item.ToUserPresence()); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
|
@@ -222,7 +238,11 @@ private Task broadcastUserPresenceUpdate(int userId, UserPresence? userPresence) | |
// we never want appearing offline users to have their status broadcast to other clients. | ||
Debug.Assert(userPresence?.Status != UserStatus.Offline); | ||
|
||
return Clients.Group(ONLINE_PRESENCE_WATCHERS_GROUP).UserPresenceUpdated(userId, userPresence); | ||
return Task.WhenAll | ||
( | ||
Clients.Group(ONLINE_PRESENCE_WATCHERS_GROUP).UserPresenceUpdated(userId, userPresence), | ||
Clients.Group(FRIEND_PRESENCE_WATCHERS_GROUP(userId)).FriendPresenceUpdated(userId, userPresence) | ||
Comment on lines
+243
to
+244
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess clients are going to get double status updates for friends, but probably fine for now. |
||
); | ||
} | ||
|
||
private bool shouldBroadcastPresenceToOtherUsers(MetadataClientState state) | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
For this particular case, do we actually need a lock? Wondering if it can be avoided..
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.
Yes, because
ItemUsage.Item
enforces that the item must be locked:osu-server-spectator/osu.Server.Spectator/Entities/EntityStore.cs
Lines 195 to 201 in 718fb5a
A deadlock concern is valid here, saved by the fact that
EntityStore
will continuously retry taking the lock over 5 seconds, before throwing a timeout exception. So I think it'll be fine after all.I've tried to resolve this in a more "proper" way, and have come up with the following. I'm not sure if it's over-engineered (the hub part), and likely not over-engineered enough:
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.
Aha, I see.
Let's... save this for another time 😅 .