From 1c4ecba950beab9662638561b4752e157f82ebab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 27 Mar 2025 08:50:11 +0100 Subject: [PATCH] Fix several issues with multiplayer & playlists room join error logging This is in response to https://osu.ppy.sh/community/forums/topics/2058708?n=5, wherein the user is having a problem with joining multiplayer, but I have basically no diagnosing capabilities, because the logs are all 2025-03-26 18:57:57 [error]: Failed to join multiplayer room: 2025-03-26 18:58:40 [error]: Failed to join multiplayer room: 2025-03-26 18:58:41 [error]: Failed to join multiplayer room: 2025-03-26 18:58:41 [error]: Failed to join multiplayer room: which appears to originate from https://github.com/ppy/osu/blob/c82eaafe98d96b9f49a4a7f168ef5c484e67d76f/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerLoungeSubScreen.cs#L91 Now, as far as I can tell, there are two possibilities here: 1. The exception's `Message` is null or empty. That's not exactly great or typical, but sure, this could possibly happen - in which case the error logging is silently eating whatever little relevant detail there is left to use. 2. The exception is *actually* `null` itself, and we're in the X Files. This PR is intending to defend against (1). In examining the logging further, I also spotted the following issues: - In the single path that specifies a custom failure handler (which is `DrawableLoungeRoom` which handles joining a passworded room), the custom failure handler being present means that the error would be presented to the user, but it would not be logged. At all. - In playlists, if the exception for whatever reason had an empty message, an empty notification would get posted. And in general to me it feels a bit dodgy to be directly presenting exception notifications to users without any preamble, hence the added "Failed to open playlist" prefix. --- .../Visual/Multiplayer/TestSceneDrawableLoungeRoom.cs | 2 +- .../Screens/OnlinePlay/Lounge/DrawableLoungeRoom.cs | 6 ++++-- .../Screens/OnlinePlay/Lounge/IOnlinePlayLounge.cs | 4 ++-- osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs | 10 +++++----- .../Multiplayer/MultiplayerLoungeSubScreen.cs | 6 +++--- .../OnlinePlay/Playlists/PlaylistsLoungeSubScreen.cs | 4 ++-- 6 files changed, 17 insertions(+), 15 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableLoungeRoom.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableLoungeRoom.cs index 459a90d096..5d6bc5e50a 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableLoungeRoom.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableLoungeRoom.cs @@ -40,7 +40,7 @@ namespace osu.Game.Tests.Visual.Multiplayer { var mockLounge = new Mock(); mockLounge - .Setup(l => l.Join(It.IsAny(), It.IsAny(), It.IsAny>(), It.IsAny>())) + .Setup(l => l.Join(It.IsAny(), It.IsAny(), It.IsAny>(), It.IsAny>())) .Callback, Action>((_, _, _, d) => { Task.Run(() => diff --git a/osu.Game/Screens/OnlinePlay/Lounge/DrawableLoungeRoom.cs b/osu.Game/Screens/OnlinePlay/Lounge/DrawableLoungeRoom.cs index d369722e5f..339780b517 100644 --- a/osu.Game/Screens/OnlinePlay/Lounge/DrawableLoungeRoom.cs +++ b/osu.Game/Screens/OnlinePlay/Lounge/DrawableLoungeRoom.cs @@ -17,6 +17,7 @@ using osu.Framework.Graphics.UserInterface; using osu.Framework.Input.Bindings; using osu.Framework.Input.Events; using osu.Framework.Localisation; +using osu.Framework.Logging; using osu.Game.Extensions; using osu.Game.Graphics; using osu.Game.Graphics.Sprites; @@ -306,13 +307,14 @@ namespace osu.Game.Screens.OnlinePlay.Lounge GetContainingFocusManager()?.TriggerFocusContention(passwordTextBox); } - private void joinFailed(string error) => Schedule(() => + private void joinFailed(string message, Exception? exception) => Schedule(() => { passwordTextBox.Text = string.Empty; GetContainingFocusManager()!.ChangeFocus(passwordTextBox); - errorText.Text = error; + Logger.Log($"Failed to join room with password. {exception}"); + errorText.Text = message; errorText .FadeIn() .FlashColour(Color4.White, 200) diff --git a/osu.Game/Screens/OnlinePlay/Lounge/IOnlinePlayLounge.cs b/osu.Game/Screens/OnlinePlay/Lounge/IOnlinePlayLounge.cs index 73ab84af13..c9f6921328 100644 --- a/osu.Game/Screens/OnlinePlay/Lounge/IOnlinePlayLounge.cs +++ b/osu.Game/Screens/OnlinePlay/Lounge/IOnlinePlayLounge.cs @@ -14,8 +14,8 @@ namespace osu.Game.Screens.OnlinePlay.Lounge /// The room to join. /// The password. /// A delegate to invoke if the user joined the room. - /// A delegate to invoke if the user is not able join the room. - void Join(Room room, string? password, Action? onSuccess = null, Action? onFailure = null); + /// A delegate to invoke if the user is not able to join the room. + void Join(Room room, string? password, Action? onSuccess = null, Action? onFailure = null); /// /// Copies the given room and opens it as a fresh (not-yet-created) one. diff --git a/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs b/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs index c455020f9a..a4e808ff76 100644 --- a/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs @@ -334,7 +334,7 @@ namespace osu.Game.Screens.OnlinePlay.Lounge popoverContainer.HidePopover(); } - public void Join(Room room, string? password, Action? onSuccess = null, Action? onFailure = null) => Schedule(() => + public void Join(Room room, string? password, Action? onSuccess = null, Action? onFailure = null) => Schedule(() => { if (joiningRoomOperation != null) return; @@ -347,19 +347,19 @@ namespace osu.Game.Screens.OnlinePlay.Lounge joiningRoomOperation?.Dispose(); joiningRoomOperation = null; onSuccess?.Invoke(room); - }, error => + }, (message, exception) => { joiningRoomOperation?.Dispose(); joiningRoomOperation = null; if (onFailure != null) - onFailure(error); + onFailure(message, exception); else - Logger.Log(error, level: LogLevel.Error); + Logger.Error(exception, message); }); }); - protected abstract void JoinInternal(Room room, string? password, Action onSuccess, Action onFailure); + protected abstract void JoinInternal(Room room, string? password, Action onSuccess, Action onFailure); public void OpenCopy(Room room) { diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerLoungeSubScreen.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerLoungeSubScreen.cs index 51c135f042..5e2619eae3 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerLoungeSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerLoungeSubScreen.cs @@ -75,7 +75,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer protected override OnlinePlaySubScreen CreateRoomSubScreen(Room room) => new MultiplayerMatchSubScreen(room); - protected override void JoinInternal(Room room, string? password, Action onSuccess, Action onFailure) + protected override void JoinInternal(Room room, string? password, Action onSuccess, Action onFailure) { client.JoinRoom(room, password).ContinueWith(result => { @@ -86,9 +86,9 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer Exception? exception = result.Exception?.AsSingular(); if (exception?.GetHubExceptionMessage() is string message) - onFailure(message); + onFailure(message, exception); else - onFailure($"Failed to join multiplayer room: {exception?.Message}"); + onFailure($"Failed to join multiplayer room. {exception?.Message}", exception); } }); } diff --git a/osu.Game/Screens/OnlinePlay/Playlists/PlaylistsLoungeSubScreen.cs b/osu.Game/Screens/OnlinePlay/Playlists/PlaylistsLoungeSubScreen.cs index eccbaf7930..cc4065a82b 100644 --- a/osu.Game/Screens/OnlinePlay/Playlists/PlaylistsLoungeSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Playlists/PlaylistsLoungeSubScreen.cs @@ -60,7 +60,7 @@ namespace osu.Game.Screens.OnlinePlay.Playlists return criteria; } - protected override void JoinInternal(Room room, string? password, Action onSuccess, Action onFailure) + protected override void JoinInternal(Room room, string? password, Action onSuccess, Action onFailure) { var joinRoomRequest = new JoinRoomRequest(room, password); @@ -68,7 +68,7 @@ namespace osu.Game.Screens.OnlinePlay.Playlists joinRoomRequest.Failure += exception => { if (exception is not OperationCanceledException) - onFailure(exception.Message); + onFailure($"Failed to open playlist. {exception.Message}", exception); }; api.Queue(joinRoomRequest);