diff --git a/src/GameCult.Caching/Contracts/cultcache-store-composition.md b/src/GameCult.Caching/Contracts/cultcache-store-composition.md index 2d9894ab..1400025f 100644 --- a/src/GameCult.Caching/Contracts/cultcache-store-composition.md +++ b/src/GameCult.Caching/Contracts/cultcache-store-composition.md @@ -131,8 +131,9 @@ which route wrote it. a cacheless file), never the reverse. What the code enforces, at run time and by `InvalidOperationException`: a thread holds at most one store's gate; while it holds a store's gate it enters no cache gate it does not already - hold, reads included; and a cache's hold (a write, a flush, an attach) entered - inside another cache's hold is refused. What it does not enforce: a cache + hold, reads included; a cacheless write made while the thread already holds + that store's gate is refused; and a cache's hold (a write, a flush, an attach) + entered inside another cache's hold is refused. What it does not enforce: a cache read made inside another cache's hold takes that cache's gate without a check, so two threads each holding one cache and reading the other can deadlock. That is open as `variants:follow_up:cross-cache-read-lock-order`; @@ -147,8 +148,10 @@ which route wrote it. `Cache` is written only by the attach, holding both gates, once and after its pull has succeeded (a failed attach never shows it, and nothing detaches), so under the cache's gate `Cache` naming that cache cannot change, and under the - store's gate it cannot change at all. The pull reads the registry and the codec - the attach lent the store, not `Cache`. A call whose guess went stale (it + store's gate it cannot change at all. The pull reads the registry the attach + lent, not `Cache`; a variant it loads carries no codec, and the cache resolves + it with its own. Until `Cache` is published the store's hold is the attach's, + so a journal the pull runs cannot write the store cacheless. A call whose guess went stale (it guessed no cache and the store then gained one) releases and retries once, so a call makes at most two passes; a cacheless write that finds a cache is refused in that same hold. A diff --git a/src/GameCult.Caching/CultCache.cs b/src/GameCult.Caching/CultCache.cs index 63bb74e6..344c94cf 100644 --- a/src/GameCult.Caching/CultCache.cs +++ b/src/GameCult.Caching/CultCache.cs @@ -2264,7 +2264,8 @@ namespace GameCult.Caching // The only writer of store.Cache, and it writes it once, last, holding the cache's gate and the store's, after the pull has // succeeded: a failed attach never publishes it, and there is no detach, so under the cache's gate Cache == this cache cannot // change, and under the store's gate Cache cannot change at all. A store call that guessed null waits on the store's gate, finds - // the cache published and retries once. + // the cache published and retries once. Until it publishes Cache the attach owns the store's hold, so a cacheless write on the + // attaching thread (a journal the pull's Admit runs) re-enters that hold and InStoreHold refuses it. Held(() => store.UnderStoreGate(() => { if (store.Cache != null) @@ -2821,13 +2822,19 @@ namespace GameCult.Caching return _gate; } - // The one lock of a store's gate. A thread holds one store's gate at a time; a store call may be entered again from inside it. - internal static T InStoreHold(object storeGate, Func body) + // The one lock of a store's gate. A thread holds one store's gate at a time; a store call may be entered again from inside it, + // except a cacheless write: it owns its store outright, so it is the outermost hold of the store's gate. + internal static T InStoreHold(object storeGate, Func body, bool cacheless = false) { if (_storeHeld != null) { if (ReferenceEquals(_storeHeld, storeGate)) + { + if (cacheless) + throw new InvalidOperationException( + "A cacheless write was made inside a call that already holds this store (an attach's pull, or a store call whose user code made it); write through the cache."); return body(); + } throw new InvalidOperationException( "A store's gate was entered inside another store's hold; user code run by a cacheless call or an attach may not enter a second store."); } @@ -3635,8 +3642,9 @@ namespace GameCult.Caching // Cache == that cache cannot change, and under the store's gate Cache cannot change at all. Attached, the call holds the cache's // gate: the store and its cache share one lock, so a load calling back into the cache never takes the gate after the store's, // and a direct call publishes what it loaded when it returns. Detached, it holds the store's gate; a cacheless call (WriteRecord, - // WriteDocument) that finds a cache there is refused in that same hold. File locks are always taken inside either gate. A call - // makes at most MaxPasses passes: Cache changes once, from null to the cache that attached the store. + // WriteDocument) that finds a cache there is refused in that same hold, and one that re-enters a hold of this store (an attach in + // its pull, or another store call's user code) is refused by InStoreHold before it reads Cache. File locks are always taken inside + // either gate. A call makes at most MaxPasses passes: Cache changes once, from null to the cache that attached the store. private T Gate(Func body, bool cacheless) { for (var pass = 0; ; pass++) @@ -3657,7 +3665,7 @@ namespace GameCult.Caching ? (true, body()) : cacheless ? throw new InvalidOperationException("The store is attached to a cache, write through the cache.") - : (false, default(T)!)); + : (false, default(T)!), cacheless); if (ran) return result; } @@ -3686,7 +3694,7 @@ namespace GameCult.Caching }, cacheless: true); // The store's own gate, taken only here. Lock order: a cache's gate, then a store's; nothing takes a store's gate and then a cache's. - internal T UnderStoreGate(Func body) => CultCache.InStoreHold(_storeGate, body); + internal T UnderStoreGate(Func body, bool cacheless = false) => CultCache.InStoreHold(_storeGate, body, cacheless); // The one writer of the lent registry: a cacheless call for the call, an attach for as long as it holds the store, and a failed // attach puts it back, so a cacheless store holds none between calls and a pull on one loads nothing. Nothing else is lent, and the diff --git a/tests/GameCult.Caching.Tests/CachelessSingleFileTests.cs b/tests/GameCult.Caching.Tests/CachelessSingleFileTests.cs index 7a0ce418..67d59a5b 100644 --- a/tests/GameCult.Caching.Tests/CachelessSingleFileTests.cs +++ b/tests/GameCult.Caching.Tests/CachelessSingleFileTests.cs @@ -1170,6 +1170,65 @@ namespace GameCult.Caching.Tests Assert.That(store.Attached, Is.Null, "the registry was taken back"); } + private const string ReentryRefusal = "A cacheless write was made inside a call that already holds this store"; + + // Until the attach publishes the cache the store's hold is the attach's, so a journal the pull's Admit runs on the attaching + // thread cannot write the store cacheless: it would write the file behind the cache and take back the registry the attach lent. + [Test] + public void AJournalOnTheAttachingThreadCannotWriteItsStoreCacheless() + { + var registry = CultDocumentRegistry.ForTypes(new[] { typeof(IdDeck) }); + new SingleFileMessagePackBackingStore(_path).WriteDocument(new CultRecordKey("a"), new IdDeck { Name = "a" }, registry: registry); + var store = new RoleStore(_path); + using var cache = new CultCache(registry, Codec(registry)); + var journalRuns = 0; + Exception? journalError = null; + using var journal = cache.AddJournal(_ => + { + journalRuns++; + try { store.WriteDocument(new CultRecordKey("b"), new IdDeck { Name = "b" }, registry: registry); } + catch (Exception error) { journalError = error; } + }); + + cache.AddBackingStore(store); + + Assert.That(journalRuns, Is.EqualTo(1), "the pull's admission ran the journal"); + Assert.That(journalError, Is.InstanceOf()); + Assert.That(journalError!.Message, Does.StartWith(ReentryRefusal)); + Assert.That(store.Attached, Is.SameAs(registry), "the refused call did not take back the attach's registry"); + Assert.That(store.ReadStoreSnapshot()!.Records.Select(record => record.Key), Is.EquivalentTo(new[] { "a" })); + + Assert.DoesNotThrow(() => cache.AddAsync(new IdDeck { Name = "later" }).GetAwaiter().GetResult()); + Assert.DoesNotThrow(() => cache.FlushAllBackingStores()); + Assert.DoesNotThrow(() => store.PullAll()); + var keys = store.ReadStoreSnapshot()!.Records.Select(record => record.Key).ToArray(); + Assert.That(keys, Has.Length.EqualTo(2)); + Assert.That(keys, Does.Contain("a")); + Assert.That(keys, Does.Not.Contain("b")); + } + + // A cacheless write owns its store outright: user code another cacheless write of the same store runs may not make a second. + [Test] + public void ACachelessWriteInsideACachelessWriteOfTheSameStoreIsRefused() + { + var registry = CultDocumentRegistry.ForTypes(new[] { typeof(TapDoc) }); + var store = new RoleStore(_path); + Exception? inner = null; + TapDoc.Hook = () => + { + try { store.WriteDocument(new CultRecordKey("inner"), new TapDoc { Name = "inner" }, registry: registry); } + catch (Exception error) { inner = error; } + }; + + var outer = Assert.Catch(() => store.WriteDocument(new CultRecordKey("outer"), new TapDoc { Name = "outer" }, registry: registry)); + + Assert.That(inner, Is.InstanceOf()); + Assert.That(inner!.Message, Does.StartWith(ReentryRefusal)); + Assert.That(Refused(outer), Is.True, "the outer write failed with the getter's exception"); + Assert.That(File.Exists(_path), Is.False); + Assert.That(store.Attached, Is.Null); + } + // The bound on a call's passes is structural: Cache goes from null to a cache once. This test writes it any other way (the // cache, then null again, under the cache's gate and the store's, in the order they are taken); the reader, which guessed null and // then the cache, is stale twice and throws instead of looping. diff --git a/tests/GameCult.Caching.Tests/ElementIdTests.cs b/tests/GameCult.Caching.Tests/ElementIdTests.cs index 3d709b74..41af34fb 100644 --- a/tests/GameCult.Caching.Tests/ElementIdTests.cs +++ b/tests/GameCult.Caching.Tests/ElementIdTests.cs @@ -918,6 +918,36 @@ namespace GameCult.Caching.Tests Assert.That(HeaderOf(path), Is.EqualTo("cultcache.store.v3"), "read without a codec, the variant it writes cannot be seen, so it is marked"); } + // Through a real attach: the second cache reads the variant from the file, and the flush it then makes writes the header its + // resolved values hold, never the one a codecless copy would claim (a codecless variant counts as holding ids). + [TestCase("name", "cultcache.store.v2")] + [TestCase("reels", "cultcache.store.v3")] + public void AnAttachedVariantStoreFlushesTheHeaderItsValuesHold(string overridden, string header) + { + var path = PathOf("attached-variant-" + overridden + ".cc"); + using (var cache = Open(path)) + { + cache.Commit(batch => + { + batch.Upsert(typeof(IdDeck), EmptyDeck("base"), new CultRecordKey("base")); + batch.UpsertVariant(new CultRecordKey("v"), new CultRecordKey("base"), new[] + { + overridden == "name" + ? cache.Override(nameof(IdDeck.Name), "v") + : cache.Override(nameof(IdDeck.Reels), new List { new() { Label = "r", Id = HexA } }), + }); + }); + } + + using (var second = Open(path)) + { + second.Commit(batch => batch.Upsert(typeof(IdDeck), EmptyDeck("other"), new CultRecordKey("other"))); + second.FlushAllBackingStores(); + } + + Assert.That(HeaderOf(path), Is.EqualTo(header)); + } + [Test] public void ADirectoryStoreIsMarkedByTheIdsItWritesAndAnEmptyDeckDoesNotMarkIt() { diff --git a/unity/org.gamecult.cultlib/CHANGELOG.md b/unity/org.gamecult.cultlib/CHANGELOG.md index 0821d622..88160f4f 100644 --- a/unity/org.gamecult.cultlib/CHANGELOG.md +++ b/unity/org.gamecult.cultlib/CHANGELOG.md @@ -23,7 +23,7 @@ Releases as 3.0.0 (major). ### Added -- A cacheless `WriteRecord` or `WriteDocument`, and an attach, hold the store's gate while they run, and user code they run (a document's property getters, a constructor while a copied record is read) now throws `InvalidOperationException` if it enters a cache it does not hold or a second store; it used to wait forever against an attach of the same store. A store call that queued on an attach that then failed runs under the gate that owns the store, not beside a cacheless call or a second cache's attach. An attach that fails takes its registry back, so a `PullAll` on the store afterwards is refused as unattached instead of loading records the next cache never sees. +- A cacheless `WriteRecord` or `WriteDocument`, and an attach, hold the store's gate while they run, and user code they run (a document's property getters, a constructor while a copied record is read) now throws `InvalidOperationException` if it enters a cache it does not hold or a second store; it used to wait forever against an attach of the same store. A store call that queued on an attach that then failed runs under the gate that owns the store, not beside a cacheless call or a second cache's attach. An attach that fails takes its registry back, so a `PullAll` on the store afterwards is refused as unattached instead of loading records the next cache never sees. A cacheless `WriteRecord` or `WriteDocument` made while the same thread already holds that store (from a journal an attach's pull runs, or from user code another cacheless write of the store runs) throws `InvalidOperationException`; it used to write the file behind the attaching cache and take that cache's registry, so the cache could never flush or pull the store again. - `SingleFileBackingStore.ReadStoreSnapshot()` reads a store file once, under the store's in-process gate, with the reader a cache's store opens with, and returns the durable snapshot or null when nothing is at the path. `SingleFileBackingStore.WriteRecord(record, catalogEntry, holdsIds, registry)` writes one record without a cache: under the store's lock every other record and catalog entry is copied as stored, the file is replaced atomically, and the header is decided as a flush decides it. It refuses on a store attached to a cache. - `SingleFileBackingStore.WriteDocument(key, document, storedAt, registry)` writes one typed document without a cache, as a cache write does: it plans element ids, encodes the document as `T` (the type its schema id and ids are taken from, also for a derived instance) and marks the store when it holds an id; a failed write gives the minted ids back. It refuses on a store attached to a cache. The `registry` of `WriteRecord` and `WriteDocument` serves that call only; a store keeps none. - `CultCodec.DecodeDocument(key, payload)` decodes a stored payload as a cache load does (`CultCacheMessagePack.CreateCodec(registry)` makes the codec). It reads no file and holds no state. `CultElementIds` stays internal.