Long running operations causes SQL timeout

src/Umbraco.Core/Persistence/SqlSyntax/ISqlSyntaxProvider.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Data;
 using System.Text.RegularExpressions;
 using NPoco;
 using Umbraco.Core.Persistence.DatabaseAnnotations;
@@ -76,6 +77,11 @@ public interface ISqlSyntaxProvider
         string ConvertIntegerToOrderableString { get; }
         string ConvertDateToOrderableString { get; }
         string ConvertDecimalToOrderableString { get; }
+
+        /// <summary>
+        /// Returns the default isolation level for the database
+        /// </summary>
+        IsolationLevel DefaultIsolationLevel { get; }
 
         IEnumerable<string> GetTablesInSchema(IDatabase db);
         IEnumerable<ColumnInfo> GetColumnsInSchema(IDatabase db);
@@ -121,5 +127,8 @@ public interface ISqlSyntaxProvider
         /// unspecified.</para>
         /// </remarks>
         bool TryGetDefaultConstraint(IDatabase db, string tableName, string columnName, out string constraintName);
+
+        void ReadLock(IDatabase db, params int[] lockIds);
+        void WriteLock(IDatabase db, params int[] lockIds);
     }
 }

src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs

@@ -1,5 +1,7 @@
 using System;
 using System.Collections.Generic;
+using System.Data;
+using System.Data.SqlServerCe;
 using System.Linq;
 using NPoco;
 using Umbraco.Core.Persistence.DatabaseAnnotations;
@@ -52,6 +54,8 @@ public override string GetConcat(params string[] args)
             return "(" + string.Join("+", args) + ")";
         }
 
+        public override System.Data.IsolationLevel DefaultIsolationLevel => System.Data.IsolationLevel.RepeatableRead;
+
         public override string FormatColumnRename(string tableName, string oldName, string newName)
         {
             //NOTE Sql CE doesn't support renaming a column, so a new column needs to be created, then copy data and finally remove old column
@@ -152,6 +156,39 @@ public override bool DoesTableExist(IDatabase db, string tableName)
             return result > 0;
         }
 
+        public override void WriteLock(IDatabase db, params int[] lockIds)
+        {
+            // soon as we get Database, a transaction is started
+
+            if (db.Transaction.IsolationLevel < IsolationLevel.RepeatableRead)
+                throw new InvalidOperationException("A transaction with minimum RepeatableRead isolation level is required.");
+
+            db.Execute(@"SET LOCK_TIMEOUT 1800;");
+            // *not* using a unique 'WHERE IN' query here because the *order* of lockIds is important to avoid deadlocks
+            foreach (var lockId in lockIds)
+            {
+                var i = db.Execute(@"UPDATE umbracoLock SET value = value*-1 WHERE id=@id", new { id = lockId });
+                if (i == 0) // ensure we are actually locking!
+                    throw new ArgumentException($"LockObject with id={lockId} does not exist.");
+            }
+        }
+
+        public override void ReadLock(IDatabase db, params int[] lockIds)
+        {
+            // soon as we get Database, a transaction is started
+
+            if (db.Transaction.IsolationLevel < IsolationLevel.RepeatableRead)
+                throw new InvalidOperationException("A transaction with minimum RepeatableRead isolation level is required.");
+
+            // *not* using a unique 'WHERE IN' query here because the *order* of lockIds is important to avoid deadlocks
+            foreach (var lockId in lockIds)
+            {
+                var i = db.ExecuteScalar<int?>("SELECT value FROM umbracoLock WHERE id=@id", new { id = lockId });
+                if (i == null) // ensure we are actually locking!
+                    throw new ArgumentException($"LockObject with id={lockId} does not exist.");
+            }
+        }
+
         protected override string FormatIdentity(ColumnDefinition column)
         {
             return column.IsIdentity ? GetIdentityString(column) : string.Empty;

src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs

@@ -2,6 +2,7 @@
 using System.Collections.Generic;
 using System.Data;
 using System.Data.Common;
+using System.Data.SqlClient;
 using System.Linq;
 using NPoco;
 using Umbraco.Core.Logging;
@@ -179,6 +180,8 @@ public override IEnumerable<string> GetTablesInSchema(IDatabase db)
             return items.Select(x => x.TABLE_NAME).Cast<string>().ToList();
         }
 
