From 2c54adacf23852453f6f0944134c85e8730d7364 Mon Sep 17 00:00:00 2001 From: Sebastiaan Janssen Date: Sat, 20 Feb 2021 13:44:15 +0100 Subject: [PATCH] Add the ability to change the SQL Write Lock TimeOut (#9750) Co-authored-by: Shannon --- .../Configuration/GlobalSettings.cs | 30 +++- src/Umbraco.Core/Constants-AppSettings.cs | 3 - .../SqlSyntax/ISqlSyntaxProvider.cs | 8 +- .../SqlSyntax/SqlCeSyntaxProvider.cs | 55 +++++-- .../SqlSyntax/SqlServerSyntaxProvider.cs | 62 ++++++-- .../SqlSyntax/SqlSyntaxProviderBase.cs | 5 +- src/Umbraco.Core/Scoping/IScope.cs | 18 +++ src/Umbraco.Core/Scoping/Scope.cs | 25 ++- src/Umbraco.Tests/Persistence/LocksTests.cs | 149 ++++++++++++++++++ .../TestHelpers/SettingsForTests.cs | 2 +- 10 files changed, 317 insertions(+), 40 deletions(-) diff --git a/src/Umbraco.Core/Configuration/GlobalSettings.cs b/src/Umbraco.Core/Configuration/GlobalSettings.cs index 1d1ccaf7b4..b7dce21285 100644 --- a/src/Umbraco.Core/Configuration/GlobalSettings.cs +++ b/src/Umbraco.Core/Configuration/GlobalSettings.cs @@ -6,7 +6,9 @@ using System.Web; using System.Web.Configuration; using System.Web.Hosting; using System.Xml.Linq; +using Umbraco.Core.Composing; using Umbraco.Core.IO; +using Umbraco.Core.Logging; namespace Umbraco.Core.Configuration { @@ -23,6 +25,7 @@ namespace Umbraco.Core.Configuration // TODO these should not be static private static string _reservedPaths; private static string _reservedUrls; + private static int _sqlWriteLockTimeOut; //ensure the built on (non-changeable) reserved paths are there at all times internal const string StaticReservedPaths = "~/app_plugins/,~/install/,~/mini-profiler-resources/,"; //must end with a comma! @@ -397,21 +400,34 @@ namespace Umbraco.Core.Configuration /// An int value representing the time in milliseconds to lock the database for a write operation /// /// - /// The default value is 1800 milliseconds + /// The default value is 5000 milliseconds /// /// The timeout in milliseconds. public int SqlWriteLockTimeOut { get { - try + if (_sqlWriteLockTimeOut != default) return _sqlWriteLockTimeOut; + + var timeOut = 5000; // 5 seconds + var appSettingSqlWriteLockTimeOut = ConfigurationManager.AppSettings[Constants.AppSettings.SqlWriteLockTimeOut]; + if(int.TryParse(appSettingSqlWriteLockTimeOut, out var configuredTimeOut)) { - return int.Parse(ConfigurationManager.AppSettings[Constants.AppSettings.SqlWriteLockTimeOut]); - } - catch - { - return 1800; + // Only apply this setting if it's not excessively high or low + const int minimumTimeOut = 100; + const int maximumTimeOut = 20000; + if (configuredTimeOut >= minimumTimeOut && configuredTimeOut <= maximumTimeOut) // between 0.1 and 20 seconds + { + timeOut = configuredTimeOut; + } + else + { + Current.Logger.Warn($"The `{Constants.AppSettings.SqlWriteLockTimeOut}` setting in web.config is not between the minimum of {minimumTimeOut} ms and maximum of {maximumTimeOut} ms, defaulting back to {timeOut}"); + } } + + _sqlWriteLockTimeOut = timeOut; + return _sqlWriteLockTimeOut; } } } diff --git a/src/Umbraco.Core/Constants-AppSettings.cs b/src/Umbraco.Core/Constants-AppSettings.cs index 0182034011..f04f0e1f5f 100644 --- a/src/Umbraco.Core/Constants-AppSettings.cs +++ b/src/Umbraco.Core/Constants-AppSettings.cs @@ -144,9 +144,6 @@ namespace Umbraco.Core /// /// An int value representing the time in milliseconds to lock the database for a write operation /// - /// - /// The default value is 1800 milliseconds - /// public const string SqlWriteLockTimeOut = "Umbraco.Core.SqlWriteLockTimeOut"; } } diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/ISqlSyntaxProvider.cs b/src/Umbraco.Core/Persistence/SqlSyntax/ISqlSyntaxProvider.cs index 7ae001bf24..4c322f9648 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/ISqlSyntaxProvider.cs +++ b/src/Umbraco.Core/Persistence/SqlSyntax/ISqlSyntaxProvider.cs @@ -9,6 +9,12 @@ using Umbraco.Core.Persistence.Querying; namespace Umbraco.Core.Persistence.SqlSyntax { + public interface ISqlSyntaxProvider2 : ISqlSyntaxProvider + { + void ReadLock(IDatabase db, TimeSpan timeout, int lockId); + void WriteLock(IDatabase db, TimeSpan timeout, int lockId); + } + /// /// Defines an SqlSyntaxProvider /// @@ -77,7 +83,7 @@ namespace Umbraco.Core.Persistence.SqlSyntax string ConvertIntegerToOrderableString { get; } string ConvertDateToOrderableString { get; } string ConvertDecimalToOrderableString { get; } - + /// /// Returns the default isolation level for the database /// diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs b/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs index 046f54405a..127d00b561 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs +++ b/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs @@ -158,6 +158,16 @@ where table_name=@0 and column_name=@1", tableName, columnName).FirstOrDefault() return result > 0; } + public override void WriteLock(IDatabase db, TimeSpan timeout, int lockId) + { + // 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."); + + ObtainWriteLock(db, timeout, lockId); + } + public override void WriteLock(IDatabase db, params int[] lockIds) { // soon as we get Database, a transaction is started @@ -165,17 +175,32 @@ where table_name=@0 and column_name=@1", tableName, columnName).FirstOrDefault() if (db.Transaction.IsolationLevel < IsolationLevel.RepeatableRead) throw new InvalidOperationException("A transaction with minimum RepeatableRead isolation level is required."); - var timeOut = Current.Configs.Global().SqlWriteLockTimeOut; - db.Execute(@"SET LOCK_TIMEOUT " + timeOut + ";"); - // *not* using a unique 'WHERE IN' query here because the *order* of lockIds is important to avoid deadlocks + var timeout = TimeSpan.FromMilliseconds(Current.Configs.Global().SqlWriteLockTimeOut); + foreach (var lockId in lockIds) { - var i = db.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 ArgumentException($"LockObject with id={lockId} does not exist."); + ObtainWriteLock(db, timeout, lockId); } } + private static void ObtainWriteLock(IDatabase db, TimeSpan timeout, int lockId) + { + db.Execute(@"SET LOCK_TIMEOUT " + timeout.TotalMilliseconds + ";"); + var i = db.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 ArgumentException($"LockObject with id={lockId} does not exist."); + } + + public override void ReadLock(IDatabase db, TimeSpan timeout, int lockId) + { + // 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."); + + ObtainReadLock(db, timeout, lockId); + } + public override void ReadLock(IDatabase db, params int[] lockIds) { // soon as we get Database, a transaction is started @@ -183,15 +208,25 @@ where table_name=@0 and column_name=@1", tableName, columnName).FirstOrDefault() 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("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."); + ObtainReadLock(db, null, lockId); } } + private static void ObtainReadLock(IDatabase db, TimeSpan? timeout, int lockId) + { + if (timeout.HasValue) + { + db.Execute(@"SET LOCK_TIMEOUT " + timeout.Value.TotalMilliseconds + ";"); + } + + var i = db.ExecuteScalar("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; diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs b/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs index 3fc5e36f6e..372b6837bc 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs +++ b/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs @@ -254,30 +254,50 @@ where tbl.[name]=@0 and col.[name]=@1;", tableName, columnName) return result > 0; } - public override void WriteLock(IDatabase db, params int[] lockIds) - { - var timeOut = Current.Configs.Global().SqlWriteLockTimeOut; - WriteLock(db, TimeSpan.FromMilliseconds(timeOut), lockIds); - } - - public void WriteLock(IDatabase db, TimeSpan timeout, params int[] lockIds) + public override void WriteLock(IDatabase db, TimeSpan timeout, int lockId) { // 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."); + ObtainWriteLock(db, timeout, lockId); + } + + 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."); + + var timeout = TimeSpan.FromMilliseconds(Current.Configs.Global().SqlWriteLockTimeOut); - // *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 {timeout.TotalMilliseconds};"); - var i = db.Execute(@"UPDATE umbracoLock WITH (REPEATABLEREAD) 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 ArgumentException($"LockObject with id={lockId} does not exist."); + ObtainWriteLock(db, timeout, lockId); } } + private static void ObtainWriteLock(IDatabase db, TimeSpan timeout, int lockId) + { + db.Execute("SET LOCK_TIMEOUT" + timeout.TotalMilliseconds + ";"); + var i = db.Execute( + @"UPDATE umbracoLock WITH (REPEATABLEREAD) 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 ArgumentException($"LockObject with id={lockId} does not exist."); + } + + public override void ReadLock(IDatabase db, TimeSpan timeout, int lockId) + { + // soon as we get Database, a transaction is started + + if (db.Transaction.IsolationLevel < IsolationLevel.ReadCommitted) + throw new InvalidOperationException("A transaction with minimum RepeatableRead isolation level is required."); + + ObtainReadLock(db, timeout, lockId); + } public override void ReadLock(IDatabase db, params int[] lockIds) { @@ -286,15 +306,25 @@ where tbl.[name]=@0 and col.[name]=@1;", tableName, columnName) 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("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)); + ObtainReadLock(db, null, lockId); } } + private static void ObtainReadLock(IDatabase db, TimeSpan? timeout, int lockId) + { + if (timeout.HasValue) + { + db.Execute(@"SET LOCK_TIMEOUT " + timeout.Value.TotalMilliseconds + ";"); + } + + var i = db.ExecuteScalar("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."); + } + public override string FormatColumnRename(string tableName, string oldName, string newName) { return string.Format(RenameColumn, tableName, oldName, newName); diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs b/src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs index 8570c49f69..6f13afb24c 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs +++ b/src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs @@ -19,7 +19,7 @@ namespace Umbraco.Core.Persistence.SqlSyntax /// All Sql Syntax provider implementations should derive from this abstract class. /// /// - public abstract class SqlSyntaxProviderBase : ISqlSyntaxProvider + public abstract class SqlSyntaxProviderBase : ISqlSyntaxProvider2 where TSyntax : ISqlSyntaxProvider { protected SqlSyntaxProviderBase() @@ -235,6 +235,9 @@ namespace Umbraco.Core.Persistence.SqlSyntax public abstract void ReadLock(IDatabase db, params int[] lockIds); public abstract void WriteLock(IDatabase db, params int[] lockIds); + public abstract void ReadLock(IDatabase db, TimeSpan timeout, int lockId); + + public abstract void WriteLock(IDatabase db, TimeSpan timeout, int lockId); public virtual bool DoesTableExist(IDatabase db, string tableName) { diff --git a/src/Umbraco.Core/Scoping/IScope.cs b/src/Umbraco.Core/Scoping/IScope.cs index de4eef0a08..0c38031558 100644 --- a/src/Umbraco.Core/Scoping/IScope.cs +++ b/src/Umbraco.Core/Scoping/IScope.cs @@ -5,6 +5,24 @@ using Umbraco.Core.Persistence; namespace Umbraco.Core.Scoping { + // TODO: This is for backward compat - Merge this in netcore + public interface IScope2 : IScope + { + /// + /// Write-locks some lock objects. + /// + /// The database timeout in milliseconds + /// The lock object identifier. + void WriteLock(TimeSpan timeout, int lockId); + + /// + /// Read-locks some lock objects. + /// + /// The database timeout in milliseconds + /// The lock object identifier. + void ReadLock(TimeSpan timeout, int lockId); + } + /// /// Represents a scope. /// diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 84273e23da..24ef92278c 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -6,6 +6,7 @@ using Umbraco.Core.Events; using Umbraco.Core.IO; using Umbraco.Core.Logging; using Umbraco.Core.Persistence; +using Umbraco.Core.Persistence.SqlSyntax; namespace Umbraco.Core.Scoping { @@ -13,7 +14,7 @@ namespace Umbraco.Core.Scoping /// Implements . /// /// Not thread-safe obviously. - internal class Scope : IScope + internal class Scope : IScope2 { private readonly ScopeProvider _scopeProvider; private readonly ILogger _logger; @@ -488,7 +489,29 @@ namespace Umbraco.Core.Scoping /// public void ReadLock(params int[] lockIds) => Database.SqlContext.SqlSyntax.ReadLock(Database, lockIds); + /// + public void ReadLock(TimeSpan timeout, int lockId) + { + var syntax2 = Database.SqlContext.SqlSyntax as ISqlSyntaxProvider2; + if (syntax2 == null) + { + throw new InvalidOperationException($"{Database.SqlContext.SqlSyntax.GetType()} is not of type {typeof(ISqlSyntaxProvider2)}"); + } + syntax2.ReadLock(Database, timeout, lockId); + } + /// public void WriteLock(params int[] lockIds) => Database.SqlContext.SqlSyntax.WriteLock(Database, lockIds); + + /// + public void WriteLock(TimeSpan timeout, int lockId) + { + var syntax2 = Database.SqlContext.SqlSyntax as ISqlSyntaxProvider2; + if (syntax2 == null) + { + throw new InvalidOperationException($"{Database.SqlContext.SqlSyntax.GetType()} is not of type {typeof(ISqlSyntaxProvider2)}"); + } + syntax2.WriteLock(Database, timeout, lockId); + } } } diff --git a/src/Umbraco.Tests/Persistence/LocksTests.cs b/src/Umbraco.Tests/Persistence/LocksTests.cs index afcd481f9f..1c651b9040 100644 --- a/src/Umbraco.Tests/Persistence/LocksTests.cs +++ b/src/Umbraco.Tests/Persistence/LocksTests.cs @@ -1,11 +1,14 @@ using System; +using System.Collections.Generic; using System.Data.SqlServerCe; using System.Linq; using System.Threading; +using System.Threading.Tasks; using NPoco; using NUnit.Framework; using Umbraco.Core; using Umbraco.Core.Persistence.Dtos; +using Umbraco.Core.Scoping; using Umbraco.Tests.TestHelpers; using Umbraco.Tests.Testing; @@ -275,6 +278,152 @@ namespace Umbraco.Tests.Persistence Assert.IsNull(e2); } + [Test] + public void Throws_When_Lock_Timeout_Is_Exceeded() + { + var t1 = Task.Run(() => + { + using (var scope = ScopeProvider.CreateScope()) + { + var realScope = (Scope)scope; + + Console.WriteLine("Write lock A"); + // This will acquire right away + realScope.WriteLock(TimeSpan.FromMilliseconds(2000), Constants.Locks.ContentTree); + Thread.Sleep(6000); // Wait longer than the Read Lock B timeout + scope.Complete(); + Console.WriteLine("Finished Write lock A"); + } + }); + + Thread.Sleep(500); // 100% sure task 1 starts first + + var t2 = Task.Run(() => + { + using (var scope = ScopeProvider.CreateScope()) + { + var realScope = (Scope)scope; + + Console.WriteLine("Read lock B"); + + // This will wait for the write lock to release but it isn't going to wait long + // enough so an exception will be thrown. + Assert.Throws(() => + realScope.ReadLock(TimeSpan.FromMilliseconds(3000), Constants.Locks.ContentTree)); + + scope.Complete(); + Console.WriteLine("Finished Read lock B"); + } + }); + + var t3 = Task.Run(() => + { + using (var scope = ScopeProvider.CreateScope()) + { + var realScope = (Scope)scope; + + Console.WriteLine("Write lock C"); + + // This will wait for the write lock to release but it isn't going to wait long + // enough so an exception will be thrown. + Assert.Throws(() => + realScope.WriteLock(TimeSpan.FromMilliseconds(3000), Constants.Locks.ContentTree)); + + scope.Complete(); + Console.WriteLine("Finished Write lock C"); + } + }); + + Task.WaitAll(t1, t2, t3); + } + + [Test] + public void Read_Lock_Waits_For_Write_Lock() + { + var locksCompleted = 0; + + var t1 = Task.Run(() => + { + using (var scope = ScopeProvider.CreateScope()) + { + var realScope = (Scope)scope; + + Console.WriteLine("Write lock A"); + // This will acquire right away + realScope.WriteLock(TimeSpan.FromMilliseconds(2000), Constants.Locks.ContentTree); + Thread.Sleep(4000); // Wait less than the Read Lock B timeout + scope.Complete(); + Interlocked.Increment(ref locksCompleted); + Console.WriteLine("Finished Write lock A"); + } + }); + + Thread.Sleep(500); // 100% sure task 1 starts first + + var t2 = Task.Run(() => + { + using (var scope = ScopeProvider.CreateScope()) + { + var realScope = (Scope)scope; + + Console.WriteLine("Read lock B"); + + // This will wait for the write lock to release + Assert.DoesNotThrow(() => + realScope.ReadLock(TimeSpan.FromMilliseconds(6000), Constants.Locks.ContentTree)); + + Assert.GreaterOrEqual(locksCompleted, 1); + + scope.Complete(); + Interlocked.Increment(ref locksCompleted); + Console.WriteLine("Finished Read lock B"); + } + }); + + var t3 = Task.Run(() => + { + using (var scope = ScopeProvider.CreateScope()) + { + var realScope = (Scope)scope; + + Console.WriteLine("Read lock C"); + + // This will wait for the write lock to release + Assert.DoesNotThrow(() => + realScope.ReadLock(TimeSpan.FromMilliseconds(6000), Constants.Locks.ContentTree)); + + Assert.GreaterOrEqual(locksCompleted, 1); + + scope.Complete(); + Interlocked.Increment(ref locksCompleted); + Console.WriteLine("Finished Read lock C"); + } + }); + + Task.WaitAll(t1, t2, t3); + + Assert.AreEqual(3, locksCompleted); + } + + [Test] + [NUnit.Framework.Ignore("We cannot run this test with SQLCE because it does not support a Command Timeout")] + public void Lock_Exceeds_Command_Timeout() + { + using (var scope = ScopeProvider.CreateScope()) + { + var realScope = (Scope)scope; + + var realDb = (Database)realScope.Database; + realDb.CommandTimeout = 1000; + + Console.WriteLine("Write lock A"); + // TODO: In theory this would throw + realScope.WriteLock(TimeSpan.FromMilliseconds(3000), Constants.Locks.ContentTree); + scope.Complete(); + Console.WriteLine("Finished Write lock A"); + } + } + private void NoDeadLockTestThread(int id, EventWaitHandle myEv, WaitHandle otherEv, ref Exception exception) { using (var scope = ScopeProvider.CreateScope()) diff --git a/src/Umbraco.Tests/TestHelpers/SettingsForTests.cs b/src/Umbraco.Tests/TestHelpers/SettingsForTests.cs index 1121a48823..c31c037c00 100644 --- a/src/Umbraco.Tests/TestHelpers/SettingsForTests.cs +++ b/src/Umbraco.Tests/TestHelpers/SettingsForTests.cs @@ -24,7 +24,7 @@ namespace Umbraco.Tests.TestHelpers settings.LocalTempPath == IOHelper.MapPath("~/App_Data/TEMP") && settings.ReservedPaths == (GlobalSettings.StaticReservedPaths + "~/umbraco") && settings.ReservedUrls == GlobalSettings.StaticReservedUrls && - settings.SqlWriteLockTimeOut == 1800); + settings.SqlWriteLockTimeOut == 5000); return config; }