NuCache+Scope - add scope lock to SnapDictionary

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<T>(IScopeProvider scopeProvider, string key, Func<T> 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);
+    }
+}

src/Umbraco.Core/Umbraco.Core.csproj

@@ -1256,6 +1256,7 @@
     <Compile Include="Scoping\RepositoryCacheMode.cs" />
     <Compile Include="Scoping\Scope.cs" />
     <Compile Include="Scoping\ScopeContext.cs" />
+    <Compile Include="Scoping\ScopeContextualBase.cs" />
     <Compile Include="Scoping\ScopeProvider.cs" />
     <Compile Include="Scoping\ScopeReference.cs" />
     <Compile Include="Security\ActiveDirectoryBackOfficeUserPasswordChecker.cs" />

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<int, string>();
+            d.Test.CollectAuto = false;
+
+            d.WriteLocked(() =>
+            {
+                d.WriteLocked(() =>
+                {
+                    d.Set(1, "one");
+                });
+            });
+        }
+
+        [Test]
+        public void WriteLocking2()
+        {
+            var d = new SnapDictionary<int, string>();
+            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<IScopeProvider>();
+            scopeProviderMock.Setup(x => x.Context).Returns<ScopeContext>(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<int, string>();
+            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<IScopeProvider>();
+            scopeProviderMock.Setup(x => x.Context).Returns<ScopeContext>(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<int, string>();
+            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<IScopeProvider>();
+            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<int, string>();
+            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<IScopeProvider>();
+            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()
         {

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<TKey, TValue> _dictionary;
+            private WriteLock _wl;
+
+            public SnapDictionaryWriter(SnapDictionary<TKey, TValue> 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<T>(Func<T> 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<GenerationObject> 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<TKey, TValue> 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);
             }
         }

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;