Fix navigationUrlService and underlying models not being thread safe (#19689)

* Fix navigationUrlService and underlying models not being thread safe

* Added migration to plan.

---------

Co-authored-by: Andy Butland <[email protected]>

Author: Migaroez

src/Umbraco.Core/Collections/ConcurrentHashSet.cs

@@ -9,7 +9,7 @@ namespace Umbraco.Cms.Core.Collections;
 /// </summary>
 /// <typeparam name="T"></typeparam>
 [Serializable]
-public class ConcurrentHashSet<T> : ICollection<T>
+public class ConcurrentHashSet<T> : ICollection<T>, ISet<T>
 {
     private readonly HashSet<T> _innerSet = new();
     private readonly ReaderWriterLockSlim _instanceLocker = new(LockRecursionPolicy.NoRecursion);
@@ -274,4 +274,180 @@ private HashSet<T> GetThreadSafeClone()
 
         return clone;
     }
+
+    public void ExceptWith(IEnumerable<T> other)
+    {
+        try
+        {
+            _instanceLocker.EnterWriteLock();
+            _innerSet.ExceptWith(other);
+        }
+        finally
+        {
+            if (_instanceLocker.IsWriteLockHeld)
+            {
+                _instanceLocker.ExitWriteLock();
+            }
+        }
+    }
+
+    public void IntersectWith(IEnumerable<T> other)
+    {
+        try
+        {
+            _instanceLocker.EnterWriteLock();
+            _innerSet.IntersectWith(other);
+        }
+        finally
+        {
+            if (_instanceLocker.IsWriteLockHeld)
+            {
+                _instanceLocker.ExitWriteLock();
+            }
+        }
+    }
+
+    public bool IsProperSubsetOf(IEnumerable<T> other)
+    {
+        try
+        {
+            _instanceLocker.EnterReadLock();
+            return _innerSet.IsProperSubsetOf(other);
+        }
+        finally
+        {
+            if (_instanceLocker.IsReadLockHeld)
+            {
+                _instanceLocker.ExitReadLock();
+            }
+        }
+    }
+
+    public bool IsProperSupersetOf(IEnumerable<T> other)
+    {
+        try
+        {
+            _instanceLocker.EnterReadLock();
+            return _innerSet.IsProperSupersetOf(other);
+        }
+        finally
+        {
+            if (_instanceLocker.IsReadLockHeld)
+            {
+                _instanceLocker.ExitReadLock();
+            }
+        }
+    }
+
+    public bool IsSubsetOf(IEnumerable<T> other)
+    {
+        try
+        {
+            _instanceLocker.EnterReadLock();
+            return _innerSet.IsSubsetOf(other);
+        }
+        finally
+        {
+            if (_instanceLocker.IsReadLockHeld)
+            {
+                _instanceLocker.ExitReadLock();
+            }
+        }
+    }
+
+    public bool IsSupersetOf(IEnumerable<T> other)
+    {
+        try
+        {
+            _instanceLocker.EnterReadLock();
+            return _innerSet.IsSupersetOf(other);
+        }
+        finally
+        {
+            if (_instanceLocker.IsReadLockHeld)
+            {
+                _instanceLocker.ExitReadLock();
+            }
+        }
+    }
+
+    public bool Overlaps(IEnumerable<T> other)
+    {
+        try
+        {
+            _instanceLocker.EnterReadLock();
+            return _innerSet.Overlaps(other);
+        }
+        finally
+        {
+            if (_instanceLocker.IsReadLockHeld)
+            {
+                _instanceLocker.ExitReadLock();
+            }
+        }
+    }
+
+    public bool SetEquals(IEnumerable<T> other)
+    {
+        try
+        {
+            _instanceLocker.EnterReadLock();
+            return _innerSet.SetEquals(other);
+        }
+        finally
+        {
+            if (_instanceLocker.IsReadLockHeld)
+            {
+                _instanceLocker.ExitReadLock();
+            }
+        }
+    }
+
+    public void SymmetricExceptWith(IEnumerable<T> other)
+    {
+        try
+        {
+            _instanceLocker.EnterWriteLock();
+            _innerSet.IntersectWith(other);
+        }
+        finally
+        {
+            if (_instanceLocker.IsWriteLockHeld)
+            {
+                _instanceLocker.ExitWriteLock();
+            }
+        }
+    }
+
+    public void UnionWith(IEnumerable<T> other)
+    {
+        try
+        {
+            _instanceLocker.EnterWriteLock();
+            _innerSet.UnionWith(other);
+        }
+        finally
+        {
+            if (_instanceLocker.IsWriteLockHeld)
+            {
+                _instanceLocker.ExitWriteLock();
+            }
+        }
+    }
+
+    bool ISet<T>.Add(T item)
+    {
+        try
+        {
+            _instanceLocker.EnterWriteLock();
+            return _innerSet.Add(item);
+        }
+        finally
+        {
+            if (_instanceLocker.IsWriteLockHeld)
+            {
+                _instanceLocker.ExitWriteLock();
+            }
+        }
+    }
 }