+        public override IsolationLevel DefaultIsolationLevel => IsolationLevel.ReadCommitted;
+
         public override IEnumerable<ColumnInfo> GetColumnsInSchema(IDatabase db)
         {
             var items = db.Fetch<dynamic>("SELECT TABLE_NAME, COLUMN_NAME, ORDINAL_POSITION, COLUMN_DEFAULT, IS_NULLABLE, DATA_TYPE FROM INFORMATION_SCHEMA.COLUMNS WHERE TABLE_SCHEMA = (SELECT SCHEMA_NAME())");
@@ -246,6 +249,41 @@ public override bool DoesTableExist(IDatabase db, string tableName)
             return result > 0;
         }
 
+        public override void WriteLock(IDatabase db, params int[] lockIds)
+        {
+            // soon as we get Database, a transaction is started
+
+            if (db.Transaction.IsolationLevel < IsolationLevel.ReadCommitted)
+                throw new InvalidOperationException("A transaction with minimum ReadCommitted isolation level is required.");
+
+
+            // *not* using a unique 'WHERE IN' query here because the *order* of lockIds is important to avoid deadlocks
+            foreach (var lockId in lockIds)
+            {
+                db.Execute(@"SET LOCK_TIMEOUT 1800;");
+                var i = db.Execute(@"UPDATE umbracoLock WITH (REPEATABLEREAD) SET value = value*-1 WHERE id=@id", new { id = lockId });
+                if (i == 0) // ensure we are actually locking!
+                    throw new ArgumentException($"LockObject with id={lockId} does not exist.");
+            }
+        }
+
+
+        public override void ReadLock(IDatabase db, params int[] lockIds)
+        {
+            // soon as we get Database, a transaction is started
+
+            if (db.Transaction.IsolationLevel < IsolationLevel.ReadCommitted)
+                throw new InvalidOperationException("A transaction with minimum ReadCommitted isolation level is required.");
+
+            // *not* using a unique 'WHERE IN' query here because the *order* of lockIds is important to avoid deadlocks
+            foreach (var lockId in lockIds)
+            {
+                var i = db.ExecuteScalar<int?>("SELECT value FROM umbracoLock WITH (REPEATABLEREAD) WHERE id=@id", new { id = lockId });
+                if (i == null) // ensure we are actually locking!
+                    throw new ArgumentException($"LockObject with id={lockId} does not exist.", nameof(lockIds));
+            }
+        }
+
         public override string FormatColumnRename(string tableName, string oldName, string newName)
         {
             return string.Format(RenameColumn, tableName, oldName, newName);

src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs

@@ -200,7 +200,9 @@ public virtual string GetSpecialDbType(SpecialDbTypes dbTypes)
 
             return "NVARCHAR";
         }
