From ea72b0a7b6a113f76925157ed741804cc3156b45 Mon Sep 17 00:00:00 2001 From: Stephan Date: Fri, 14 Jul 2017 12:51:57 +0200 Subject: [PATCH] NuCache+Scope - more SnapDictionary scoping --- .../Scoping/ScopeContextualBase.cs | 6 +- .../Cache/SnapDictionaryTests.cs | 41 ++-- .../PublishedCache/NuCache/FacadeService.cs | 8 +- .../PublishedCache/NuCache/SnapDictionary.cs | 183 +++++++++--------- 4 files changed, 123 insertions(+), 115 deletions(-) diff --git a/src/Umbraco.Core/Scoping/ScopeContextualBase.cs b/src/Umbraco.Core/Scoping/ScopeContextualBase.cs index df19d7b2ea..f7f5f852d6 100644 --- a/src/Umbraco.Core/Scoping/ScopeContextualBase.cs +++ b/src/Umbraco.Core/Scoping/ScopeContextualBase.cs @@ -9,15 +9,15 @@ namespace Umbraco.Core.Scoping { private bool _using, _scoped; - public static T Get(IScopeProvider scopeProvider, string key, Func ctor) + public static T Get(IScopeProvider scopeProvider, string key, Func ctor) where T : ScopeContextualBase { var scopeContext = scopeProvider.Context; if (scopeContext == null) - return ctor(); + return ctor(false); var w = scopeContext.Enlist("ScopeContextualBase_" + key, - ctor, + () => ctor(true), (completed, item) => { item.Release(completed); }); if (w._using) throw new InvalidOperationException("panic: used."); diff --git a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs index a73776badf..013dbadbb8 100644 --- a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs +++ b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs @@ -632,7 +632,7 @@ namespace Umbraco.Tests.Cache Assert.AreEqual(1, d.Test.LiveGen); Assert.IsTrue(d.Test.NextGen); - d.WriteLocked(() => + using (d.GetWriter(GetScopeProvider())) { var s1 = d.CreateSnapshot(); @@ -640,7 +640,7 @@ namespace Umbraco.Tests.Cache Assert.AreEqual(1, d.Test.LiveGen); Assert.IsTrue(d.Test.NextGen); Assert.IsNull(s1.Get(1)); - }); + } var s2 = d.CreateSnapshot(); @@ -685,7 +685,7 @@ namespace Umbraco.Tests.Cache Assert.IsFalse(d.Test.NextGen); Assert.AreEqual("uno", s2.Get(1)); - d.WriteLocked(() => + using (d.GetWriter(GetScopeProvider())) { // gen 3 Assert.AreEqual(2, d.Test.GetValues(1).Length); @@ -701,7 +701,7 @@ namespace Umbraco.Tests.Cache Assert.AreEqual(3, d.Test.LiveGen); Assert.IsTrue(d.Test.NextGen); // has NOT changed when (non) creating snapshot Assert.AreEqual("uno", s3.Get(1)); - }); + } var s4 = d.CreateSnapshot(); @@ -717,13 +717,14 @@ namespace Umbraco.Tests.Cache var d = new SnapDictionary(); d.Test.CollectAuto = false; - d.WriteLocked(() => + var scopeProvider = GetScopeProvider(); + using (d.GetWriter(scopeProvider)) { - d.WriteLocked(() => + using (d.GetWriter(scopeProvider)) { d.Set(1, "one"); - }); - }); + } + } } [Test] @@ -761,9 +762,7 @@ namespace Umbraco.Tests.Cache Assert.IsFalse(d.Test.NextGen); Assert.AreEqual("uno", s2.Get(1)); - var scopeProviderMock = new Mock(); - scopeProviderMock.Setup(x => x.Context).Returns(null); - var scopeProvider = scopeProviderMock.Object; + var scopeProvider = GetScopeProvider(); using (d.GetWriter(scopeProvider)) { @@ -808,9 +807,7 @@ namespace Umbraco.Tests.Cache Assert.AreEqual(2, s2.Gen); Assert.AreEqual("uno", s2.Get(1)); - var scopeProviderMock = new Mock(); - scopeProviderMock.Setup(x => x.Context).Returns(null); - var scopeProvider = scopeProviderMock.Object; + var scopeProvider = GetScopeProvider(); using (d.GetWriter(scopeProvider)) { @@ -849,10 +846,7 @@ namespace Umbraco.Tests.Cache Assert.AreEqual(2, s2.Gen); Assert.AreEqual("uno", s2.Get(1)); - var scopeProviderMock = new Mock(); - var scopeContext = new ScopeContext(); - scopeProviderMock.Setup(x => x.Context).Returns(scopeContext); - var scopeProvider = scopeProviderMock.Object; + var scopeProvider = GetScopeProvider(true); using (d.GetWriter(scopeProvider)) { @@ -873,7 +867,7 @@ namespace Umbraco.Tests.Cache Assert.AreEqual(2, s4.Gen); Assert.AreEqual("uno", s4.Get(1)); - scopeContext.ScopeExit(true); + ((ScopeContext) scopeProvider.Context).ScopeExit(true); var s5 = d.CreateSnapshot(); Assert.AreEqual(3, s5.Gen); @@ -960,5 +954,14 @@ namespace Umbraco.Tests.Cache Assert.AreEqual("uno", all[0]); Assert.AreEqual("four", all[3]); } + + private IScopeProvider GetScopeProvider(bool withContext = false) + { + var scopeProviderMock = new Mock(); + var scopeContext = withContext ? new ScopeContext() : null; + scopeProviderMock.Setup(x => x.Context).Returns(scopeContext); + var scopeProvider = scopeProviderMock.Object; + return scopeProvider; + } } } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/FacadeService.cs b/src/Umbraco.Web/PublishedCache/NuCache/FacadeService.cs index 2563656c66..8020cf5098 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/FacadeService.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/FacadeService.cs @@ -427,7 +427,7 @@ namespace Umbraco.Web.PublishedCache.NuCache private void LockAndLoadDomains() { - _domainStore.WriteLocked(() => + using (_domainStore.GetWriter(_scopeProvider)) { using (var uow = _uowProvider.CreateUnitOfWork()) { @@ -435,7 +435,7 @@ namespace Umbraco.Web.PublishedCache.NuCache LoadDomainsLocked(); uow.Complete(); } - }); + } } private void LoadDomainsLocked() @@ -758,7 +758,7 @@ namespace Umbraco.Web.PublishedCache.NuCache if (_isReady == false) return; - _domainStore.WriteLocked(() => + using (_domainStore.GetWriter(_scopeProvider)) { foreach (var payload in payloads) { @@ -787,7 +787,7 @@ namespace Umbraco.Web.PublishedCache.NuCache break; } } - }); + } } #endregion diff --git a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs index 3b126acdd2..67fe72d31b 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs @@ -53,6 +53,14 @@ namespace Umbraco.Web.PublishedCache.NuCache #region Locking + // read and write locks are not exclusive + // it is not possible to write-lock while someone is read-locked + // it is possible to read-lock while someone is write-locked + // + // so when getting a read-lock, + // either we are write-locked or not, but if not, we won't be write-locked + // otoh the write-lock may be released in the meantime + // Lock has a 'forceGen' parameter: // used to start a set of changes that may not commit, to isolate the set from any pending // changes that would not have been snapshotted yet, so they cannot be rolled back by accident @@ -67,43 +75,47 @@ namespace Umbraco.Web.PublishedCache.NuCache private readonly string _instanceId = Guid.NewGuid().ToString("N"); - // a scope contextual that represents a locked writer to the dictionary - private class SnapDictionaryWriter : ScopeContextualBase + private class ReadLockInfo { - private SnapDictionary _dictionary; - private WriteLock _wl; - - public SnapDictionaryWriter(SnapDictionary dictionary) - { - _dictionary = dictionary; - _wl = new WriteLock(); - dictionary.Lock(_wl, true); - } - - public override void Release(bool completed) - { - if (_wl == null) return; - _dictionary.Release(_wl, completed); - _wl = null; - _dictionary = null; - } + public bool Taken; } - // gets a scope contextual representing a locked writer to the dictionary - public IDisposable GetWriter(IScopeProvider scopeProvider) - { - return ScopeContextualBase.Get(scopeProvider, _instanceId, () => new SnapDictionaryWriter(this)); - } - - private class WriteLock + private class WriteLockInfo { public bool Taken; public bool Count; } - private void Lock(WriteLock wl, bool forceGen) + // a scope contextual that represents a locked writer to the dictionary + private class SnapDictionaryWriter : ScopeContextualBase { - Monitor.Enter(_wlocko, ref wl.Taken); + private readonly WriteLockInfo _lockinfo = new WriteLockInfo(); + private SnapDictionary _dictionary; + + public SnapDictionaryWriter(SnapDictionary dictionary, bool scoped) + { + _dictionary = dictionary; + dictionary.Lock(_lockinfo, scoped); + } + + public override void Release(bool completed) + { + if (_dictionary == null) return; + _dictionary.Release(_lockinfo, completed); + _dictionary = null; + } + } + + // gets a scope contextual representing a locked writer to the dictionary + // fixme GetScopedWriter? should the dict have a ref onto the scope provider? + public IDisposable GetWriter(IScopeProvider scopeProvider) + { + return ScopeContextualBase.Get(scopeProvider, _instanceId, scoped => new SnapDictionaryWriter(this, scoped)); + } + + private void Lock(WriteLockInfo lockInfo, bool forceGen = false) + { + Monitor.Enter(_wlocko, ref lockInfo.Taken); var rtaken = false; try @@ -116,12 +128,10 @@ namespace Umbraco.Web.PublishedCache.NuCache // http://joeduffyblog.com/2007/02/07/introducing-the-new-readerwriterlockslim-in-orcas/ // http://chabster.blogspot.fr/2013/12/readerwriterlockslim-fails-on-dual.html //RuntimeHelpers.PrepareConstrainedRegions(); - try - { } - finally + try { } finally { _wlocked++; - wl.Count = true; + lockInfo.Count = true; if (_nextGen == false || (forceGen && _wlocked == 1)) // if true already... ok to have "holes" in generation objects { // because we are changing things, a new generation @@ -137,12 +147,29 @@ namespace Umbraco.Web.PublishedCache.NuCache } } - private void Release(WriteLock wl, bool commit = true) + private void Lock(ReadLockInfo lockInfo) + { + Monitor.Enter(_rlocko, ref lockInfo.Taken); + } + + private void Release(WriteLockInfo lockInfo, bool commit = true) { if (commit == false) { - _nextGen = false; - _liveGen -= 1; + var rtaken = false; + try + { + Monitor.Enter(_rlocko, ref rtaken); + try { } finally + { + _nextGen = false; + _liveGen -= 1; + } + } + finally + { + if (rtaken) Monitor.Exit(_rlocko); + } foreach (var item in _items) { @@ -157,56 +184,13 @@ namespace Umbraco.Web.PublishedCache.NuCache } } - if (wl.Count) _wlocked--; - if (wl.Taken) Monitor.Exit(_wlocko); + if (lockInfo.Count) _wlocked--; + if (lockInfo.Taken) Monitor.Exit(_wlocko); } - public void WriteLocked(Action action) + private void Release(ReadLockInfo lockInfo) { - var wl = new WriteLock(); - try - { - // lock (again) - don't force a next gen if there's already one - Lock(wl, false); - action(); - } - finally - { - Release(wl); - } - } - - public T WriteLocked(Func func) - { - var wl = new WriteLock(); - try - { - // lock (again) - don't force a next gen if there's already one - Lock(wl, false); - return func(); - } - finally - { - Release(wl); - } - } - - private T ReadLocked(Func func) - { - var rtaken = false; - try - { - Monitor.Enter(_rlocko, ref rtaken); - - // we have rlock, so it cannot ++ - // it could -- though, so... volatile - var wlocked = _wlocked > 0; - return func(wlocked); - } - finally - { - if (rtaken) Monitor.Exit(_rlocko); - } + if (lockInfo.Taken) Monitor.Exit(_rlocko); } #endregion @@ -223,8 +207,11 @@ namespace Umbraco.Web.PublishedCache.NuCache public void Set(TKey key, TValue value) { - WriteLocked(() => + var lockInfo = new WriteLockInfo(); + try { + Lock(lockInfo); + // this is safe only because we're write-locked var link = GetHead(key); if (link != null) @@ -251,7 +238,11 @@ namespace Umbraco.Web.PublishedCache.NuCache { _items.TryAdd(key, new LinkedNode(value, _liveGen)); } - }); + } + finally + { + Release(lockInfo); + } } public void Clear(TKey key) @@ -261,8 +252,11 @@ namespace Umbraco.Web.PublishedCache.NuCache public void Clear() { - WriteLocked(() => + var lockInfo = new WriteLockInfo(); + try { + Lock(lockInfo); + // this is safe only because we're write-locked foreach (var kvp in _items.Where(x => x.Value != null)) { @@ -276,7 +270,11 @@ namespace Umbraco.Web.PublishedCache.NuCache kvp.Value.Value = null; } } - }); + } + finally + { + Release(lockInfo); + } } public TValue Get(TKey key, long gen) @@ -335,8 +333,11 @@ namespace Umbraco.Web.PublishedCache.NuCache public Snapshot CreateSnapshot() { - return ReadLocked(wlocked => + var lockInfo = new ReadLockInfo(); + try { + Lock(lockInfo); + // if no next generation is required, and we already have one, // use it and create a new snapshot if (_nextGen == false && _generationObject != null) @@ -345,7 +346,7 @@ namespace Umbraco.Web.PublishedCache.NuCache // else we need to try to create a new gen ref // whether we are wlocked or not, noone can rlock while we do, // so _liveGen and _nextGen are safe - if (wlocked) + if (_wlocked > 0) // volatile, cannot ++ but could -- { // write-locked, cannot use latest gen (at least 1) so use previous var snapGen = _nextGen ? _liveGen - 1 : _liveGen; @@ -379,7 +380,11 @@ namespace Umbraco.Web.PublishedCache.NuCache CollectAsyncLocked(); return snapshot; - }); + } + finally + { + Release(lockInfo); + } } public Task CollectAsync()