From 1caf7f458720b219488de9e43aafce81ae6838ea Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 25 Sep 2019 13:51:19 +0200 Subject: [PATCH] Fixes regression issue in nucache causing a panic exception --- .../PublishedContent/NuCacheChildrenTests.cs | 193 ++++++++++++++++-- .../PublishedCache/NuCache/ContentStore.cs | 25 ++- .../NuCache/PublishedSnapshotService.cs | 7 + 3 files changed, 195 insertions(+), 30 deletions(-) diff --git a/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs b/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs index fbcdf7731a..998c9589b0 100644 --- a/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs +++ b/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs @@ -108,8 +108,8 @@ namespace Umbraco.Tests.PublishedContent ); // create a scope provider - var scopeProvider = Mock.Of(); - Mock.Get(scopeProvider) + var scopeProvider = new Mock(); + scopeProvider .Setup(x => x.CreateScope( It.IsAny(), It.IsAny(), @@ -143,7 +143,7 @@ namespace Umbraco.Tests.PublishedContent _snapshotAccessor, _variationAccesor, Mock.Of(), - scopeProvider, + scopeProvider.Object, Mock.Of(), Mock.Of(), Mock.Of(), @@ -956,8 +956,10 @@ namespace Umbraco.Tests.PublishedContent } [Test] - public void Issue6353() + public void Set_All_Fast_Sorted_Ensure_LastChildContentId() { + //see https://github.com/umbraco/Umbraco-CMS/issues/6353 + IEnumerable GetKits() { var paths = new Dictionary { { -1, "-1" } }; @@ -968,28 +970,181 @@ namespace Umbraco.Tests.PublishedContent Init(GetKits()); - var snapshotService = (PublishedSnapshotService) _snapshotService; - var contentStoreField = typeof(PublishedSnapshotService).GetField("_contentStore", BindingFlags.Instance | BindingFlags.NonPublic); - var contentStore = (ContentStore) contentStoreField.GetValue(snapshotService); - var contentNodesField = typeof(ContentStore).GetField("_contentNodes", BindingFlags.Instance | BindingFlags.NonPublic); - var contentNodes = (ConcurrentDictionary>) contentNodesField.GetValue(contentStore); + var snapshotService = (PublishedSnapshotService)_snapshotService; + var contentStore = snapshotService.GetContentStore(); - var parentNode = contentNodes[1].Value; - Assert.AreEqual(-1, parentNode.PreviousSiblingContentId); - Assert.AreEqual(-1, parentNode.NextSiblingContentId); - Assert.AreEqual(2, parentNode.FirstChildContentId); - Assert.AreEqual(2, parentNode.LastChildContentId); + var parentNodes = contentStore.Test.GetValues(1); + var parentNode = parentNodes[0]; + AssertLinkedNode(parentNode.contentNode, -1, -1, -1, 2, 2); _snapshotService.Notify(new[] { new ContentCacheRefresher.JsonPayload(2, TreeChangeTypes.Remove) }, out _, out _); - parentNode = contentNodes[1].Value; - Assert.AreEqual(-1, parentNode.PreviousSiblingContentId); - Assert.AreEqual(-1, parentNode.NextSiblingContentId); - Assert.AreEqual(-1, parentNode.FirstChildContentId); - Assert.AreEqual(-1, parentNode.LastChildContentId); + parentNodes = contentStore.Test.GetValues(1); + parentNode = parentNodes[0]; + + AssertLinkedNode(parentNode.contentNode, -1, -1, -1, -1, -1); + } + + [Test] + public void Remove_Node_Ensures_Linked_List() + { + // NOTE: these tests are not using real scopes, in which case a Scope does not control + // how the snapshots generations work. We are forcing new snapshot generations manually. + + IEnumerable GetKits() + { + var paths = new Dictionary { { -1, "-1" } }; + + //root + yield return CreateInvariantKit(1, -1, 1, paths); + + //children + yield return CreateInvariantKit(2, 1, 1, paths); + yield return CreateInvariantKit(3, 1, 2, paths); //middle child + yield return CreateInvariantKit(4, 1, 3, paths); + } + + Init(GetKits()); + + var snapshotService = (PublishedSnapshotService)_snapshotService; + var contentStore = snapshotService.GetContentStore(); + + Assert.AreEqual(1, contentStore.Test.LiveGen); + Assert.IsTrue(contentStore.Test.NextGen); + + var parentNode = contentStore.Test.GetValues(1)[0]; + Assert.AreEqual(1, parentNode.gen); + AssertLinkedNode(parentNode.contentNode, -1, -1, -1, 2, 4); + + var child1 = contentStore.Test.GetValues(2)[0]; + Assert.AreEqual(1, child1.gen); + AssertLinkedNode(child1.contentNode, 1, -1, 3, -1, -1); + + var child2 = contentStore.Test.GetValues(3)[0]; + Assert.AreEqual(1, child2.gen); + AssertLinkedNode(child2.contentNode, 1, 2, 4, -1, -1); + + var child3 = contentStore.Test.GetValues(4)[0]; + Assert.AreEqual(1, child3.gen); + AssertLinkedNode(child3.contentNode, 1, 3, -1, -1, -1); + + //This will set a flag to force creating a new Gen next time the store is locked (i.e. In Notify) + contentStore.CreateSnapshot(); + + Assert.IsFalse(contentStore.Test.NextGen); + + _snapshotService.Notify(new[] + { + new ContentCacheRefresher.JsonPayload(3, TreeChangeTypes.Remove) //remove middle child + }, out _, out _); + + Assert.AreEqual(2, contentStore.Test.LiveGen); + Assert.IsTrue(contentStore.Test.NextGen); + + var parentNodes = contentStore.Test.GetValues(1); + Assert.AreEqual(1, parentNodes.Length); // the parent doesn't get changed, not new gen's are added + parentNode = parentNodes[0]; + Assert.AreEqual(1, parentNode.gen); // the parent node's gen has not changed + AssertLinkedNode(parentNode.contentNode, -1, -1, -1, 2, 4); + + child1 = contentStore.Test.GetValues(2)[0]; + Assert.AreEqual(2, child1.gen); // there is now 2x gen's of this item + AssertLinkedNode(child1.contentNode, 1, -1, 4, -1, -1); + + child2 = contentStore.Test.GetValues(3)[0]; + Assert.AreEqual(2, child2.gen); // there is now 2x gen's of this item + Assert.IsNull(child2.contentNode); // because it doesn't exist anymore + + child3 = contentStore.Test.GetValues(4)[0]; + Assert.AreEqual(2, child3.gen); // there is now 2x gen's of this item + AssertLinkedNode(child3.contentNode, 1, 2, -1, -1, -1); + } + + [Test] + public void Refresh_Branch_Ensures_Linked_List() + { + // NOTE: these tests are not using real scopes, in which case a Scope does not control + // how the snapshots generations work. We are forcing new snapshot generations manually. + + IEnumerable GetKits() + { + var paths = new Dictionary { { -1, "-1" } }; + + //root + yield return CreateInvariantKit(1, -1, 1, paths); + + //children + yield return CreateInvariantKit(2, 1, 1, paths); + yield return CreateInvariantKit(3, 1, 2, paths); //middle child + yield return CreateInvariantKit(4, 1, 3, paths); + } + + Init(GetKits()); + + var snapshotService = (PublishedSnapshotService)_snapshotService; + var contentStore = snapshotService.GetContentStore(); + + Assert.AreEqual(1, contentStore.Test.LiveGen); + Assert.IsTrue(contentStore.Test.NextGen); + + var parentNode = contentStore.Test.GetValues(1)[0]; + Assert.AreEqual(1, parentNode.gen); + AssertLinkedNode(parentNode.contentNode, -1, -1, -1, 2, 4); + + var child1 = contentStore.Test.GetValues(2)[0]; + Assert.AreEqual(1, child1.gen); + AssertLinkedNode(child1.contentNode, 1, -1, 3, -1, -1); + + var child2 = contentStore.Test.GetValues(3)[0]; + Assert.AreEqual(1, child2.gen); + AssertLinkedNode(child2.contentNode, 1, 2, 4, -1, -1); + + var child3 = contentStore.Test.GetValues(4)[0]; + Assert.AreEqual(1, child3.gen); + AssertLinkedNode(child3.contentNode, 1, 3, -1, -1, -1); + + //This will set a flag to force creating a new Gen next time the store is locked (i.e. In Notify) + contentStore.CreateSnapshot(); + + Assert.IsFalse(contentStore.Test.NextGen); + + _snapshotService.Notify(new[] + { + new ContentCacheRefresher.JsonPayload(3, TreeChangeTypes.RefreshBranch) //remove middle child + }, out _, out _); + + Assert.AreEqual(2, contentStore.Test.LiveGen); + Assert.IsTrue(contentStore.Test.NextGen); + + var parentNodes = contentStore.Test.GetValues(1); + Assert.AreEqual(1, parentNodes.Length); // the parent doesn't get changed, not new gen's are added + parentNode = parentNodes[0]; + Assert.AreEqual(1, parentNode.gen); // the parent node's gen has not changed + AssertLinkedNode(parentNode.contentNode, -1, -1, -1, 2, 4); + + child1 = contentStore.Test.GetValues(2)[0]; + Assert.AreEqual(2, child1.gen); // there is now 2x gen's of this item + AssertLinkedNode(child1.contentNode, 1, -1, 3, -1, -1); + + child2 = contentStore.Test.GetValues(3)[0]; + Assert.AreEqual(2, child2.gen); // there is now 2x gen's of this item + AssertLinkedNode(child2.contentNode, 1, 2, 4, -1, -1); + + child3 = contentStore.Test.GetValues(4)[0]; + Assert.AreEqual(2, child3.gen); // there is now 2x gen's of this item + AssertLinkedNode(child3.contentNode, 1, 3, -1, -1, -1); + } + + private void AssertLinkedNode(ContentNode node, int parent, int prevSibling, int nextSibling, int firstChild, int lastChild) + { + Assert.AreEqual(parent, node.ParentContentId); + Assert.AreEqual(prevSibling, node.PreviousSiblingContentId); + Assert.AreEqual(nextSibling, node.NextSiblingContentId); + Assert.AreEqual(firstChild, node.FirstChildContentId); + Assert.AreEqual(lastChild, node.LastChildContentId); } private void AssertDocuments(IPublishedContent[] documents, params string[] names) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index c7fc389cb1..f0d695f090 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -953,8 +953,6 @@ namespace Umbraco.Web.PublishedCache.NuCache var parent = parentLink.Value; - var currentGen = parentLink.Gen; - // if parent has no children, clone parent + add as first child if (parent.FirstChildContentId < 0) { @@ -965,7 +963,7 @@ namespace Umbraco.Web.PublishedCache.NuCache } // get parent's first child - var childLink = GetRequiredLinkedNode(parent.FirstChildContentId, "first child", currentGen); + var childLink = GetRequiredLinkedNode(parent.FirstChildContentId, "first child", null); var child = childLink.Value; // if first, clone parent + insert as first child @@ -985,7 +983,7 @@ namespace Umbraco.Web.PublishedCache.NuCache } // get parent's last child - var lastChildLink = GetRequiredLinkedNode(parent.LastChildContentId, "last child", currentGen); + var lastChildLink = GetRequiredLinkedNode(parent.LastChildContentId, "last child", null); var lastChild = lastChildLink.Value; // if last, clone parent + append as last child @@ -1010,7 +1008,7 @@ namespace Umbraco.Web.PublishedCache.NuCache while (child.NextSiblingContentId > 0) { // get next child - var nextChildLink = GetRequiredLinkedNode(child.NextSiblingContentId, "next child", currentGen); + var nextChildLink = GetRequiredLinkedNode(child.NextSiblingContentId, "next child", null); var nextChild = nextChildLink.Value; // if here, clone previous + append/insert @@ -1034,7 +1032,7 @@ namespace Umbraco.Web.PublishedCache.NuCache } // should never get here - throw new Exception("panic: no more children."); + throw new PanicException("No more children."); } // replaces the root node @@ -1370,7 +1368,7 @@ namespace Umbraco.Web.PublishedCache.NuCache #endregion - #region Unit testing + #region Internals/Unit testing private TestHelper _unitTesting; @@ -1393,17 +1391,22 @@ namespace Umbraco.Web.PublishedCache.NuCache set => _store._collectAuto = value; } - public Tuple[] GetValues(int id) + /// + /// Return a list of Gen/ContentNode values + /// + /// + /// + public (long gen, ContentNode contentNode)[] GetValues(int id) { _store._contentNodes.TryGetValue(id, out LinkedNode link); // else null if (link == null) - return new Tuple[0]; + return Array.Empty<(long, ContentNode)>(); - var tuples = new List>(); + var tuples = new List<(long, ContentNode)>(); do { - tuples.Add(Tuple.Create(link.Gen, link.Value)); + tuples.Add((link.Gen, link.Value)); link = link.Next; } while (link != null); return tuples.ToArray(); diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs index 4239eda9f5..9522e76622 100755 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs @@ -1688,5 +1688,12 @@ AND cmsContentNu.nodeId IS NULL } #endregion + + #region Internals/Testing + + internal ContentStore GetContentStore() => _contentStore; + internal ContentStore GetMediaStore() => _mediaStore; + + #endregion } }