Fix Carousel.FilterAsync not working when called from a non-update thread

[peppy]

Spotted by @frenzibyte during implementation in the bigger picture.

I was trying to be smart about things and make use of our SynchronisationContext setup, but it turns out to not work in all cases due to the context being missing depending on where you are calling the method from.

For now let's prefer the "works everywhere" method of scheduling the final work back to update.

[bdach]

The lock-taking on potentially-the-update-thread earlier in this method still has me slightly skeeved out:

osu/osu.Game/Screens/SelectV2/Carousel.cs

Lines 236 to 240 in 8c5b19d

	lock (this)
	{
	cancellationSource.Cancel();
	cancellationSource = cts;
	}

but I guess this change in isolation makes sense...

[smoogipoo]

"slightly skeeved out" is a tame way to put it. I'd put it as "completely wrong" and needs to change as a priority.

lock (this) must never be done. Please refer to C# usage guidelines: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/lock#guidelines

[bdach]

Valid point which I missed, although I meant more the act of potentially taking a lock on the update thread at all than the mechanics on which object the lock is taken specifically. Like locking on this is technically wrong but I also think I'm yet to actually real-world be bitten by that in however many years, you know?

[peppy]

The lock-taking on potentially-the-update-thread earlier in this method still has me slightly skeeved out

It's locking over instant operations, but I'm fine with an alternative approach, just can't find another case we're doing this in a better way.

I'd put it as "completely wrong" and needs to change as a priority.

Is this regarding the lock object? If so I've fixed that. If it's something else you'll need to elaborate or provide a diff.

[smoogipoo]

Is this regarding the lock object?

As mentioned, it was about lock (this). Your change fixed my issue.

[bdach]

It's locking over instant operations, but I'm fine with an alternative approach

Hmmmm... This lockless alternative leveraging cpu atomics works in my head:

diff --git a/osu.Game/Screens/SelectV2/Carousel.cs b/osu.Game/Screens/SelectV2/Carousel.cs
index 648c2d090a..7c1392ce6e 100644
--- a/osu.Game/Screens/SelectV2/Carousel.cs
+++ b/osu.Game/Screens/SelectV2/Carousel.cs
@@ -227,16 +227,13 @@ private async Task performFilter()
             Stopwatch stopwatch = Stopwatch.StartNew();
             var cts = new CancellationTokenSource();
 
-            lock (this)
-            {
-                cancellationSource.Cancel();
-                cancellationSource = cts;
-            }
+            var previousCancellationSource = Interlocked.Exchange(ref cancellationSource, cts);
+            await previousCancellationSource.CancelAsync().ConfigureAwait(false);
 
             if (DebounceDelay > 0)
             {
                 log($"Filter operation queued, waiting for {DebounceDelay} ms debounce");
-                await Task.Delay(DebounceDelay, cts.Token).ConfigureAwait(true);
+                await Task.Delay(DebounceDelay, cts.Token).ConfigureAwait(false);
             }
 
             // Copy must be performed on update thread for now (see ConfigureAwait above).

Not sure whether better or worse?

(That last change to ConfigureAwait() is not super relevant, I was just thrown off how it's half-false half-true in the method now. Also the cancel has to be async lest rider complain that the method has an async overload, which wasn't an issue before because you can't await anything in a synchronised block.)

[peppy]

I think either is fine, so go ahead and apply your change.

(That last change to ConfigureAwait() is not super relevant, I was just thrown off how it's half-false half-true in the method now. Also the cancel has to be async lest rider complain that the method has an async overload, which wasn't an issue before because you can't await anything in a synchronised block.)

I missed this one. Should be false in all places now that we're never needing the schedule-back-to-update mode of operation.