From 851c844c8b3268909b0939bb3ec8a5202ea554c5 Mon Sep 17 00:00:00 2001 From: Stephan Date: Thu, 14 Mar 2019 18:56:23 +0100 Subject: [PATCH] NuCache: fix panic exception --- .../Cache/SnapDictionaryTests.cs | 81 +++++++++++++++++++ .../PublishedCache/NuCache/ContentStore.cs | 4 +- .../PublishedCache/NuCache/SnapDictionary.cs | 15 +--- 3 files changed, 87 insertions(+), 13 deletions(-) diff --git a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs index eb034eec26..e4ca35fbd3 100644 --- a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs +++ b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs @@ -1098,6 +1098,87 @@ namespace Umbraco.Tests.Cache Assert.AreEqual("four", all[3]); } + [Test] + public void DontPanic() + { + var d = new SnapDictionary(); + d.Test.CollectAuto = false; + + Assert.IsNull(d.Test.GenObj); // set with first snapshot or first lock, then never null + + // gen 1 + d.Set(1, "one"); + Assert.IsTrue(d.Test.NextGen); + Assert.AreEqual(1, d.Test.LiveGen); + Assert.IsNotNull(d.Test.GenObj); // set with lock + + var s1 = d.CreateSnapshot(); + Assert.IsFalse(d.Test.NextGen); + Assert.AreEqual(1, d.Test.LiveGen); + Assert.IsNotNull(d.Test.GenObj); + Assert.AreEqual(1, d.Test.GenObj.Gen); + + Assert.AreEqual(1, s1.Gen); + Assert.AreEqual("one", s1.Get(1)); + + d.Set(1, "uno"); + Assert.IsTrue(d.Test.NextGen); + Assert.AreEqual(2, d.Test.LiveGen); + Assert.IsNotNull(d.Test.GenObj); + Assert.AreEqual(1, d.Test.GenObj.Gen); + + var scopeContext = new ScopeContext(); + var scopeProvider = GetScopeProvider(scopeContext); + + // scopeProvider.Context == scopeContext -> writer is scoped + // writer is scope contextual and scoped + // when disposed, nothing happens + // when the context exists, the writer is released + using (d.GetWriter(scopeProvider)) + { + d.Set(1, "ein"); + Assert.IsTrue(d.Test.NextGen); + Assert.AreEqual(3, d.Test.LiveGen); + Assert.IsNotNull(d.Test.GenObj); + Assert.AreEqual(2, d.Test.GenObj.Gen); + } + + // writer has not released + Assert.AreEqual(1, d.Test.WLocked); + Assert.IsNotNull(d.Test.GenObj); + Assert.AreEqual(2, d.Test.GenObj.Gen); + + // nothing changed + Assert.IsTrue(d.Test.NextGen); + Assert.AreEqual(3, d.Test.LiveGen); + + // panic! + var s2 = d.CreateSnapshot(); + + Assert.AreEqual(1, d.Test.WLocked); + Assert.IsNotNull(d.Test.GenObj); + Assert.AreEqual(2, d.Test.GenObj.Gen); + Assert.AreEqual(3, d.Test.LiveGen); + Assert.IsTrue(d.Test.NextGen); + + // release writer + scopeContext.ScopeExit(true); + + Assert.AreEqual(0, d.Test.WLocked); + Assert.IsNotNull(d.Test.GenObj); + Assert.AreEqual(2, d.Test.GenObj.Gen); + Assert.AreEqual(3, d.Test.LiveGen); + Assert.IsTrue(d.Test.NextGen); + + var s3 = d.CreateSnapshot(); + + Assert.AreEqual(0, d.Test.WLocked); + Assert.IsNotNull(d.Test.GenObj); + Assert.AreEqual(3, d.Test.GenObj.Gen); + Assert.AreEqual(3, d.Test.LiveGen); + Assert.IsFalse(d.Test.NextGen); + } + private IScopeProvider GetScopeProvider(ScopeContext scopeContext = null) { var scopeProvider = Mock.Of(); diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index 2548046e23..2b9c9bee4d 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -133,11 +133,12 @@ namespace Umbraco.Web.PublishedCache.NuCache { _wlocked++; lockInfo.Count = true; - if (_nextGen == false || (forceGen && _wlocked == 1)) // if true already... ok to have "holes" in generation objects + if (_nextGen == false || (forceGen && _wlocked == 1)) { // because we are changing things, a new generation // is created, which will trigger a new snapshot _nextGen = true; + _genObjs.Enqueue(_genObj = new GenObj(_liveGen)); _liveGen += 1; } } @@ -214,7 +215,6 @@ namespace Umbraco.Web.PublishedCache.NuCache else dictionary.TryUpdate(key, link.Next, link); } - } #endregion diff --git a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs index 1d462f1b76..73904d1452 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs @@ -139,18 +139,12 @@ namespace Umbraco.Web.PublishedCache.NuCache _wlocked++; lockInfo.Count = true; - // fixme - this comment: "ok to have holes in generation objects" is annoying - // 'cos if you have a hole, then doing _liveGen-1 in some places would be wrong - // besides, forceGen is used only when getting a scoped write lock, which is not reentrant? - // - // but... if _wlocked ==1 and we just incremented it, it means it was 0, so it wasnt locked, so wtf? - // this is the only place where _nextGen would turn true so ... this makes no sense at all! - - if (_nextGen == false || (forceGen && _wlocked == 1)) // if true already... ok to have "holes" in generation objects + if (_nextGen == false || (forceGen && _wlocked == 1)) { // because we are changing things, a new generation // is created, which will trigger a new snapshot _nextGen = true; // this is the ONLY place where _nextGen becomes true + _genObjs.Enqueue(_genObj = new GenObj(_liveGen)); _liveGen += 1; } } @@ -379,9 +373,6 @@ namespace Umbraco.Web.PublishedCache.NuCache if (_genObj == null) _genObjs.Enqueue(_genObj = new GenObj(snapGen)); - // fixme - getting a panic exception here - // means _genObj != null, means _nextGen == true - // if we have one already, ensure it's consistent else if (_genObj.Gen != snapGen) throw new Exception("panic"); @@ -551,6 +542,8 @@ namespace Umbraco.Web.PublishedCache.NuCache set => _dict._collectAuto = value; } + public GenObj GenObj => _dict._genObj; + public ConcurrentQueue GenObjs => _dict._genObjs; public Snapshot LiveSnapshot => new Snapshot(_dict, _dict._liveGen);