-
+
+        public abstract IsolationLevel DefaultIsolationLevel { get; }
+
         public virtual IEnumerable<string> GetTablesInSchema(IDatabase db)
         {
             return new List<string>();
@@ -225,6 +227,9 @@ public virtual IEnumerable<Tuple<string, string, string>> GetConstraintsPerColum
 
         public abstract bool TryGetDefaultConstraint(IDatabase db, string tableName, string columnName, out string constraintName);
 
+        public abstract void ReadLock(IDatabase db, params int[] lockIds);
+        public abstract void WriteLock(IDatabase db, params int[] lockIds);
+
         public virtual bool DoesTableExist(IDatabase db, string tableName)
         {
             return false;

src/Umbraco.Core/Persistence/UmbracoDatabase.cs

@@ -20,9 +20,6 @@ namespace Umbraco.Core.Persistence
     /// </remarks>
     public class UmbracoDatabase : Database, IUmbracoDatabase
     {
-        // Umbraco's default isolation level is RepeatableRead
-        private const IsolationLevel DefaultIsolationLevel = IsolationLevel.RepeatableRead;
-
         private readonly ILogger _logger;
         private readonly RetryPolicy _connectionRetryPolicy;
         private readonly RetryPolicy _commandRetryPolicy;
@@ -38,7 +35,7 @@ public class UmbracoDatabase : Database, IUmbracoDatabase
         /// <para>Also used by DatabaseBuilder for creating databases and installing/upgrading.</para>
         /// </remarks>
         public UmbracoDatabase(string connectionString, ISqlContext sqlContext, DbProviderFactory provider, ILogger logger, RetryPolicy connectionRetryPolicy = null, RetryPolicy commandRetryPolicy = null)
-            : base(connectionString, sqlContext.DatabaseType, provider, DefaultIsolationLevel)
+            : base(connectionString, sqlContext.DatabaseType, provider, sqlContext.SqlSyntax.DefaultIsolationLevel)
         {
             SqlContext = sqlContext;
 
@@ -54,7 +51,7 @@ public UmbracoDatabase(string connectionString, ISqlContext sqlContext, DbProvid
         /// </summary>
         /// <remarks>Internal for unit tests only.</remarks>
         internal UmbracoDatabase(DbConnection connection, ISqlContext sqlContext, ILogger logger)
-            : base(connection, sqlContext.DatabaseType, DefaultIsolationLevel)
+            : base(connection, sqlContext.DatabaseType, sqlContext.SqlSyntax.DefaultIsolationLevel)
         {
             SqlContext = sqlContext;
             _logger = logger;

src/Umbraco.Core/Scoping/Scope.cs

@@ -33,8 +33,6 @@ internal class Scope : IScope
         private ICompletable _fscope;
         private IEventDispatcher _eventDispatcher;
 
-        private const IsolationLevel DefaultIsolationLevel = IsolationLevel.RepeatableRead;
-
         // initializes a new scope
         private Scope(ScopeProvider scopeProvider,
             ILogger logger, FileSystems fileSystems, Scope parent, ScopeContext scopeContext, bool detachable,
@@ -205,7 +203,7 @@ public IsolationLevel IsolationLevel
             {
                 if (_isolationLevel != IsolationLevel.Unspecified) return _isolationLevel;
                 if (ParentScope != null) return ParentScope.IsolationLevel;
-                return DefaultIsolationLevel;
+                return Database.SqlContext.SqlSyntax.DefaultIsolationLevel;
             }
         }
 
@@ -488,37 +486,9 @@ private static void TryFinally(int index, Action[] actions)
             ?? (_logUncompletedScopes = Current.Configs.CoreDebug().LogUncompletedScopes)).Value;
 
         /// <inheritdoc />
-        public void ReadLock(params int[] lockIds)
-        {
-            // soon as we get Database, a transaction is started
-
-            if (Database.Transaction.IsolationLevel < IsolationLevel.RepeatableRead)
-                throw new InvalidOperationException("A transaction with minimum RepeatableRead isolation level is required.");
-
-            // *not* using a unique 'WHERE IN' query here because the *order* of lockIds is important to avoid deadlocks
-            foreach (var lockId in lockIds)
-            {
-                var i = Database.ExecuteScalar<int?>("SELECT value FROM umbracoLock WHERE id=@id", new { id = lockId });
-                if (i == null) // ensure we are actually locking!
-                    throw new Exception($"LockObject with id={lockId} does not exist.");
-            }
-        }
+        public void ReadLock(params int[] lockIds) => Database.SqlContext.SqlSyntax.ReadLock(Database, lockIds);
 
         /// <inheritdoc />
-        public void WriteLock(params int[] lockIds)
-        {
-            // soon as we get Database, a transaction is started
-
-            if (Database.Transaction.IsolationLevel < IsolationLevel.RepeatableRead)
-                throw new InvalidOperationException("A transaction with minimum RepeatableRead isolation level is required.");
-
-            // *not* using a unique 'WHERE IN' query here because the *order* of lockIds is important to avoid deadlocks
-            foreach (var lockId in lockIds)
-            {
-                var i = Database.Execute("UPDATE umbracoLock SET value = (CASE WHEN (value=1) THEN -1 ELSE 1 END) WHERE id=@id", new { id = lockId });
-                if (i == 0) // ensure we are actually locking!
-                    throw new Exception($"LockObject with id={lockId} does not exist.");
-            }
-        }
+        public void WriteLock(params int[] lockIds) => Database.SqlContext.SqlSyntax.WriteLock(Database, lockIds);
     }
 }

src/Umbraco.Core/Umbraco.Core.csproj

@@ -982,7 +982,6 @@
     <Compile Include="Persistence\DatabaseModelDefinitions\ModificationType.cs" />
     <Compile Include="Persistence\DatabaseModelDefinitions\SystemMethods.cs" />
     <Compile Include="Persistence\DatabaseModelDefinitions\TableDefinition.cs" />
-    <Compile Include="Persistence\DatabaseNodeLockExtensions.cs" />
     <Compile Include="Persistence\DbCommandExtensions.cs" />
     <Compile Include="Persistence\DbConnectionExtensions.cs" />
     <Compile Include="Persistence\EntityNotFoundException.cs" />

src/Umbraco.Tests/Models/UserExtensionsTests.cs

@@ -57,6 +57,9 @@ public void Determines_Path_Based_Access_To_Content(int startNodeId, string star
         [TestCase("1,-1", "1", "1")] // was an issue
         [TestCase("-1,1", "1", "1")] // was an issue
 
+        [TestCase("-1", "", "-1")]
+        [TestCase("", "-1", "-1")]
+
         public void CombineStartNodes(string groupSn, string userSn, string expected)
         {
             // 1

src/Umbraco.Tests/Persistence/LocksTests.cs

@@ -5,7 +5,6 @@
 using NPoco;
 using NUnit.Framework;
 using Umbraco.Core;
-using Umbraco.Core.Persistence;
 using Umbraco.Core.Persistence.Dtos;
 using Umbraco.Tests.TestHelpers;
 using Umbraco.Tests.Testing;
@@ -37,7 +36,7 @@ public void SingleReadLockTest()
         {
             using (var scope = ScopeProvider.CreateScope())
             {
-                scope.Database.AcquireLockNodeReadLock(Constants.Locks.Servers);
+                scope.WriteLock(Constants.Locks.Servers);
                 scope.Complete();
             }
         }
@@ -62,7 +61,7 @@ public void ConcurrentReadersTest()
                     {
                         try
                         {
-                            scope.Database.AcquireLockNodeReadLock(Constants.Locks.Servers);
+                            scope.ReadLock(Constants.Locks.Servers);
                             lock (locker)
                             {
                                 acquired++;
@@ -131,7 +130,7 @@ public void ConcurrentWritersTest()
                                 if (entered == threadCount) m1.Set();
                             }
                             ms[ic].WaitOne();
-                            scope.Database.AcquireLockNodeWriteLock(Constants.Locks.Servers);
+                            scope.WriteLock(Constants.Locks.Servers);
                             lock (locker)
                             {
                                 acquired++;
@@ -221,7 +220,7 @@ private void DeadLockTestThread(int id1, int id2, EventWaitHandle myEv, WaitHand
                 {
                     otherEv.WaitOne();
                     Console.WriteLine($"[{id1}] WAIT {id1}");
-                    scope.Database.AcquireLockNodeWriteLock(id1);
+                    scope.WriteLock(id1);
                     Console.WriteLine($"[{id1}] GRANT {id1}");
                     WriteLocks(scope.Database);
                     myEv.Set();
@@ -232,7 +231,7 @@ private void DeadLockTestThread(int id1, int id2, EventWaitHandle myEv, WaitHand
                         Thread.Sleep(200); // cannot wait due to deadlock... just give it a bit of time
 
                     Console.WriteLine($"[{id1}] WAIT {id2}");
-                    scope.Database.AcquireLockNodeWriteLock(id2);
+                    scope.WriteLock(id2);
                     Console.WriteLine($"[{id1}] GRANT {id2}");
                     WriteLocks(scope.Database);
                 }
@@ -284,7 +283,7 @@ private void NoDeadLockTestThread(int id, EventWaitHandle myEv, WaitHandle other
                 {
                     otherEv.WaitOne();
                     Console.WriteLine($"[{id}] WAIT {id}");
-                    scope.Database.AcquireLockNodeWriteLock(id);
+                    scope.WriteLock(id);
                     Console.WriteLine($"[{id}] GRANT {id}");
                     WriteLocks(scope.Database);
                     myEv.Set();