From dfe6f2029ac5d65aa8bac1c91f27bc0da5ca5253 Mon Sep 17 00:00:00 2001 From: Stephan Date: Thu, 14 Mar 2019 19:48:44 +0100 Subject: [PATCH] NuCache: better fixing, cleanup --- .../Scoping/ScopeContextualBase.cs | 7 +- .../Cache/SnapDictionaryTests.cs | 67 +++++-------------- .../PublishedCache/NuCache/ContentStore.cs | 13 ++-- .../NuCache/PublishedSnapshotService.cs | 18 ++--- .../PublishedCache/NuCache/SnapDictionary.cs | 18 ++--- 5 files changed, 39 insertions(+), 84 deletions(-) diff --git a/src/Umbraco.Core/Scoping/ScopeContextualBase.cs b/src/Umbraco.Core/Scoping/ScopeContextualBase.cs index 1f2b6155e692..25f176d4719a 100644 --- a/src/Umbraco.Core/Scoping/ScopeContextualBase.cs +++ b/src/Umbraco.Core/Scoping/ScopeContextualBase.cs @@ -12,7 +12,7 @@ namespace Umbraco.Core.Scoping /// public abstract class ScopeContextualBase : IDisposable { - private bool _using, _scoped; + private bool _scoped; /// /// Gets a contextual object. @@ -38,9 +38,6 @@ public static T Get(IScopeProvider scopeProvider, string key, Func c () => ctor(true), (completed, item) => { item.Release(completed); }); - // the object can be 'used' only once at a time - if (w._using) throw new InvalidOperationException("panic: used."); - w._using = true; w._scoped = true; return w; @@ -52,8 +49,6 @@ public static T Get(IScopeProvider scopeProvider, string key, Func c /// public void Dispose() { - _using = false; - if (_scoped == false) Release(true); } diff --git a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs index e4ca35fbd360..b435af9e7721 100644 --- a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs +++ b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs @@ -632,7 +632,7 @@ public void WriteLockingFirstSnapshot() Assert.AreEqual(1, d.Test.LiveGen); Assert.IsTrue(d.Test.NextGen); - using (d.GetWriter(GetScopeProvider())) + using (d.GetScopedWriteLock(GetScopeProvider())) { var s1 = d.CreateSnapshot(); @@ -685,7 +685,7 @@ public void WriteLocking() Assert.IsFalse(d.Test.NextGen); Assert.AreEqual("uno", s2.Get(1)); - using (d.GetWriter(GetScopeProvider())) + using (d.GetScopedWriteLock(GetScopeProvider())) { // gen 3 Assert.AreEqual(2, d.Test.GetValues(1).Length); @@ -724,13 +724,13 @@ public void NestedWriteLocking1() var scopeProvider = GetScopeProvider(); - using (var w1 = d.GetWriter(scopeProvider)) + using (var w1 = d.GetScopedWriteLock(scopeProvider)) { Assert.AreEqual(1, t.LiveGen); Assert.AreEqual(1, t.WLocked); Assert.IsTrue(t.NextGen); - using (var w2 = d.GetWriter(scopeProvider)) + using (var w2 = d.GetScopedWriteLock(scopeProvider)) { Assert.AreEqual(1, t.LiveGen); Assert.AreEqual(2, t.WLocked); @@ -770,9 +770,9 @@ public void NestedWriteLocking2() var scopeContext = new ScopeContext(); var scopeProvider = GetScopeProvider(scopeContext); - using (var w1 = d.GetWriter(scopeProvider)) + using (var w1 = d.GetScopedWriteLock(scopeProvider)) { - using (var w2 = d.GetWriter(scopeProvider)) + using (var w2 = d.GetScopedWriteLock(scopeProvider)) { Assert.AreSame(w1, w2); @@ -794,13 +794,13 @@ public void NestedWriteLocking3() var scopeProvider1 = GetScopeProvider(); var scopeProvider2 = GetScopeProvider(scopeContext); - using (var w1 = d.GetWriter(scopeProvider1)) + using (var w1 = d.GetScopedWriteLock(scopeProvider1)) { Assert.AreEqual(1, t.LiveGen); Assert.AreEqual(1, t.WLocked); Assert.IsTrue(t.NextGen); - using (var w2 = d.GetWriter(scopeProvider2)) + using (var w2 = d.GetScopedWriteLock(scopeProvider2)) { Assert.AreEqual(1, t.LiveGen); Assert.AreEqual(2, t.WLocked); @@ -850,7 +850,7 @@ public void WriteLocking2() var scopeProvider = GetScopeProvider(); - using (d.GetWriter(scopeProvider)) + using (d.GetScopedWriteLock(scopeProvider)) { // gen 3 Assert.AreEqual(2, d.Test.GetValues(1).Length); @@ -895,7 +895,7 @@ public void WriteLocking3() var scopeProvider = GetScopeProvider(); - using (d.GetWriter(scopeProvider)) + using (d.GetScopedWriteLock(scopeProvider)) { // creating a snapshot in a write-lock does NOT return the "current" content // it uses the previous snapshot, so new snapshot created only on release @@ -935,7 +935,7 @@ public void ScopeLocking1() var scopeContext = new ScopeContext(); var scopeProvider = GetScopeProvider(scopeContext); - using (d.GetWriter(scopeProvider)) + using (d.GetScopedWriteLock(scopeProvider)) { // creating a snapshot in a write-lock does NOT return the "current" content // it uses the previous snapshot, so new snapshot created only on release @@ -985,7 +985,7 @@ public void ScopeLocking2() var scopeContext = new ScopeContext(); var scopeProvider = GetScopeProvider(scopeContext); - using (d.GetWriter(scopeProvider)) + using (d.GetScopedWriteLock(scopeProvider)) { // creating a snapshot in a write-lock does NOT return the "current" content // it uses the previous snapshot, so new snapshot created only on release @@ -1015,45 +1015,8 @@ public void ScopeLocking2() Assert.AreEqual(2, s4.Gen); Assert.AreEqual("uno", s4.Get(1)); - // fixme - remove debugging code - /* - Exception caught = null; - var genFlip = 0; - var lckFlip = 0; - var thread = new System.Threading.Thread(() => - { - try - { - for (var i = 0; i < 20; i++) - { - if (t.LiveGen == 2 && genFlip == 0) genFlip = i; // flips at 1 - if (t.WLocked == 0 && lckFlip == 0) lckFlip = i; // flips at 10 ie 5s, as expected - d.CreateSnapshot(); - System.Threading.Thread.Sleep(500); - } - } - catch (Exception e) - { - caught = e; - } - }); - thread.Start(); - */ - scopeContext.ScopeExit(false); - // fixme - remove debugging code - /* - thread.Join(); - - Assert.IsNull(caught); // but then how can it be not null? - - Console.WriteLine(genFlip); - Console.WriteLine(lckFlip); - Assert.AreEqual(1, genFlip); - Assert.AreEqual(10, lckFlip); - */ - // now things have changed Assert.AreEqual(2, t.LiveGen); Assert.IsFalse(t.NextGen); @@ -1104,13 +1067,13 @@ 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 + Assert.IsNull(d.Test.GenObj); // 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 + Assert.IsNull(d.Test.GenObj); var s1 = d.CreateSnapshot(); Assert.IsFalse(d.Test.NextGen); @@ -1134,7 +1097,7 @@ public void DontPanic() // writer is scope contextual and scoped // when disposed, nothing happens // when the context exists, the writer is released - using (d.GetWriter(scopeProvider)) + using (d.GetScopedWriteLock(scopeProvider)) { d.Set(1, "ein"); Assert.IsTrue(d.Test.NextGen); diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index 2b9c9bee4db5..7ab4a64f3108 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -92,13 +92,13 @@ private class WriteLockInfo } // a scope contextual that represents a locked writer to the dictionary - private class ContentStoreWriter : ScopeContextualBase + private class ScopedWriteLock : ScopeContextualBase { private readonly WriteLockInfo _lockinfo = new WriteLockInfo(); private readonly ContentStore _store; private int _released; - public ContentStoreWriter(ContentStore store, bool scoped) + public ScopedWriteLock(ContentStore store, bool scoped) { _store = store; store.Lock(_lockinfo, scoped); @@ -114,9 +114,9 @@ public override void Release(bool completed) // gets a scope contextual representing a locked writer to the dictionary // TODO: GetScopedWriter? should the dict have a ref onto the scope provider? - public IDisposable GetWriter(IScopeProvider scopeProvider) + public IDisposable GetScopedWriteLock(IScopeProvider scopeProvider) { - return ScopeContextualBase.Get(scopeProvider, _instanceId, scoped => new ContentStoreWriter(this, scoped)); + return ScopeContextualBase.Get(scopeProvider, _instanceId, scoped => new ScopedWriteLock(this, scoped)); } private void Lock(WriteLockInfo lockInfo, bool forceGen = false) @@ -137,9 +137,10 @@ private void Lock(WriteLockInfo lockInfo, bool forceGen = false) { // because we are changing things, a new generation // is created, which will trigger a new snapshot - _nextGen = true; - _genObjs.Enqueue(_genObj = new GenObj(_liveGen)); + if (_nextGen) + _genObjs.Enqueue(_genObj = new GenObj(_liveGen)); _liveGen += 1; + _nextGen = true; } } } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs index 541ff2ea2336..9c5587fbd50c 100755 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs @@ -333,7 +333,7 @@ private void LockAndLoadContent(Action action) // first get a writer, then a scope // if there already is a scope, the writer will attach to it // otherwise, it will only exist here - cheap - using (_contentStore.GetWriter(_scopeProvider)) + using (_contentStore.GetScopedWriteLock(_scopeProvider)) using (var scope = _scopeProvider.CreateScope()) { scope.ReadLock(Constants.Locks.ContentTree); @@ -401,7 +401,7 @@ private void LoadContentFromLocalDbLocked(IScope scope) private void LockAndLoadMedia(Action action) { // see note in LockAndLoadContent - using (_mediaStore.GetWriter(_scopeProvider)) + using (_mediaStore.GetScopedWriteLock(_scopeProvider)) using (var scope = _scopeProvider.CreateScope()) { scope.ReadLock(Constants.Locks.MediaTree); @@ -529,7 +529,7 @@ private void LoadMediaFromLocalDbLocked(IScope scope) private void LockAndLoadDomains() { // see note in LockAndLoadContent - using (_domainStore.GetWriter(_scopeProvider)) + using (_domainStore.GetScopedWriteLock(_scopeProvider)) using (var scope = _scopeProvider.CreateScope()) { scope.ReadLock(Constants.Locks.Domains); @@ -586,7 +586,7 @@ public override void Notify(ContentCacheRefresher.JsonPayload[] payloads, out bo return; } - using (_contentStore.GetWriter(_scopeProvider)) + using (_contentStore.GetScopedWriteLock(_scopeProvider)) { NotifyLocked(payloads, out bool draftChanged2, out bool publishedChanged2); draftChanged = draftChanged2; @@ -682,7 +682,7 @@ public override void Notify(MediaCacheRefresher.JsonPayload[] payloads, out bool return; } - using (_mediaStore.GetWriter(_scopeProvider)) + using (_mediaStore.GetScopedWriteLock(_scopeProvider)) { NotifyLocked(payloads, out bool anythingChanged2); anythingChanged = anythingChanged2; @@ -803,7 +803,7 @@ private void Notify(ContentStore store, ContentTypeCacheRefresher.JsonPayload if (removedIds.Count == 0 && refreshedIds.Count == 0 && otherIds.Count == 0 && newIds.Count == 0) return; - using (store.GetWriter(_scopeProvider)) + using (store.GetScopedWriteLock(_scopeProvider)) { // ReSharper disable AccessToModifiedClosure action(removedIds, refreshedIds, otherIds, newIds); @@ -824,8 +824,8 @@ public override void Notify(DataTypeCacheRefresher.JsonPayload[] payloads) payload.Removed ? "Removed" : "Refreshed", payload.Id); - using (_contentStore.GetWriter(_scopeProvider)) - using (_mediaStore.GetWriter(_scopeProvider)) + using (_contentStore.GetScopedWriteLock(_scopeProvider)) + using (_mediaStore.GetScopedWriteLock(_scopeProvider)) { // TODO: need to add a datatype lock // this is triggering datatypes reload in the factory, and right after we create some @@ -858,7 +858,7 @@ public override void Notify(DomainCacheRefresher.JsonPayload[] payloads) return; // see note in LockAndLoadContent - using (_domainStore.GetWriter(_scopeProvider)) + using (_domainStore.GetScopedWriteLock(_scopeProvider)) { foreach (var payload in payloads) { diff --git a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs index 73904d145201..c5b1df120648 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs @@ -87,13 +87,13 @@ private class WriteLockInfo } // a scope contextual that represents a locked writer to the dictionary - private class SnapDictionaryWriter : ScopeContextualBase + private class ScopedWriteLock : ScopeContextualBase { private readonly WriteLockInfo _lockinfo = new WriteLockInfo(); private readonly SnapDictionary _dictionary; private int _released; - public SnapDictionaryWriter(SnapDictionary dictionary, bool scoped) + public ScopedWriteLock(SnapDictionary dictionary, bool scoped) { _dictionary = dictionary; dictionary.Lock(_lockinfo, scoped); @@ -108,14 +108,12 @@ public override void Release(bool completed) } // gets a scope contextual representing a locked writer to the dictionary - // fixme GetScopedWriter? should the dict have a ref onto the scope provider? - // fixme this is not a "writer" but a "write lock" => rename GetWriteLock // the dict is write-locked until the write-lock is released // which happens when it is disposed (non-scoped) // or when the scope context exits (scoped) - public IDisposable GetWriter(IScopeProvider scopeProvider) + public IDisposable GetScopedWriteLock(IScopeProvider scopeProvider) { - return ScopeContextualBase.Get(scopeProvider, _instanceId, scoped => new SnapDictionaryWriter(this, scoped)); + return ScopeContextualBase.Get(scopeProvider, _instanceId, scoped => new ScopedWriteLock(this, scoped)); } private void Lock(WriteLockInfo lockInfo, bool forceGen = false) @@ -143,9 +141,10 @@ private void Lock(WriteLockInfo lockInfo, bool forceGen = false) { // 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)); + if (_nextGen) + _genObjs.Enqueue(_genObj = new GenObj(_liveGen)); _liveGen += 1; + _nextGen = true; // this is the ONLY place where _nextGen becomes true } } } @@ -197,9 +196,6 @@ private void Release(WriteLockInfo lockInfo, bool commit = true) } } - // fixme - pretend we need to do something that takes time - //System.Threading.Thread.Sleep(TimeSpan.FromSeconds(5)); - // decrement the lock count, if counting, then exit the lock if (lockInfo.Count) _wlocked--; Monitor.Exit(_wlocko);