diff --git a/src/Umbraco.Core/Scoping/ScopeContextualBase.cs b/src/Umbraco.Core/Scoping/ScopeContextualBase.cs new file mode 100644 index 0000000000..df19d7b2ea --- /dev/null +++ b/src/Umbraco.Core/Scoping/ScopeContextualBase.cs @@ -0,0 +1,40 @@ +using System; + +namespace Umbraco.Core.Scoping +{ + // base class for an object that will be enlisted in scope context, if any. it + // must be used in a 'using' block, and if not scoped, released when disposed, + // else when scope context runs enlisted actions + public abstract class ScopeContextualBase : IDisposable + { + private bool _using, _scoped; + + public static T Get(IScopeProvider scopeProvider, string key, Func ctor) + where T : ScopeContextualBase + { + var scopeContext = scopeProvider.Context; + if (scopeContext == null) + return ctor(); + + var w = scopeContext.Enlist("ScopeContextualBase_" + key, + ctor, + (completed, item) => { item.Release(completed); }); + + if (w._using) throw new InvalidOperationException("panic: used."); + w._using = true; + w._scoped = true; + + return w; + } + + public void Dispose() + { + _using = false; + + if (_scoped == false) + Release(true); + } + + public abstract void Release(bool completed); + } +} \ No newline at end of file diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 282fd422fc..b43f9f3500 100644 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -1256,6 +1256,7 @@ + diff --git a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs index 4364cb9328..a73776badf 100644 --- a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs +++ b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs @@ -1,7 +1,9 @@ using System; using System.Linq; using System.Threading.Tasks; +using Moq; using NUnit.Framework; +using Umbraco.Core.Scoping; using Umbraco.Web.PublishedCache.NuCache; namespace Umbraco.Tests.Cache @@ -709,6 +711,223 @@ namespace Umbraco.Tests.Cache Assert.AreEqual("ein", s4.Get(1)); } + [Test] + public void NestedWriteLocking() + { + var d = new SnapDictionary(); + d.Test.CollectAuto = false; + + d.WriteLocked(() => + { + d.WriteLocked(() => + { + d.Set(1, "one"); + }); + }); + } + + [Test] + public void WriteLocking2() + { + var d = new SnapDictionary(); + d.Test.CollectAuto = false; + + // gen 1 + d.Set(1, "one"); + Assert.AreEqual(1, d.Test.GetValues(1).Length); + + Assert.AreEqual(1, d.Test.LiveGen); + Assert.IsTrue(d.Test.NextGen); + + var s1 = d.CreateSnapshot(); + + Assert.AreEqual(1, s1.Gen); + Assert.AreEqual(1, d.Test.LiveGen); + Assert.IsFalse(d.Test.NextGen); + Assert.AreEqual("one", s1.Get(1)); + + // gen 2 + Assert.AreEqual(1, d.Test.GetValues(1).Length); + d.Set(1, "uno"); + Assert.AreEqual(2, d.Test.GetValues(1).Length); + + Assert.AreEqual(2, d.Test.LiveGen); + Assert.IsTrue(d.Test.NextGen); + + var s2 = d.CreateSnapshot(); + + Assert.AreEqual(2, s2.Gen); + Assert.AreEqual(2, d.Test.LiveGen); + 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; + + using (d.GetWriter(scopeProvider)) + { + // gen 3 + Assert.AreEqual(2, d.Test.GetValues(1).Length); + d.Set(1, "ein"); + Assert.AreEqual(3, d.Test.GetValues(1).Length); + + Assert.AreEqual(3, d.Test.LiveGen); + Assert.IsTrue(d.Test.NextGen); + + var s3 = d.CreateSnapshot(); + + Assert.AreEqual(2, s3.Gen); + 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(); + + Assert.AreEqual(3, s4.Gen); + Assert.AreEqual(3, d.Test.LiveGen); + Assert.IsFalse(d.Test.NextGen); + Assert.AreEqual("ein", s4.Get(1)); + } + + [Test] + public void WriteLocking3() + { + var d = new SnapDictionary(); + d.Test.CollectAuto = false; + + // gen 1 + d.Set(1, "one"); + var s1 = d.CreateSnapshot(); + Assert.AreEqual(1, s1.Gen); + Assert.AreEqual("one", s1.Get(1)); + + d.Set(1, "uno"); + var s2 = d.CreateSnapshot(); + 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; + + using (d.GetWriter(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 + d.Set(1, "ein"); + var s3 = d.CreateSnapshot(); + Assert.AreEqual(2, s3.Gen); + Assert.AreEqual("uno", s3.Get(1)); + + // but live snapshot contains changes + var ls = d.Test.LiveSnapshot; + Assert.AreEqual("ein", ls.Get(1)); + Assert.AreEqual(3, ls.Gen); + } + + var s4 = d.CreateSnapshot(); + Assert.AreEqual(3, s4.Gen); + Assert.AreEqual("ein", s4.Get(1)); + } + + [Test] + public void ScopeLocking1() + { + var d = new SnapDictionary(); + d.Test.CollectAuto = false; + + // gen 1 + d.Set(1, "one"); + var s1 = d.CreateSnapshot(); + Assert.AreEqual(1, s1.Gen); + Assert.AreEqual("one", s1.Get(1)); + + d.Set(1, "uno"); + var s2 = d.CreateSnapshot(); + 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; + + using (d.GetWriter(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 + d.Set(1, "ein"); + var s3 = d.CreateSnapshot(); + Assert.AreEqual(2, s3.Gen); + Assert.AreEqual("uno", s3.Get(1)); + + // but live snapshot contains changes + var ls = d.Test.LiveSnapshot; + Assert.AreEqual("ein", ls.Get(1)); + Assert.AreEqual(3, ls.Gen); + } + + var s4 = d.CreateSnapshot(); + Assert.AreEqual(2, s4.Gen); + Assert.AreEqual("uno", s4.Get(1)); + + scopeContext.ScopeExit(true); + + var s5 = d.CreateSnapshot(); + Assert.AreEqual(3, s5.Gen); + Assert.AreEqual("ein", s5.Get(1)); + } + + [Test] + public void ScopeLocking2() + { + var d = new SnapDictionary(); + d.Test.CollectAuto = false; + + // gen 1 + d.Set(1, "one"); + var s1 = d.CreateSnapshot(); + Assert.AreEqual(1, s1.Gen); + Assert.AreEqual("one", s1.Get(1)); + + d.Set(1, "uno"); + var s2 = d.CreateSnapshot(); + 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; + + using (d.GetWriter(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 + d.Set(1, "ein"); + var s3 = d.CreateSnapshot(); + Assert.AreEqual(2, s3.Gen); + Assert.AreEqual("uno", s3.Get(1)); + + // but live snapshot contains changes + var ls = d.Test.LiveSnapshot; + Assert.AreEqual("ein", ls.Get(1)); + Assert.AreEqual(3, ls.Gen); + } + + var s4 = d.CreateSnapshot(); + Assert.AreEqual(2, s4.Gen); + Assert.AreEqual("uno", s4.Get(1)); + + scopeContext.ScopeExit(false); + + var s5 = d.CreateSnapshot(); + Assert.AreEqual(2, s5.Gen); + Assert.AreEqual("uno", s5.Get(1)); + } + [Test] public void GetAll() { diff --git a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs index 0c4b4a05b1..3b126acdd2 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs @@ -4,6 +4,7 @@ using System.Collections.Generic; using System.Linq; using System.Threading; using System.Threading.Tasks; +using Umbraco.Core.Scoping; namespace Umbraco.Web.PublishedCache.NuCache { @@ -52,93 +53,141 @@ namespace Umbraco.Web.PublishedCache.NuCache #region Locking - public void WriteLocked(Action action) + // 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 + // + // Release has a 'commit' parameter: + // if false, the live gen is scrapped and changes that have been applied as part of the lock + // are all ignored - Release is private and meant to be invoked with 'commit' being false only + // only on the outermost lock (by SnapDictionaryWriter) + + // using (...) {} for locking is prone to nasty leaks in case of weird exceptions + // such as thread-abort or out-of-memory, but let's not worry about it now + + private readonly string _instanceId = Guid.NewGuid().ToString("N"); + + // a scope contextual that represents a locked writer to the dictionary + private class SnapDictionaryWriter : ScopeContextualBase { - var wtaken = false; - var wcount = false; + 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; + } + } + + // 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 + { + public bool Taken; + public bool Count; + } + + private void Lock(WriteLock wl, bool forceGen) + { + Monitor.Enter(_wlocko, ref wl.Taken); + + var rtaken = false; try { - Monitor.Enter(_wlocko, ref wtaken); + Monitor.Enter(_rlocko, ref rtaken); - var rtaken = false; + // assume everything in finally runs atomically + // http://stackoverflow.com/questions/18501678/can-this-unexpected-behavior-of-prepareconstrainedregions-and-thread-abort-be-ex + // http://joeduffyblog.com/2005/03/18/atomicity-and-asynchronous-exception-failures/ + // 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 - { - Monitor.Enter(_rlocko, ref rtaken); - - // assume everything in finally runs atomically - // http://stackoverflow.com/questions/18501678/can-this-unexpected-behavior-of-prepareconstrainedregions-and-thread-abort-be-ex - // http://joeduffyblog.com/2005/03/18/atomicity-and-asynchronous-exception-failures/ - // 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 - { - _wlocked++; - wcount = true; - if (_nextGen == false) - { - // because we are changing things, a new generation - // is created, which will trigger a new snapshot - _nextGen = true; - _liveGen += 1; - } - } - } + { } finally { - if (rtaken) Monitor.Exit(_rlocko); + _wlocked++; + wl.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 + // is created, which will trigger a new snapshot + _nextGen = true; + _liveGen += 1; + } } + } + finally + { + if (rtaken) Monitor.Exit(_rlocko); + } + } + private void Release(WriteLock wl, bool commit = true) + { + if (commit == false) + { + _nextGen = false; + _liveGen -= 1; + + foreach (var item in _items) + { + var link = item.Value; + if (link.Gen <= _liveGen) continue; + + var key = item.Key; + if (link.Next == null) + _items.TryRemove(key, out link); + else + _items.TryUpdate(key, link.Next, link); + } + } + + if (wl.Count) _wlocked--; + if (wl.Taken) Monitor.Exit(_wlocko); + } + + public void WriteLocked(Action action) + { + var wl = new WriteLock(); + try + { + // lock (again) - don't force a next gen if there's already one + Lock(wl, false); action(); } finally { - if (wcount) _wlocked--; - if (wtaken) Monitor.Exit(_wlocko); + Release(wl); } } public T WriteLocked(Func func) { - var wtaken = false; - var wcount = false; + var wl = new WriteLock(); try { - Monitor.Enter(_wlocko, ref wtaken); - - var rtaken = false; - try - { - Monitor.Enter(_rlocko, ref rtaken); - - try - { } - finally - { - _wlocked++; - wcount = true; - if (_nextGen == false) - { - // because we are changing things, a new generation - // is created, which will trigger a new snapshot - _nextGen = true; - _liveGen += 1; - } - } - } - finally - { - if (rtaken) Monitor.Exit(_rlocko); - } - + // lock (again) - don't force a next gen if there's already one + Lock(wl, false); return func(); } finally { - if (wcount) _wlocked--; - if (wtaken) Monitor.Exit(_wlocko); + Release(wl); } } @@ -168,8 +217,7 @@ namespace Umbraco.Web.PublishedCache.NuCache private LinkedNode GetHead(TKey key) { - LinkedNode link; - _items.TryGetValue(key, out link); // else null + _items.TryGetValue(key, out LinkedNode link); // else null return link; } @@ -364,8 +412,7 @@ namespace Umbraco.Web.PublishedCache.NuCache private void Collect() { // see notes in CreateSnapshot - GenerationObject generationObject; - while (_generationObjects.TryPeek(out generationObject) && (generationObject.Count == 0 || generationObject.WeakReference.IsAlive == false)) + while (_generationObjects.TryPeek(out GenerationObject generationObject) && (generationObject.Count == 0 || generationObject.WeakReference.IsAlive == false)) { _generationObjects.TryDequeue(out generationObject); // cannot fail since TryPeek has succeeded _floorGen = generationObject.Gen; @@ -471,10 +518,11 @@ namespace Umbraco.Web.PublishedCache.NuCache public ConcurrentQueue GenerationObjects => _dict._generationObjects; + public Snapshot LiveSnapshot => new Snapshot(_dict, _dict._liveGen); + public GenVal[] GetValues(TKey key) { - LinkedNode link; - _dict._items.TryGetValue(key, out link); // else null + _dict._items.TryGetValue(key, out LinkedNode link); // else null if (link == null) return new GenVal[0]; @@ -538,6 +586,12 @@ namespace Umbraco.Web.PublishedCache.NuCache _generationReference.GenerationObject.Reference(); } + internal Snapshot(SnapDictionary store, long gen) + { + _store = store; + _gen = gen; + } + public TValue Get(TKey key) { if (_gen < 0) @@ -576,7 +630,7 @@ namespace Umbraco.Web.PublishedCache.NuCache { if (_gen < 0) return; _gen = -1; - _generationReference.GenerationObject.Release(); + _generationReference?.GenerationObject.Release(); GC.SuppressFinalize(this); } } diff --git a/src/Umbraco.Web/PublishedCache/XmlPublishedCache/SafeXmlReaderWriter.cs b/src/Umbraco.Web/PublishedCache/XmlPublishedCache/SafeXmlReaderWriter.cs index bf31691cca..bf44eea3fa 100644 --- a/src/Umbraco.Web/PublishedCache/XmlPublishedCache/SafeXmlReaderWriter.cs +++ b/src/Umbraco.Web/PublishedCache/XmlPublishedCache/SafeXmlReaderWriter.cs @@ -5,6 +5,7 @@ using Umbraco.Core.Scoping; namespace Umbraco.Web.PublishedCache.XmlPublishedCache { + // fixme should be a ScopeContextualBase internal class SafeXmlReaderWriter : IDisposable { private readonly bool _scoped;