It's not universally accepted here and a `when` crept in that can be
bypassed entirely using rather clean baseline language constructs, so
why bother at this point.
@peppy noticed recently that attempting to spectate just a few users was
very likely to end up in requests very quickly being rejected with code
429 ("Too Many Requests").
I'm somewhat certain that the reason for that is that a significant
number of players is wont to retry a lot in quick succession. That means
that spectator server is going to note a lot of gameplay start and end
messages in quick succession, too. And as it turns out, every gameplay
start would end up triggering a new beatmap set fetch request:
ccf1acce56/osu.Game/Screens/Spectate/SpectatorScreen.cs (L131-L134)ccf1acce56/osu.Game/Screens/Play/SoloSpectatorScreen.cs (L168-L172)ccf1acce56/osu.Game/Screens/Play/SoloSpectatorScreen.cs (L243-L256)
To attempt to curtail that, use the beatmap cache instead, which should
prevent these unnecessary requests from firing in the first place,
therefore reducing the chance of the client getting throttled.
This technically means that a different endpoint is used to fetch the
data (`GET /beatmaps/?ids[]=` rather than `GET
/beatmapsets/lookup?beatmap_id={id}`), but docs claim that both should
return the same data, and it looks to work fine in practice.
Compare: https://github.com/ppy/osu/pull/24548.
I don't have a reproduction scenario (judging from the stack trace
of the crash it's likely to be nigh-impossible to concoct a reliable
one), but there is some circumstantial evidence that this might help,
namely that that previous fix above worked, and the pathway that's
failing here is similarly async to the one that pull fixed. So I'm just
gonna start with that and hope that it does the job.
Should fix https://github.com/ppy/osu/issues/28629.
First of all, to support the claim that this does fix the issue -
reproduction is rather difficult, but I believe I found a way to
maximise the chances of it reproducing by performing the following
steps:
1. Apply the following diff:
diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs
index eacd2b3e75..4c00da031a 100644
--- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs
@@ -6,6 +6,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
+using System.Threading;
using JetBrains.Annotations;
using osu.Framework.Allocation;
using osu.Framework.Bindables;
@@ -95,6 +96,8 @@ public DrawableSlider([CanBeNull] Slider s = null)
[BackgroundDependencyLoader]
private void load()
{
+ Thread.Sleep(100);
+
tailContainer = new Container<DrawableSliderTail> { RelativeSizeAxes = Axes.Both };
AddRangeInternal(new Drawable[]
2. Download https://osu.ppy.sh/beatmapsets/1470790#osu/3023028 and open
it in the editor.
3. Select all objects using Ctrl-A. Yes, it'll take a while, especially
so with the patch above.
4. Rotate the selection by any amount using the right toolbox.
5. Press undo. The game should crash. If it doesn't spam redo and undo
until it does.
Now to explain what the fix is.
In the issue thread I spent a considerable time hemming and hawing about
which of the dimmable pieces was null, which was a complete miss and a
failure at reading. Let's see the stack trace again:
2024-06-27 02:15:20 [error]: at osu.Game.Rulesets.Osu.Objects.Drawables.DrawableOsuHitObject.<UpdateInitialTransforms>g__applyDim|15_0(Drawable piece) in /home/runner/work/osu-auth-client/osu-auth-client/osu/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs:line 101
Line 101, you say? What could be null here?
bd8addfb5f/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs (L101)
Okay, what's `InitialLifetimeOffset`, then?
bd8addfb5f/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs (L108)
Yes, that's right. It's `HitObject` that is null here.
Now why does *that* happen? First, let's note that all stacks where this
died went through `UpdateState()`, which means that the problematic
`applyDim()` calls had to be `ApplyCustomUpdateState` event callbacks.
Meaning that the pieces where `HitObject` was null were DHOs themselves.
Recall that parent DHOs and child DHOs are pooled separately. Therefore,
there is no guarantee that any parent and child DHOs will remain
associated with each other for the entire duration of a gameplay
session; it is quite the contrary, and nobody should rely on that.
Unfortunately for us, adding a `applyDimToDrawableHitObject` callback to
a child object's `ApplyCustomUpdateState` *implicitly creates* such an
association, because it ends up allocating a closure that captures
`this` (meaning the parent in this context).
Therefore, this now creates a situation where a child DHO can attempt to
read state from a former parent DHO which can be in an indeterminate
state, and in fact, when this crashes, the former parent DHO is most
likely not even in use - hence the null `HitObject`.
Thus, the fix is to clear the association by unsubscribing from the
event when nested objects are cleared.
My hypothesis why the reproduction scenario is like it is, is that both
the sleep and the increased pressure on the pool (by way of selecting
all objects and therefore forcing the DHOs to be materialised beyond
pool capacity) increases the likelihood of getting a crosslink.
When pool pressure is low, it is much more likely that a parent DHO
*will* get the same child DHO again on re-application, even though
that is not guaranteed.
Just as an additional detail, note that the sentry issue for this lists
the "first seen" version as 2024.312.0, which is the release that
included https://github.com/ppy/osu/pull/27401 which would be directly
responsible for this mess.
These aren't used in many places, but we've since moved away from
opacity in UI elements like this, so let's just nuke it here for
legibility.
Addresses https://github.com/ppy/osu/discussions/29906.
Dan Balasescu
Dean Herbert
Bartłomiej Dach
Marvin Schürz
Neku
OliBomby