From 422260797b68cc6e3710d327c4e3accc66d4797c Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 12 Jan 2021 16:03:12 +0900 Subject: [PATCH 1/5] Revert polling changes to fix participant list display It turns out this polling was necessary to get extra data that isn't included in the main listing request. It was removed deemed useless, and in order to fix the order of rooms changing when selecting a room. Weirdly, I can't reproduce this happening any more, and on close inspection of the code can't see how it could happen in the first place. For now, let's revert this change and iterate from there, if/when the same issue arises again. I've discussed avoiding this second poll by potentially including more data (just `user_id`s?) in the main listing request, but not 100% sure on this - even if the returned data is minimal it's an extra join server-side, which could cause performance issues for large numbers of rooms. --- .../TestSceneMultiplayerRoomManager.cs | 1 + .../OnlinePlay/Multiplayer/Multiplayer.cs | 5 +++- .../Multiplayer/MultiplayerRoomManager.cs | 28 ++++++++++++++++++- 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerRoomManager.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerRoomManager.cs index 80d1acd145..7a3845cbf3 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerRoomManager.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerRoomManager.cs @@ -143,6 +143,7 @@ namespace osu.Game.Tests.Visual.Multiplayer RoomManager = { TimeBetweenListingPolls = { Value = 1 }, + TimeBetweenSelectionPolls = { Value = 1 } } }; diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Multiplayer.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Multiplayer.cs index 310617a0bc..76f5c74433 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Multiplayer.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Multiplayer.cs @@ -33,6 +33,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer if (!this.IsCurrentScreen()) { multiplayerRoomManager.TimeBetweenListingPolls.Value = 0; + multiplayerRoomManager.TimeBetweenSelectionPolls.Value = 0; } else { @@ -40,16 +41,18 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer { case LoungeSubScreen _: multiplayerRoomManager.TimeBetweenListingPolls.Value = isIdle ? 120000 : 15000; + multiplayerRoomManager.TimeBetweenSelectionPolls.Value = isIdle ? 120000 : 15000; break; // Don't poll inside the match or anywhere else. default: multiplayerRoomManager.TimeBetweenListingPolls.Value = 0; + multiplayerRoomManager.TimeBetweenSelectionPolls.Value = 0; break; } } - Logger.Log($"Polling adjusted (listing: {multiplayerRoomManager.TimeBetweenListingPolls.Value})"); + Logger.Log($"Polling adjusted (listing: {multiplayerRoomManager.TimeBetweenListingPolls.Value}, selection: {multiplayerRoomManager.TimeBetweenSelectionPolls.Value})"); } protected override Room CreateNewRoom() diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerRoomManager.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerRoomManager.cs index 5c327266a3..3cb263298f 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerRoomManager.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerRoomManager.cs @@ -23,7 +23,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer private StatefulMultiplayerClient multiplayerClient { get; set; } public readonly Bindable TimeBetweenListingPolls = new Bindable(); - + public readonly Bindable TimeBetweenSelectionPolls = new Bindable(); private readonly IBindable isConnected = new Bindable(); private readonly Bindable allowPolling = new Bindable(); @@ -119,6 +119,11 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer TimeBetweenPolls = { BindTarget = TimeBetweenListingPolls }, AllowPolling = { BindTarget = allowPolling } }, + new MultiplayerSelectionPollingComponent + { + TimeBetweenPolls = { BindTarget = TimeBetweenSelectionPolls }, + AllowPolling = { BindTarget = allowPolling } + } }; private class MultiplayerListingPollingComponent : ListingPollingComponent @@ -141,5 +146,26 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer protected override Task Poll() => !AllowPolling.Value ? Task.CompletedTask : base.Poll(); } + + private class MultiplayerSelectionPollingComponent : SelectionPollingComponent + { + public readonly IBindable AllowPolling = new Bindable(); + + protected override void LoadComplete() + { + base.LoadComplete(); + + AllowPolling.BindValueChanged(allowPolling => + { + if (!allowPolling.NewValue) + return; + + if (IsLoaded) + PollImmediately(); + }); + } + + protected override Task Poll() => !AllowPolling.Value ? Task.CompletedTask : base.Poll(); + } } } From b51b07c3a90c0dc1635984f9346175746c5e3da7 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 12 Jan 2021 18:05:29 +0900 Subject: [PATCH 2/5] Fix unstable multiplayer room ordering when selection is made --- .../OnlinePlay/Components/SelectionPollingComponent.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/osu.Game/Screens/OnlinePlay/Components/SelectionPollingComponent.cs b/osu.Game/Screens/OnlinePlay/Components/SelectionPollingComponent.cs index 0eec155060..dcf3c94b76 100644 --- a/osu.Game/Screens/OnlinePlay/Components/SelectionPollingComponent.cs +++ b/osu.Game/Screens/OnlinePlay/Components/SelectionPollingComponent.cs @@ -2,6 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using System.Collections.Generic; +using System.Linq; using System.Threading.Tasks; using osu.Framework.Allocation; using osu.Framework.Bindables; @@ -47,9 +48,11 @@ namespace osu.Game.Screens.OnlinePlay.Components pollReq.Success += result => { - var rooms = new List(roomManager.Rooms); + // existing rooms need to be ordered by their position because the received of NotifyRoomsReceives expects to be able to sort them based on this order. + var rooms = new List(roomManager.Rooms.OrderBy(r => r.Position.Value)); int index = rooms.FindIndex(r => r.RoomID.Value == result.RoomID.Value); + if (index < 0) return; From 7298adc9d9b8ef878f4be5b8799be222aa0dd90b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 12 Jan 2021 19:04:16 +0900 Subject: [PATCH 3/5] Fix non-threadsafe usage of MultiplayerClient.IsConnected --- osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs | 1 + .../OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs | 4 ++-- .../Screens/OnlinePlay/Multiplayer/MultiplayerPlayer.cs | 4 ++-- .../OnlinePlay/Multiplayer/MultiplayerRoomManager.cs | 6 ++---- 4 files changed, 7 insertions(+), 8 deletions(-) diff --git a/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs b/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs index 34cba09e8c..770039d79d 100644 --- a/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs @@ -52,6 +52,7 @@ namespace osu.Game.Online.Multiplayer /// /// Whether the is currently connected. + /// This is NOT thread safe and usage should be scheduled. /// public abstract IBindable IsConnected { get; } diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs index 87b0e49b5b..a13d2cf540 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs @@ -34,8 +34,8 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer { base.LoadComplete(); - isConnected.BindValueChanged(_ => updateState()); - operationInProgress.BindValueChanged(_ => updateState(), true); + isConnected.BindValueChanged(_ => Scheduler.AddOnce(updateState)); + operationInProgress.BindValueChanged(_ => Scheduler.AddOnce(updateState), true); } private void updateState() => Enabled.Value = isConnected.Value && !operationInProgress.Value; diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerPlayer.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerPlayer.cs index 4bee502e2e..04d9e0a72a 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerPlayer.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerPlayer.cs @@ -77,14 +77,14 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer }); isConnected = client.IsConnected.GetBoundCopy(); - isConnected.BindValueChanged(connected => + isConnected.BindValueChanged(connected => Schedule(() => { if (!connected.NewValue) { // messaging to the user about this disconnect will be provided by the MultiplayerMatchSubScreen. failAndBail(); } - }, true); + }), true); Debug.Assert(client.Room != null); } diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerRoomManager.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerRoomManager.cs index 5c327266a3..bcae2e5cbb 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerRoomManager.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerRoomManager.cs @@ -34,10 +34,8 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer base.LoadComplete(); isConnected.BindTo(multiplayerClient.IsConnected); - isConnected.BindValueChanged(_ => Schedule(updatePolling)); - JoinedRoom.BindValueChanged(_ => updatePolling()); - - updatePolling(); + isConnected.BindValueChanged(_ => Scheduler.AddOnce(updatePolling)); + JoinedRoom.BindValueChanged(_ => Scheduler.AddOnce(updatePolling), true); } public override void CreateRoom(Room room, Action onSuccess = null, Action onError = null) From 8a0b975d71d08f6a845b10b46aec50d714d00c68 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 Jan 2021 18:25:32 +0900 Subject: [PATCH 4/5] Fix deadlock scenario when calculating fallback difficulty The previous code would run a calcaulation for the beatmap's own ruleset if the current one failed. While this does make sense, with the current way we use this component (and the implementation flow) it is quite unsafe. The to the call on `.Result` in the `catch` block, this would 100% deadlock due to the thread concurrency of the `ThreadedTaskScheduler` being 1. Even if the nested run could be run inline (it should be), the task scheduler won't even get to the point of checking whether this is feasible due to it being saturated by the already running task. I'm not sure if we still need this fallback lookup logic. After removing it, it's feasible that 0 stars will be returned during the scenario that previously caused a deadlock, but I don't necessarily think this is incorrect. There may be another reason for this needing to exist which I'm not aware of (diffcalc?) but if that's the case we may want to move the try-catch handling to the point of usage. To reproduce the deadlock scenario with 100% success (the repro instructions in the linked issue aren't that simple and require some patience and good timing), the main portion of the lookup can be changed to randomly trigger a nested lookup: ``` if (RNG.NextSingle() > 0.5f) return GetAsync(new DifficultyCacheLookup(key.Beatmap, key.Beatmap.Ruleset, key.OrderedMods)).Result; else return new StarDifficulty(attributes); ``` After switching beatmap once or twice, pausing debug and viewing the state of threads should show exactly what is going on. --- osu.Game/Beatmaps/BeatmapDifficultyCache.cs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapDifficultyCache.cs b/osu.Game/Beatmaps/BeatmapDifficultyCache.cs index 3b58062add..37d262abe5 100644 --- a/osu.Game/Beatmaps/BeatmapDifficultyCache.cs +++ b/osu.Game/Beatmaps/BeatmapDifficultyCache.cs @@ -260,17 +260,10 @@ namespace osu.Game.Beatmaps } catch (BeatmapInvalidForRulesetException e) { - // Conversion has failed for the given ruleset, so return the difficulty in the beatmap's default ruleset. - - // Ensure the beatmap's default ruleset isn't the one already being converted to. - // This shouldn't happen as it means something went seriously wrong, but if it does an endless loop should be avoided. if (rulesetInfo.Equals(beatmapInfo.Ruleset)) - { Logger.Error(e, $"Failed to convert {beatmapInfo.OnlineBeatmapID} to the beatmap's default ruleset ({beatmapInfo.Ruleset})."); - return new StarDifficulty(); - } - return GetAsync(new DifficultyCacheLookup(key.Beatmap, key.Beatmap.Ruleset, key.OrderedMods)).Result; + return new StarDifficulty(); } catch { From 0a65ae8f1ef72f883e9f1e324bf3cd622bd321d0 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 15 Jan 2021 14:07:24 +0900 Subject: [PATCH 5/5] Fix the beatmap carousel playing the difficulty change sample on beatmap change --- osu.Game/Screens/Select/SongSelect.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Select/SongSelect.cs b/osu.Game/Screens/Select/SongSelect.cs index 40db04ae71..6c0bd3a228 100644 --- a/osu.Game/Screens/Select/SongSelect.cs +++ b/osu.Game/Screens/Select/SongSelect.cs @@ -500,7 +500,7 @@ namespace osu.Game.Screens.Select if (beatmap != null) { - if (beatmap.BeatmapSetInfoID == beatmapNoDebounce?.BeatmapSetInfoID) + if (beatmap.BeatmapSetInfoID == previous?.BeatmapInfo.BeatmapSetInfoID) sampleChangeDifficulty.Play(); else sampleChangeBeatmap.Play();