src/Umbraco.Core/Models/Navigation/NavigationNode.cs

@@ -1,10 +1,11 @@
 using System.Collections.Concurrent;
+using Umbraco.Cms.Core.Collections;
 
 namespace Umbraco.Cms.Core.Models.Navigation;
 
 public sealed class NavigationNode
 {
-    private HashSet<Guid> _children;
+    private ConcurrentHashSet<Guid> _children;
 
     public Guid Key { get; private set; }
 
@@ -21,7 +22,7 @@ public NavigationNode(Guid key, Guid contentTypeKey, int sortOrder = 0)
         Key = key;
         ContentTypeKey = contentTypeKey;
         SortOrder = sortOrder;
-        _children = new HashSet<Guid>();
+        _children = new ConcurrentHashSet<Guid>();
     }
 
     public void UpdateSortOrder(int newSortOrder) => SortOrder = newSortOrder;

src/Umbraco.Core/Persistence/Constants-Locks.cs

@@ -85,5 +85,10 @@ public static class Locks
         ///     Long-running operations.
         /// </summary>
         public const int LongRunningOperations = -344;
+
+        /// <summary>
+        ///     All document URLs.
+        /// </summary>
+        public const int DocumentUrls = -345;
     }
 }

src/Umbraco.Core/Services/DocumentUrlService.cs

@@ -369,6 +369,7 @@ public async Task CreateOrUpdateUrlSegmentsAsync(IEnumerable<IContent> documents
 
         if (toSave.Count > 0)
         {
+            scope.WriteLock(Constants.Locks.DocumentUrls);
             _documentUrlRepository.Save(toSave);
         }
 

src/Umbraco.Infrastructure/Migrations/Install/DatabaseDataCreator.cs

@@ -1051,6 +1051,7 @@ private void CreateLockData()
         _database.Insert(Constants.DatabaseSchema.Tables.Lock, "id", false, new LockDto { Id = Constants.Locks.WebhookRequest, Name = "WebhookRequest" });
         _database.Insert(Constants.DatabaseSchema.Tables.Lock, "id", false, new LockDto { Id = Constants.Locks.WebhookLogs, Name = "WebhookLogs" });
         _database.Insert(Constants.DatabaseSchema.Tables.Lock, "id", false, new LockDto { Id = Constants.Locks.LongRunningOperations, Name = "LongRunningOperations" });
+        _database.Insert(Constants.DatabaseSchema.Tables.Lock, "id", false, new LockDto { Id = Constants.Locks.DocumentUrls, Name = "DocumentUrls" });
     }
 
     private void CreateContentTypeData()

src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs

@@ -122,5 +122,6 @@ protected virtual void DefinePlan()
 
         // To 16.2.0
         To<V_16_2_0.AddLongRunningOperations>("{741C22CF-5FB8-4343-BF79-B97A58C2CCBA}");
+        To<V_16_2_0.AddDocumentUrlLock>("{BE11D4D3-3A1F-4598-90D4-B548BD188C48}");
     }
 }

src/Umbraco.Infrastructure/Migrations/Upgrade/V_16_2_0/AddDocumentUrlLock.cs

@@ -0,0 +1,30 @@
+using NPoco;
+using Umbraco.Cms.Core;
+using Umbraco.Cms.Infrastructure.Persistence;
+using Umbraco.Cms.Infrastructure.Persistence.Dtos;
+using Umbraco.Extensions;
+
+namespace Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_16_2_0;
+
+[Obsolete("Remove in Umbraco 18.")]
+internal class AddDocumentUrlLock : MigrationBase
+{
+    public AddDocumentUrlLock(IMigrationContext context)
+        : base(context)
+    {
+    }
+
+    protected override void Migrate()
+    {
+        Sql<ISqlContext> sql = Database.SqlContext.Sql()
+            .Select<LockDto>()
+            .From<LockDto>()
+            .Where<LockDto>(x => x.Id == Constants.Locks.DocumentUrls);
+
+        LockDto? existingLockDto = Database.FirstOrDefault<LockDto>(sql);
+        if (existingLockDto is null)
+        {
+            Database.Insert(Constants.DatabaseSchema.Tables.Lock, "id", false, new LockDto { Id = Constants.Locks.DocumentUrls, Name = "DocumentUrls" });
+        }
+    }
+}

src/Umbraco.Infrastructure/Migrations/Upgrade/V_16_2_0/AddLongRunningOperations.cs

@@ -26,8 +26,8 @@ protected override void Migrate()
             .From<LockDto>()
             .Where<LockDto>(x => x.Id == Constants.Locks.LongRunningOperations);
 
-        LockDto? longRunningOperationsLock = Database.FirstOrDefault<LockDto>(sql);
-        if (longRunningOperationsLock is null)
+        LockDto? existingLockDto = Database.FirstOrDefault<LockDto>(sql);
+        if (existingLockDto is null)
         {
             Database.Insert(Constants.DatabaseSchema.Tables.Lock, "id", false, new LockDto { Id = Constants.Locks.LongRunningOperations, Name = "LongRunningOperations" });
         }