Add store for caching online assets to disk

[bdach]

RFC.

Previously: #35892

The reason why I am coming back to this is the following excerpt from #36527 (comment):

maybe a good time to setup a local cache for this + seasonal backgrounds which are currently fetch every time??

This would be one way to make this happen.

Changes from previous attempt

This PR incorporates the following pieces of feedback from the previous round:

• No longer trying to impose structure on the incoming URLs, instead treating them as an opaque content address from the start
• No longer trying to impose a separate hierarchical structure on the disk storage, instead using the same file store as beatmap / skin content
• Using a time-based expiry scheme (files not accessed for over 1 month are purged)

Open questions:

Content addressability via the URL

For this cache to work well, the URLs it fetches must be "content address-like". To explain in more human terms what that means: If an asset is assigned an URL once and fetched via this cache, then its assigned URL should ideally never be reused for a new asset (practically, at least no sooner than after a month of ceasing use of the old asset).

The following types of online assets already meet this standard:

• User avatars (cache-busting time-of-update-based query string)
• User covers (hash of raw content in the filename)
• Team flags (hash of raw content in the filename)

The following types of online assets probably meet this standard:

• Beatmap covers (looks like cache-busting time-of-update-based query string)
• Placeholder assets (example: https://osu.ppy.sh/assets/images/0.b3bb5a86.jpg; seems to contain a hash-like string)

In general determining whether it is permissible to use this cache is difficult to ascertain in some cases.

Expiry scheme

The 1 month time-based threshold is arbitrary. It can be tweaked as desired.

Extent of use & potential resulting impact on performance

This PR is not using a "separate realm or sqlite database" as mentioned here. Whether this is acceptable likely depends on frequency of use. If we only decide to put background in this, it's probably okay to disregard performance; if user avatars and/or beatmap covers are to live here too, it becomes a larger concern.

There is also the open question as to whether we want to allow user-submitted content to be stored on other users' drives.

To that end, please treat c44d33d as a proof of concept of how this cache is to be used at most and a vehicle to easily test its operation, rather than a serious statement on any of the above questions.

[peppy]

I like this.

I also have concerns regarding performance, but these concerns are realm level, and not with the way this has been implemented. If anything, I think we should keep putting load on realm until it breaks and we have to replace it. Or split out into multiple realm files (probably not great for this one because it would require also making RealmFileStore exist in multiple realm files too).

So I think it's fine to push forward in this direction 👍 .

[peppy]

As above

[bdach]

Are you fine with user avatars / profile covers / team flags landing on disk like this is currently doing in c44d33d or should I revert / otherwise adjust that part for now?

(CI is also looking very bad on this, will need to figure out what that is about.)

[peppy]

Are you fine with user avatars / profile covers / team flags landing on disk like this is currently doing in c44d33d or should I revert / otherwise adjust that part for now?

Yeah, as long as these have correct caching considerations (i think the url changes when they change, so should be fine) we should do it. Stable also does this.

[bdach]

Please disregard / forgive above commit but I am out of ideas. I will force-push it out if I can reach some concrete conclusion as to why this is dying on single-thread CI runs.

[bdach]

Well, the good news is that timeout on realm retrieval worked. The bad news is that there's basically zero further feedback about it.

Clearly this is something to do with attempting to do realm accesses from inside BDL which I don't think we do anywhere else and probably majorly messes with this because drawables have one common lock for load and disposal which probably means there's a possibility of a drawable being inside BDL, trying to retrieve a realm instance from a disposed realm which will hang forever, and the drawable blocks game clean-up because its disposal is dependent on the same lock that the drawable is already holding in trying to load.

@peppy do you have any suggestions? I can only think of a couple:

• actually unironically going with 57d5ee8,
• maybe-possibly finagling something such that disposal of RealmAccess immediately kills all blocked writers,
• maybe-possibly enforcing that this new store does all realm accesses on update thread?

[peppy]

trying to retrieve a realm instance from a disposed realm which will hang forever

Curious where you see this happening. I do think fixing the issue at RealmAccess is our best path forward (and I'm looking to do that), but can't reproduce your specific scenario via:

diff --git a/osu.Game.Tests/Database/GeneralUsageTests.cs b/osu.Game.Tests/Database/GeneralUsageTests.cs
index 3550d39ab8..5b2df55bfc 100644
--- a/osu.Game.Tests/Database/GeneralUsageTests.cs
+++ b/osu.Game.Tests/Database/GeneralUsageTests.cs
@@ -26,6 +26,19 @@ public void TestConstructRealm()
             RunTestWithRealm((realm, _) => { realm.Run(r => r.Refresh()); });
         }
 
+        [Test]
+        public void TestGetInstancePostDisposal()
+        {
+            RealmAccess realmPostDisposal = null!;
+
+            RunTestWithRealm((realm, _) =>
+            {
+                realmPostDisposal = realm;
+            });
+
+            Assert.Throws<ObjectDisposedException>(() => realmPostDisposal.Run(_ => { }));
+        }
+
         [Test]
         public void TestBlockOperations()
         {

Is there some race condition required here? Are you thinking that disposal check needs to happen a second time after the lock retrieval? The following seems like it may be required for race condition safety:

diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs
index 67fb5a7b25..8eada868ea 100644
--- a/osu.Game/Database/RealmAccess.cs
+++ b/osu.Game/Database/RealmAccess.cs
@@ -1384,6 +1384,8 @@ public IDisposable BlockAllOperations(string reason)
             {
                 realmRetrievalLock.Wait();
 
+                ObjectDisposedException.ThrowIf(isDisposed, this);
+
                 if (hasInitialisedOnce)
                 {
                     syncContext = SynchronizationContext.Current;

[bdach]

Curious where you see this happening

I can't observe this myself anymore but I did see it before ad4e6ed which is why I was hoping that commit would be enough. Somehow it still wasn't, and 57d5ee8 miraculously fixing the tests points a pretty strong finger at this still being the case in some set of circumstances.

See also

osu/osu.Game/Database/RealmAccess.cs

Lines 1515 to 1519 in 57d5ee8

	// intentionally block realm retrieval indefinitely. this ensures that nothing can start consuming a new instance after disposal.
	realmRetrievalLock.Wait();
	realmRetrievalLock.Dispose();
	isDisposed = true;