diff --git a/src/Umbraco.Core/Persistence/DatabasenodeLockExtensions.cs b/src/Umbraco.Core/Persistence/DatabasenodeLockExtensions.cs deleted file mode 100644 index 48edee3c94..0000000000 --- a/src/Umbraco.Core/Persistence/DatabasenodeLockExtensions.cs +++ /dev/null @@ -1,43 +0,0 @@ -using System; -using System.Collections.Generic; -using System.Data; -using System.Runtime.CompilerServices; - -namespace Umbraco.Core.Persistence -{ - internal static class DatabaseNodeLockExtensions - { - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static void ValidateDatabase(IUmbracoDatabase database) - { - if (database == null) - throw new ArgumentNullException("database"); - if (database.GetCurrentTransactionIsolationLevel() < IsolationLevel.RepeatableRead) - throw new InvalidOperationException("A transaction with minimum RepeatableRead isolation level is required."); - } - - // updating a record within a repeatable-read transaction gets an exclusive lock on - // that record which will be kept until the transaction is ended, effectively locking - // out all other accesses to that record - thus obtaining an exclusive lock over the - // protected resources. - public static void AcquireLockNodeWriteLock(this IUmbracoDatabase database, int nodeId) - { - ValidateDatabase(database); - - database.Execute("UPDATE umbracoLock SET value = (CASE WHEN (value=1) THEN -1 ELSE 1 END) WHERE id=@id", - new { @id = nodeId }); - } - - // reading a record within a repeatable-read transaction gets a shared lock on - // that record which will be kept until the transaction is ended, effectively preventing - // other write accesses to that record - thus obtaining a shared lock over the protected - // resources. - public static void AcquireLockNodeReadLock(this IUmbracoDatabase database, int nodeId) - { - ValidateDatabase(database); - - database.ExecuteScalar("SELECT value FROM umbracoLock WHERE id=@id", - new { @id = nodeId }); - } - } -} diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/ISqlSyntaxProvider.cs b/src/Umbraco.Core/Persistence/SqlSyntax/ISqlSyntaxProvider.cs index 55625ff04e..7ae001bf24 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/ISqlSyntaxProvider.cs +++ b/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 @@ namespace Umbraco.Core.Persistence.SqlSyntax string ConvertIntegerToOrderableString { get; } string ConvertDateToOrderableString { get; } string ConvertDecimalToOrderableString { get; } + + /// + /// Returns the default isolation level for the database + /// + IsolationLevel DefaultIsolationLevel { get; } IEnumerable GetTablesInSchema(IDatabase db); IEnumerable GetColumnsInSchema(IDatabase db); @@ -121,5 +127,8 @@ namespace Umbraco.Core.Persistence.SqlSyntax /// unspecified. /// 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); } } diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs b/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs index cb4b7a5176..2ed0fb878c 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/SqlCeSyntaxProvider.cs +++ b/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 @@ namespace Umbraco.Core.Persistence.SqlSyntax 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 @@ where table_name=@0 and column_name=@1", tableName, columnName).FirstOrDefault() 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 = (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, 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("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 fab7526a6b..3d0adf175e 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/SqlServerSyntaxProvider.cs +++ b/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 @@ namespace Umbraco.Core.Persistence.SqlSyntax return items.Select(x => x.TABLE_NAME).Cast().ToList(); } + public override IsolationLevel DefaultIsolationLevel => IsolationLevel.ReadCommitted; + public override IEnumerable GetColumnsInSchema(IDatabase db) { var items = db.Fetch("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 @@ where tbl.[name]=@0 and col.[name]=@1;", tableName, columnName) 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 = (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, 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("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); diff --git a/src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs b/src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs index 0c27ac2d50..b2e03df96e 100644 --- a/src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs +++ b/src/Umbraco.Core/Persistence/SqlSyntax/SqlSyntaxProviderBase.cs @@ -200,7 +200,9 @@ namespace Umbraco.Core.Persistence.SqlSyntax return "NVARCHAR"; } - + + public abstract IsolationLevel DefaultIsolationLevel { get; } + public virtual IEnumerable GetTablesInSchema(IDatabase db) { return new List(); @@ -225,6 +227,9 @@ namespace Umbraco.Core.Persistence.SqlSyntax 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; diff --git a/src/Umbraco.Core/Persistence/UmbracoDatabase.cs b/src/Umbraco.Core/Persistence/UmbracoDatabase.cs index 51e0172f35..072813b4e6 100644 --- a/src/Umbraco.Core/Persistence/UmbracoDatabase.cs +++ b/src/Umbraco.Core/Persistence/UmbracoDatabase.cs @@ -20,9 +20,6 @@ namespace Umbraco.Core.Persistence /// 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 @@ namespace Umbraco.Core.Persistence /// Also used by DatabaseBuilder for creating databases and installing/upgrading. /// 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 @@ namespace Umbraco.Core.Persistence /// /// Internal for unit tests only. internal UmbracoDatabase(DbConnection connection, ISqlContext sqlContext, ILogger logger) - : base(connection, sqlContext.DatabaseType, DefaultIsolationLevel) + : base(connection, sqlContext.DatabaseType, sqlContext.SqlSyntax.DefaultIsolationLevel) { SqlContext = sqlContext; _logger = logger; diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index e9dd04c5fa..84273e23da 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -33,8 +33,6 @@ namespace Umbraco.Core.Scoping 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 @@ namespace Umbraco.Core.Scoping { if (_isolationLevel != IsolationLevel.Unspecified) return _isolationLevel; if (ParentScope != null) return ParentScope.IsolationLevel; - return DefaultIsolationLevel; + return Database.SqlContext.SqlSyntax.DefaultIsolationLevel; } } @@ -488,37 +486,9 @@ namespace Umbraco.Core.Scoping ?? (_logUncompletedScopes = Current.Configs.CoreDebug().LogUncompletedScopes)).Value; /// - 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("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); /// - 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); } } diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 43d168f442..80db137b6c 100755 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -984,7 +984,6 @@ - diff --git a/src/Umbraco.Tests/Models/UserExtensionsTests.cs b/src/Umbraco.Tests/Models/UserExtensionsTests.cs index d9c8057377..bf76031b59 100644 --- a/src/Umbraco.Tests/Models/UserExtensionsTests.cs +++ b/src/Umbraco.Tests/Models/UserExtensionsTests.cs @@ -57,6 +57,9 @@ namespace Umbraco.Tests.Models [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 diff --git a/src/Umbraco.Tests/Persistence/LocksTests.cs b/src/Umbraco.Tests/Persistence/LocksTests.cs index 56779ace0f..afcd481f9f 100644 --- a/src/Umbraco.Tests/Persistence/LocksTests.cs +++ b/src/Umbraco.Tests/Persistence/LocksTests.cs @@ -5,7 +5,6 @@ using System.Threading; 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 @@ namespace Umbraco.Tests.Persistence { using (var scope = ScopeProvider.CreateScope()) { - scope.Database.AcquireLockNodeReadLock(Constants.Locks.Servers); + scope.ReadLock(Constants.Locks.Servers); scope.Complete(); } } @@ -62,7 +61,7 @@ namespace Umbraco.Tests.Persistence { try { - scope.Database.AcquireLockNodeReadLock(Constants.Locks.Servers); + scope.ReadLock(Constants.Locks.Servers); lock (locker) { acquired++; @@ -131,7 +130,7 @@ namespace Umbraco.Tests.Persistence 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 @@ namespace Umbraco.Tests.Persistence { 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 @@ namespace Umbraco.Tests.Persistence 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 @@ namespace Umbraco.Tests.Persistence { otherEv.WaitOne(); Console.WriteLine($"[{id}] WAIT {id}"); - scope.Database.AcquireLockNodeWriteLock(id); + scope.WriteLock(id); Console.WriteLine($"[{id}] GRANT {id}"); WriteLocks(scope.Database); myEv.Set(); diff --git a/src/Umbraco.Tests/Persistence/UnitOfWorkTests.cs b/src/Umbraco.Tests/Persistence/UnitOfWorkTests.cs index 5651dd1457..02ad6b3971 100644 --- a/src/Umbraco.Tests/Persistence/UnitOfWorkTests.cs +++ b/src/Umbraco.Tests/Persistence/UnitOfWorkTests.cs @@ -14,7 +14,7 @@ namespace Umbraco.Tests.Persistence public void ReadLockNonExisting() { var provider = TestObjects.GetScopeProvider(Logger); - Assert.Throws(() => + Assert.Throws(() => { using (var scope = provider.CreateScope()) { @@ -39,7 +39,7 @@ namespace Umbraco.Tests.Persistence public void WriteLockNonExisting() { var provider = TestObjects.GetScopeProvider(Logger); - Assert.Throws(() => + Assert.Throws(() => { using (var scope = provider.CreateScope()) {