Merge branch 'v14/dev' into v14/QA/rendering-content/checkbox-date-tests

build/azure-pipelines.yml

@@ -75,7 +75,7 @@ stages:
       - job: A
         displayName: Build Umbraco CMS
         pool:
-          vmImage: 'ubuntu-latest'
+          vmImage: 'windows-latest'
         steps:
           - checkout: self
             submodules: true
@@ -506,6 +506,10 @@ stages:
             condition: and(succeeded(), eq(variables['Agent.OS'], 'Windows_NT'))
             workingDirectory: $(Agent.BuildDirectory)/app
 
+          # Ensures we have the package wait-on installed
+          - pwsh: npm install wait-on
+            displayName: Install wait-on package
+
           # Wait for application to start responding to requests
           - pwsh: npx wait-on -v --interval 1000 --timeout 120000 $(ASPNETCORE_URLS)
             displayName: Wait for application

src/Umbraco.Core/MonitorLock.cs

@@ -4,6 +4,7 @@ namespace Umbraco.Cms.Core;
 ///     Provides an equivalent to the c# lock statement, to be used in a using block.
 /// </summary>
 /// <remarks>Ie replace <c>lock (o) {...}</c> by <c>using (new MonitorLock(o)) { ... }</c></remarks>
+[Obsolete("Use System.Threading.Lock instead. This will be removed in a future version.")]
 public class MonitorLock : IDisposable
 {
     private readonly bool _entered;

src/Umbraco.PublishedCache.NuCache/ContentStore.cs

@@ -52,9 +52,9 @@ public class ContentStore
     // SnapDictionary has unit tests to ensure it all works correctly
     // For locking information, see SnapDictionary
     private readonly IPublishedSnapshotAccessor _publishedSnapshotAccessor;
-    private readonly object _rlocko = new();
     private readonly IVariationContextAccessor _variationContextAccessor;
-    private readonly object _wlocko = new();
+    private readonly object _rlocko = new();
+    private readonly SemaphoreSlim _writeLock = new(1);
     private Task? _collectTask;
     private GenObj? _genObj;
     private long _liveGen;
@@ -319,22 +319,24 @@ public override void Release(bool completed)
 
     private void EnsureLocked()
     {
-        if (!Monitor.IsEntered(_wlocko))
+        if (_writeLock.CurrentCount != 0)
         {
             throw new InvalidOperationException("Write lock must be acquried.");
         }
     }
 
     private void Lock(WriteLockInfo lockInfo, bool forceGen = false)
     {
-        if (Monitor.IsEntered(_wlocko))
+        if (_writeLock.CurrentCount == 0)
         {
             throw new InvalidOperationException("Recursive locks not allowed");
         }
 
-        Monitor.TryEnter(_wlocko, _monitorTimeout, ref lockInfo.Taken);
-
-        if (Monitor.IsEntered(_wlocko) is false)
+        if (_writeLock.Wait(_monitorTimeout))
+        {
+            lockInfo.Taken = true;
+        }
+        else
         {
             throw new TimeoutException("Could not enter monitor before timeout in content store");
         }
@@ -344,6 +346,7 @@ private void Lock(WriteLockInfo lockInfo, bool forceGen = false)
             // see SnapDictionary
             try
             {
+                // Run all code in finally to ensure ThreadAbortException does not interrupt execution
             }
             finally
             {
@@ -374,6 +377,7 @@ private void Release(WriteLockInfo lockInfo, bool commit = true)
                     // see SnapDictionary
                     try
                     {
+                        // Run all code in finally to ensure ThreadAbortException does not interrupt execution
                     }
                     finally
                     {
@@ -409,7 +413,7 @@ private void Release(WriteLockInfo lockInfo, bool commit = true)
         {
             if (lockInfo.Taken)
             {
-                Monitor.Exit(_wlocko);
+                _writeLock.Release();
             }
         }
     }
@@ -1810,7 +1814,7 @@ public Snapshot CreateSnapshot()
             // 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 (Monitor.IsEntered(_wlocko))
+            if (_writeLock.CurrentCount == 0)
             {
                 // write-locked, cannot use latest gen (at least 1) so use previous
                 var snapGen = _nextGen ? _liveGen - 1 : _liveGen;
@@ -1822,8 +1826,7 @@ public Snapshot CreateSnapshot()
                 }
                 else if (_genObj.Gen != snapGen)
                 {
-                    throw new PanicException(
-                        $"The generation {_genObj.Gen} does not equal the snapshot generation {snapGen}");
+                    throw new PanicException($"The generation {_genObj.Gen} does not equal the snapshot generation {snapGen}");
                 }
             }
             else

src/Umbraco.PublishedCache.NuCache/DependencyInjection/UmbracoBuilderExtensions.cs

@@ -37,23 +37,7 @@ public static IUmbracoBuilder AddNuCache(this IUmbracoBuilder builder)
         builder.Services.TryAddSingleton<IPublishedSnapshotStatus, PublishedSnapshotStatus>();
         builder.Services.TryAddTransient<IReservedFieldNamesService, ReservedFieldNamesService>();
 
-        // replace this service since we want to improve the content/media
-        // mapping lookups if we are using nucache.
-        // TODO: Gotta wonder how much this does actually improve perf? It's a lot of weird code to make this happen so hope it's worth it
-        builder.Services.AddUnique<IIdKeyMap>(factory =>
-        {
-            var idkSvc = new IdKeyMap(
-                factory.GetRequiredService<ICoreScopeProvider>(),
-                factory.GetRequiredService<IIdKeyMapRepository>());
-            if (factory.GetRequiredService<IPublishedSnapshotService>() is PublishedSnapshotService
-                publishedSnapshotService)
-            {
-                idkSvc.SetMapper(UmbracoObjectTypes.Document, id => publishedSnapshotService.GetDocumentUid(id), uid => publishedSnapshotService.GetDocumentId(uid));
-                idkSvc.SetMapper(UmbracoObjectTypes.Media, id => publishedSnapshotService.GetMediaUid(id), uid => publishedSnapshotService.GetMediaId(uid));
-            }
-
-            return idkSvc;
-        });
+        builder.Services.AddUnique<IIdKeyMap, IdKeyMap>();
 
         builder.AddNuCacheNotifications();
 

src/Umbraco.PublishedCache.NuCache/SnapDictionary.cs

@@ -28,13 +28,9 @@ public class SnapDictionary<TKey, TValue>
     // This class is optimized for many readers, few writers
     // Readers are lock-free
 
-    // NOTE - we used to lock _rlocko the long hand way with Monitor.Enter(_rlocko, ref lockTaken) but this has
-    // been replaced with a normal c# lock because that's exactly how the normal c# lock works,
-    // see https://blogs.msdn.microsoft.com/ericlippert/2009/03/06/locks-and-exceptions-do-not-mix/
-    // for the readlock, there's no reason here to use the long hand way.
     private readonly ConcurrentDictionary<TKey, LinkedNode<TValue>> _items;
     private readonly object _rlocko = new();
-    private readonly object _wlocko = new();
+    private readonly SemaphoreSlim _writeLock = new(1);
     private Task? _collectTask;
     private GenObj? _genObj;
     private long _liveGen;
@@ -187,22 +183,24 @@ public ScopedWriteLock(SnapDictionary<TKey, TValue> dictionary, bool scoped)
 
     private void EnsureLocked()
     {
-        if (!Monitor.IsEntered(_wlocko))
+        if (_writeLock.CurrentCount != 0)
         {
             throw new InvalidOperationException("Write lock must be acquried.");
         }
     }
 
     private void Lock(WriteLockInfo lockInfo, bool forceGen = false)
     {
-        if (Monitor.IsEntered(_wlocko))
+        if (_writeLock.CurrentCount == 0)
         {
             throw new InvalidOperationException("Recursive locks not allowed");
         }
 
-        Monitor.TryEnter(_wlocko, _monitorTimeout, ref lockInfo.Taken);
-
-        if (Monitor.IsEntered(_wlocko) is false)
+        if (_writeLock.Wait(_monitorTimeout))
+        {
+            lockInfo.Taken = true;
+        }
+        else
         {
             throw new TimeoutException("Could not enter the monitor before timeout in SnapDictionary");
         }
@@ -217,6 +215,7 @@ private void Lock(WriteLockInfo lockInfo, bool forceGen = false)
             // RuntimeHelpers.PrepareConstrainedRegions();
             try
             {
+                // Run all code in finally to ensure ThreadAbortException does not interrupt execution
             }
             finally
             {
@@ -244,43 +243,48 @@ private void Release(WriteLockInfo lockInfo, bool commit = true)
             return;
         }
 
-        if (commit == false)
+        try
         {
-            lock (_rlocko)
+            if (commit == false)
             {
-                try
-                {
-                }
-                finally
+                lock (_rlocko)
                 {
-                    // forget about the temp. liveGen
-                    _nextGen = false;
-                    _liveGen -= 1;
+                    try
+                    {
+                        // Run all code in finally to ensure ThreadAbortException does not interrupt execution
+                    }
+                    finally
+                    {
+                        // forget about the temp. liveGen
+                        _nextGen = false;
+                        _liveGen -= 1;
+                    }
                 }
-            }
 
-            foreach (KeyValuePair<TKey, LinkedNode<TValue>> item in _items)
-            {
-                LinkedNode<TValue>? link = item.Value;
-                if (link.Gen <= _liveGen)
+                foreach (KeyValuePair<TKey, LinkedNode<TValue>> item in _items)
                 {
-                    continue;
-                }
+                    LinkedNode<TValue>? link = item.Value;
+                    if (link.Gen <= _liveGen)
+                    {
+                        continue;
+                    }
 
-                TKey key = item.Key;
-                if (link.Next == null)
-                {
-                    _items.TryRemove(key, out link);
-                }
-                else
-                {
-                    _items.TryUpdate(key, link.Next, link);
+                    TKey key = item.Key;
+                    if (link.Next == null)
+                    {
+                        _items.TryRemove(key, out link);
+                    }
+                    else
+                    {
+                        _items.TryUpdate(key, link.Next, link);
+                    }
                 }
             }
         }
-
-        // TODO: Shouldn't this be in a finally block?
-        Monitor.Exit(_wlocko);
+        finally
+        {
+            _writeLock.Release();
+        }
     }
 
     #endregion
@@ -434,7 +438,7 @@ public Snapshot CreateSnapshot()
             // else we need to try to create a new gen object
             // whether we are wlocked or not, noone can rlock while we do,
             // so _liveGen and _nextGen are safe
-            if (Monitor.IsEntered(_wlocko))
+            if (_writeLock.CurrentCount == 0)
             {
                 // write-locked, cannot use latest gen (at least 1) so use previous
                 var snapGen = _nextGen ? _liveGen - 1 : _liveGen;
@@ -624,7 +628,7 @@ internal class TestHelper
 
         public bool NextGen => _dict._nextGen;
 
-        public bool IsLocked => Monitor.IsEntered(_dict._wlocko);
+        public bool IsLocked => _dict._writeLock.CurrentCount == 0;
 
         public bool CollectAuto
         {

tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/SyntaxProvider/SqlServerSyntaxProviderTests.cs

@@ -68,12 +68,8 @@ string c(string x)
         }
 
         Assert.AreEqual(
-            @$"DELETE FROM {t("cmsContentNu")} WHERE {c("nodeId")} IN (SELECT {c("nodeId")} FROM (SELECT DISTINCT cmsContentNu.nodeId
-FROM {t("cmsContentNu")}
-INNER JOIN {t("umbracoNode")}
-ON {t("cmsContentNu")}.{c("nodeId")} = {t("umbracoNode")}.{c("id")}
-WHERE (({t("umbracoNode")}.{c("nodeObjectType")} = @0))) x)".Replace(Environment.NewLine, " ").Replace("\n", " ")
-                .Replace("\r", " "),
+            @$"DELETE FROM {t("cmsContentNu")} WHERE {c("nodeId")} IN (SELECT {c("nodeId")} FROM (SELECT DISTINCT cmsContentNu.nodeId FROM {t("cmsContentNu")} INNER JOIN {t("umbracoNode")} ON {t("cmsContentNu")}.{c("nodeId")} = {t("umbracoNode")}.{c("id")} WHERE (({t("umbracoNode")}.{c("nodeObjectType")} = @0))) x)".Replace(Environment.NewLine, " ")
+                .Replace("\n", " ").Replace("\r", " "),
             sqlOutput.SQL.Replace(Environment.NewLine, " ").Replace("\n", " ").Replace("\r", " "));
 
         Assert.AreEqual(1, sqlOutput.Arguments.Length);

tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/PublishedCache/ContentCacheTests.cs

@@ -0,0 +1,77 @@
+using Microsoft.Extensions.Logging;
+using NUnit.Framework;
+using Umbraco.Cms.Core.Configuration.Models;
+using Umbraco.Cms.Core.Hosting;
+using Umbraco.Cms.Core.Models.PublishedContent;
+using Umbraco.Cms.Core.PublishedCache;
+using Umbraco.Cms.Infrastructure.PublishedCache;
+using Umbraco.Cms.Infrastructure.PublishedCache.DataSource;
+using Umbraco.Cms.Tests.Common.Builders;
+using Umbraco.Cms.Tests.Common.Builders.Extensions;
+using Umbraco.Cms.Tests.Common.Testing;
+using Umbraco.Cms.Tests.Integration.Testing;
+
+namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.PublishedCache;
+
+[TestFixture]
+[UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)]
+public class ContentCacheTests : UmbracoIntegrationTestWithContent
+{
+    private ContentStore GetContentStore()
+    {
+        var path = Path.Combine(GetRequiredService<IHostingEnvironment>().LocalTempPath, "NuCache");
+        Directory.CreateDirectory(path);
+
+        var localContentDbPath = Path.Combine(path, "NuCache.Content.db");
+        var localContentDbExists = File.Exists(localContentDbPath);
+        var contentDataSerializer = new ContentDataSerializer(new DictionaryOfPropertyDataSerializer());
+        var localContentDb = BTree.GetTree(localContentDbPath, localContentDbExists, new NuCacheSettings(), contentDataSerializer);
+
+        return new ContentStore(
+            GetRequiredService<IPublishedSnapshotAccessor>(),
+            GetRequiredService<IVariationContextAccessor>(),
+            LoggerFactory.CreateLogger<ContentCacheTests>(),
+            LoggerFactory,
+            GetRequiredService<IPublishedModelFactory>(), // new NoopPublishedModelFactory
+            localContentDb);
+    }
+
+    private ContentNodeKit CreateContentNodeKit()
+    {
+        var contentData = new ContentDataBuilder()
+            .WithName("Content 1")
+            .WithProperties(new PropertyDataBuilder()
+                .WithPropertyData("welcomeText", "Welcome")
+                .WithPropertyData("welcomeText", "Welcome", "en-US")
+                .WithPropertyData("welcomeText", "Willkommen", "de")
+                .WithPropertyData("welcomeText", "Welkom", "nl")
+                .WithPropertyData("welcomeText2", "Welcome")
+                .WithPropertyData("welcomeText2", "Welcome", "en-US")
+                .WithPropertyData("noprop", "xxx")
+                .Build())
+            .Build();
+
+        return ContentNodeKitBuilder.CreateWithContent(
+            ContentType.Id,
+            1,
+            "-1,1",
+            draftData: contentData,
+            publishedData: contentData);
+    }
+
+    [Test]
+    public async Task SetLocked()
+    {
+        var contentStore = GetContentStore();
+
+        using (contentStore.GetScopedWriteLock(ScopeProvider))
+        {
+            var contentNodeKit = CreateContentNodeKit();
+
+            contentStore.SetLocked(contentNodeKit);
+
+            // Try running the same operation again in an async task
+            await Task.Run(() => contentStore.SetLocked(contentNodeKit));
+        }
+    }
+}