Skip to content
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

Fix a few issues in metadata hub's disconnection requested flow #27193

Merged
merged 4 commits into from Feb 17, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Feb 16, 2024

Before you read on: as far as I can tell the current effect of the flaws I'm about to describe on current client / production is zero, because all of the issues are masked by the fact that currently being told to disconnect by the server via any other hub will fully log the user out, implicitly severing the connection to the metadata hub too.

That said this might be changing soon (in certain circumstances, at least) as I work on ppy/osu-server-spectator#52, which is how I found this out.

To test, you can disable the aforementioned masking behaviour by applying:

diff --git a/osu.Game/Online/HubClientConnector.cs b/osu.Game/Online/HubClientConnector.cs
index 9d414deade..ad71038732 100644
--- a/osu.Game/Online/HubClientConnector.cs
+++ b/osu.Game/Online/HubClientConnector.cs
@@ -103,7 +103,6 @@ protected override Task<PersistentEndpointClient> BuildConnectionAsync(Cancellat
         async Task IHubClientConnector.Disconnect()
         {
             await Disconnect().ConfigureAwait(false);
-            API.Logout();
         }
 
         protected override string ClientName { get; }

After applying the above, you can perform the following test scenario on master:

  • Log onto client instance no. 1
  • Log onto client instance no. 2. Client instance no. 1 will be disconnected from multiplayer and spectator hubs but not logged out. You should still see the notification saying that you "were logged out" still, though.
  • On client instance no. 1, change the user's status via the login overlay, or do anything that would change the user's current activity.
  • On master you should see unobserved errors on every change; on this branch you should see no errors.

As for the actual description of the changes:

Fix missing RPC method mapping

Pretty obvious what went wrong there.

Actually disconnect from metadata hub when requested to

Nothing in this override, on in the base implementation, would actually attempt to disconnect.

Do not attempt to stop watching user presence when requested to disconnect

First of all, this sort of cleanup isn't really the client's responsibility, and secondly, at the point the client received this
request to disconnect, none of its requests will be honored anymore (currently the only scenario of this if another client has connected - the server-side concurrency filter will reject this request).

When disconnection is requested, the only valid thing to do with respect to talking to the server is to stop doing it.

This will probably be moved server-side in a follow-up change, although I'm not even strictly sure that's required - I'd like to think signalr would know to clean up a disconnecting client from all groups they were in.

Add extended xmldoc to DisconnectRequested()

Just expounds on what the previous commit message said.

Nothing in this override, on in the base implementation, would actually
attempt to disconnect.
…nnect

First of all, this sort of cleanup isn't really the client's
responsibility, and secondly, at the point the client received this
request to disconnect, *none of its requests will be honored anymore*
(currently the only scenario of this if another client has connected
- the server-side concurrency filter will reject this request).

When disconnection is requested, the only valid thing to do with respect
to talking to the server is to stop doing it.

This will be moved server-side in a follow-up change, although I'm not
even strictly sure that's required - I'd like to think signalr would
know to clean up a disconnecting client from all groups they were in.
Just expounds on what the previous commit message said.
@peppy peppy merged commit 520576a into ppy:master Feb 17, 2024
15 of 17 checks passed
@bdach bdach deleted the fix-not-working-metadata-hub-cleanup branch February 19, 2024 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants