Skip to content

Conversation

@smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Dec 11, 2025

Fixes #35886 (comment)

Seems pretty difficult to unit test this so I've only tested via:

diff --git a/osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs b/osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs
index 7cfa036dcd..e65d1b0738 100644
--- a/osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs
+++ b/osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs
@@ -121,6 +121,8 @@ protected override async Task<MultiplayerRoom> JoinRoomInternal(long roomId, str
 
             Debug.Assert(connection != null);
 
+            await Task.Delay(5000).ConfigureAwait(false);
+
             try
             {
                 return await connection.InvokeAsync<MultiplayerRoom>(nameof(IMultiplayerServer.JoinRoomWithPassword), roomId, password ?? string.Empty).ConfigureAwait(false);

Will document more in comments on the PR itself.

@smoogipoo smoogipoo requested a review from a team December 11, 2025 11:11
@smoogipoo smoogipoo self-assigned this Dec 11, 2025
@smoogipoo smoogipoo moved this from Inbox to Pending Review in osu! untitled project Dec 11, 2025
Comment on lines -322 to -323
if (Room == null)
return Task.CompletedTask;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know why this was added or necessary, because it doesn't look to be. Basically, there's a moment in time (that can actually be quite lengthy due to user lookups), where the room is "joined" but Room hasn't been set. The patch in the OP shows this.

It seems like we should always run through the full leave process?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originates from 02369ba. Appears it wasn't asked about at review time, probably because it looked obvious / harmless.

bdach
bdach previously approved these changes Dec 11, 2025
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems probably fine

Comment on lines -322 to -323
if (Room == null)
return Task.CompletedTask;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originates from 02369ba. Appears it wasn't asked about at review time, probably because it looked obvious / harmless.

if (base.OnExiting(e))
return true;

client.LeaveRoom().FireAndForget();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're here doing this, the xmldoc says this can throw:

/// <exception cref="NotJoinedRoomException">If the user is not in a room.</exception>

but looking at the server-side implementation I don't see how it can throw because this specific scenario is guarded to do nothing instead of throwing, so might as well strip that mention.

@peppy peppy merged commit 62e92bb into ppy:master Dec 12, 2025
7 of 9 checks passed
@peppy peppy deleted the fix-mp-screen-leave branch December 12, 2025 08:06
@github-project-automation github-project-automation bot moved this from Pending Review to Done in osu! untitled project Dec 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

The interface of joining a multiplayer room is interruptible.

3 participants