diff --git a/src/Umbraco.Core/Persistence/LockedRepository.cs b/src/Umbraco.Core/Persistence/LockedRepository.cs index b5d2d672f2..c092a3329f 100644 --- a/src/Umbraco.Core/Persistence/LockedRepository.cs +++ b/src/Umbraco.Core/Persistence/LockedRepository.cs @@ -7,21 +7,27 @@ namespace Umbraco.Core.Persistence internal class LockedRepository where TRepository : IDisposable, IRepository { - public LockedRepository(Transaction transaction, IDatabaseUnitOfWork unitOfWork, TRepository repository) + public LockedRepository(IDatabaseUnitOfWork unitOfWork, TRepository repository) { - Transaction = transaction; UnitOfWork = unitOfWork; Repository = repository; } - public Transaction Transaction { get; private set; } + public LockedRepository(Transaction transaction, IDatabaseUnitOfWork unitOfWork, TRepository repository) + { + //Transaction = transaction; + UnitOfWork = unitOfWork; + Repository = repository; + } + + //public Transaction Transaction { get; private set; } public IDatabaseUnitOfWork UnitOfWork { get; private set; } public TRepository Repository { get; private set; } - public void Commit() - { - UnitOfWork.Commit(); - Transaction.Complete(); - } + //public void Commit() + //{ + // UnitOfWork.Commit(); + // Transaction.Complete(); + //} } } \ No newline at end of file diff --git a/src/Umbraco.Core/Persistence/LockingRepository.cs b/src/Umbraco.Core/Persistence/LockingRepository.cs index 88df2c492e..09195f8ce6 100644 --- a/src/Umbraco.Core/Persistence/LockingRepository.cs +++ b/src/Umbraco.Core/Persistence/LockingRepository.cs @@ -28,92 +28,88 @@ namespace Umbraco.Core.Persistence public void WithReadLocked(Action> action, bool autoCommit = true) { - var uow = _uowProvider.GetUnitOfWork(); - using (var transaction = uow.Database.GetTransaction(IsolationLevel.RepeatableRead)) + using (var uow = ((PetaPocoUnitOfWorkProvider) _uowProvider).GetUnitOfWork(IsolationLevel.RepeatableRead)) { + // getting the database creates a scope and a transaction + // the scope is IsolationLevel.RepeatableRead (because UnitOfWork is) + // and will throw if outer scope (if any) has a lower isolation level + foreach (var lockId in _readLockIds) uow.Database.AcquireLockNodeReadLock(lockId); using (var repository = _repositoryFactory(uow)) { - action(new LockedRepository(transaction, uow, repository)); + action(new LockedRepository(uow, repository)); if (autoCommit == false) return; uow.Commit(); - transaction.Complete(); - } - } + + } // dispose repository => dispose uow => complete (or not) scope + } // dispose uow again => nothing } public TResult WithReadLocked(Func, TResult> func, bool autoCommit = true) { - using (var uow = _uowProvider.GetUnitOfWork()) - using (var transaction = uow.Database.GetTransaction(IsolationLevel.RepeatableRead)) + using (var uow = ((PetaPocoUnitOfWorkProvider) _uowProvider).GetUnitOfWork(IsolationLevel.RepeatableRead)) { + // getting the database creates a scope and a transaction + // the scope is IsolationLevel.RepeatableRead (because UnitOfWork is) + // and will throw if outer scope (if any) has a lower isolation level + foreach (var lockId in _readLockIds) uow.Database.AcquireLockNodeReadLock(lockId); using (var repository = _repositoryFactory(uow)) { - var ret = func(new LockedRepository(transaction, uow, repository)); + var ret = func(new LockedRepository(uow, repository)); if (autoCommit == false) return ret; uow.Commit(); - transaction.Complete(); return ret; - } - } + + } // dispose repository => dispose uow => complete (or not) scope + } // dispose uow again => nothing } public void WithWriteLocked(Action> action, bool autoCommit = true) { - // must use the uow to ensure it's disposed if GetTransaction fails! - - using (var uow = _uowProvider.GetUnitOfWork()) - using (var transaction = uow.Database.GetTransaction(IsolationLevel.RepeatableRead)) + using (var uow = ((PetaPocoUnitOfWorkProvider) _uowProvider).GetUnitOfWork(IsolationLevel.RepeatableRead)) { + // getting the database creates a scope and a transaction + // the scope is IsolationLevel.RepeatableRead (because UnitOfWork is) + // and will throw if outer scope (if any) has a lower isolation level + foreach (var lockId in _writeLockIds) uow.Database.AcquireLockNodeWriteLock(lockId); using (var repository = _repositoryFactory(uow)) { - action(new LockedRepository(transaction, uow, repository)); + action(new LockedRepository(uow, repository)); if (autoCommit == false) return; uow.Commit(); - transaction.Complete(); - } - } - // fixme - // the change above ensures that scopes are properly disposed - // TODO apply to all methods - // now how can we manage the isolation level? - //uow.ScopeIsolationLevel = IsolationLevel.RepeatableRead; // might throw when creating the scope - // getTransaction here throws because of different levels - // but the exception gets eaten because on the way out, disposing the _outer_ scope fails - // so... how shall we figure it out?! - // - we should be able to set the isolation level on the uow scope - // - should we be able to dispose parent scopes and then what happens? NO! - // - // if we can create scopes with isolation levels what happens when we nest scopes? - // note: this is a *very* special use case + } // dispose repository => dispose uow => complete (or not) scope + } // dispose uow again => nothing } public TResult WithWriteLocked(Func, TResult> func, bool autoCommit = true) { - using (var uow = _uowProvider.GetUnitOfWork()) - using (var transaction = uow.Database.GetTransaction(IsolationLevel.RepeatableRead)) + using (var uow = ((PetaPocoUnitOfWorkProvider) _uowProvider).GetUnitOfWork(IsolationLevel.RepeatableRead)) { + // getting the database creates a scope and a transaction + // the scope is IsolationLevel.RepeatableRead (because UnitOfWork is) + // and will throw if outer scope (if any) has a lower isolation level + foreach (var lockId in _writeLockIds) uow.Database.AcquireLockNodeReadLock(lockId); using (var repository = _repositoryFactory(uow)) { - var ret = func(new LockedRepository(transaction, uow, repository)); + var ret = func(new LockedRepository(uow, repository)); if (autoCommit == false) return ret; uow.Commit(); - transaction.Complete(); return ret; - } - } + + } // dispose repository => dispose uow => complete (or not) scope + } // dispose uow again => nothing } } } diff --git a/src/Umbraco.Core/Persistence/UnitOfWork/IDatabaseUnitOfWorkProvider.cs b/src/Umbraco.Core/Persistence/UnitOfWork/IDatabaseUnitOfWorkProvider.cs index c18c08d8bb..a63299d0a6 100644 --- a/src/Umbraco.Core/Persistence/UnitOfWork/IDatabaseUnitOfWorkProvider.cs +++ b/src/Umbraco.Core/Persistence/UnitOfWork/IDatabaseUnitOfWorkProvider.cs @@ -6,5 +6,5 @@ namespace Umbraco.Core.Persistence.UnitOfWork public interface IDatabaseUnitOfWorkProvider { IDatabaseUnitOfWork GetUnitOfWork(); - } + } } \ No newline at end of file diff --git a/src/Umbraco.Core/Persistence/UnitOfWork/PetaPocoUnitOfWork.cs b/src/Umbraco.Core/Persistence/UnitOfWork/PetaPocoUnitOfWork.cs index 5398a00095..9caf787ddc 100644 --- a/src/Umbraco.Core/Persistence/UnitOfWork/PetaPocoUnitOfWork.cs +++ b/src/Umbraco.Core/Persistence/UnitOfWork/PetaPocoUnitOfWork.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Data; using Umbraco.Core.Models.EntityBase; using Umbraco.Core.Scoping; @@ -11,6 +12,7 @@ namespace Umbraco.Core.Persistence.UnitOfWork internal class PetaPocoUnitOfWork : DisposableObject, IDatabaseUnitOfWork { private readonly Queue _operations = new Queue(); + private readonly IsolationLevel _isolationLevel; private readonly IScopeProvider _scopeProvider; private bool _completeScope = true; // scope is completed by default private IScope _scope; @@ -28,9 +30,10 @@ namespace Umbraco.Core.Persistence.UnitOfWork /// /// This should normally not be used directly and should be created with the UnitOfWorkProvider /// - internal PetaPocoUnitOfWork(IScopeProvider scopeProvider) + internal PetaPocoUnitOfWork(IScopeProvider scopeProvider, IsolationLevel isolationLevel = IsolationLevel.Unspecified) { _scopeProvider = scopeProvider; + _isolationLevel = isolationLevel; _key = Guid.NewGuid(); InstanceId = Guid.NewGuid(); } @@ -143,7 +146,7 @@ namespace Umbraco.Core.Persistence.UnitOfWork private IScope ThisScope { - get { return _scope ?? (_scope = _scopeProvider.CreateScope()); } + get { return _scope ?? (_scope = _scopeProvider.CreateScope(_isolationLevel)); } } public UmbracoDatabase Database diff --git a/src/Umbraco.Core/Persistence/UnitOfWork/PetaPocoUnitOfWorkProvider.cs b/src/Umbraco.Core/Persistence/UnitOfWork/PetaPocoUnitOfWorkProvider.cs index 6b901d34f7..4d1b638016 100644 --- a/src/Umbraco.Core/Persistence/UnitOfWork/PetaPocoUnitOfWorkProvider.cs +++ b/src/Umbraco.Core/Persistence/UnitOfWork/PetaPocoUnitOfWorkProvider.cs @@ -1,4 +1,5 @@ using System; +using System.Data; using Umbraco.Core.Configuration; using Umbraco.Core.Logging; using Umbraco.Core.Scoping; @@ -69,6 +70,11 @@ namespace Umbraco.Core.Persistence.UnitOfWork return new PetaPocoUnitOfWork(_scopeProvider); } + public IDatabaseUnitOfWork GetUnitOfWork(IsolationLevel isolationLevel) + { + return new PetaPocoUnitOfWork(_scopeProvider, isolationLevel); + } + #endregion /// diff --git a/src/Umbraco.Core/Scoping/IScopeProvider.cs b/src/Umbraco.Core/Scoping/IScopeProvider.cs index 6622b1a417..9a3e935a78 100644 --- a/src/Umbraco.Core/Scoping/IScopeProvider.cs +++ b/src/Umbraco.Core/Scoping/IScopeProvider.cs @@ -1,4 +1,5 @@ using System.Collections.Generic; +using System.Data; namespace Umbraco.Core.Scoping { @@ -16,7 +17,7 @@ namespace Umbraco.Core.Scoping /// If an ambient scope already exists, it becomes the parent of the created scope. /// When the created scope is disposed, the parent scope becomes the ambient scope again. /// - IScope CreateScope(); + IScope CreateScope(IsolationLevel isolationLevel = IsolationLevel.Unspecified); /// /// Creates a detached scope. @@ -26,7 +27,7 @@ namespace Umbraco.Core.Scoping /// A detached scope is not ambient and has no parent. /// It is meant to be attached by . /// - IScope CreateDetachedScope(); + IScope CreateDetachedScope(IsolationLevel isolationLevel = IsolationLevel.Unspecified); /// /// Attaches a scope. diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 5b220fe433..14d7fdd8d9 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Data; using Umbraco.Core.Events; using Umbraco.Core.Persistence; @@ -12,16 +13,21 @@ namespace Umbraco.Core.Scoping internal class Scope : IScope { private readonly ScopeProvider _scopeProvider; + private readonly IsolationLevel _isolationLevel; private bool _disposed; private bool? _completed; private UmbracoDatabase _database; private IList _messages; + // this is v7, in v8 this has to change to RepeatableRead + private const IsolationLevel DefaultIsolationLevel = IsolationLevel.ReadCommitted; + // initializes a new scope - public Scope(ScopeProvider scopeProvider, bool detachable = false) + public Scope(ScopeProvider scopeProvider, IsolationLevel isolationLevel = IsolationLevel.Unspecified, bool detachable = false) { _scopeProvider = scopeProvider; + _isolationLevel = isolationLevel; Detachable = detachable; #if DEBUG_SCOPES _scopeProvider.Register(this); @@ -29,15 +35,15 @@ namespace Umbraco.Core.Scoping } // initializes a new scope in a nested scopes chain, with its parent - public Scope(ScopeProvider scopeProvider, Scope parent) - : this(scopeProvider) + public Scope(ScopeProvider scopeProvider, Scope parent, IsolationLevel isolationLevel = IsolationLevel.Unspecified) + : this(scopeProvider, isolationLevel) { ParentScope = parent; } // initializes a new scope, replacing a NoScope instance - public Scope(ScopeProvider scopeProvider, NoScope noScope) - : this(scopeProvider) + public Scope(ScopeProvider scopeProvider, NoScope noScope, IsolationLevel isolationLevel = IsolationLevel.Unspecified) + : this(scopeProvider, isolationLevel) { // steal everything from NoScope _database = noScope.DatabaseOrNull; @@ -63,24 +69,56 @@ namespace Umbraco.Core.Scoping // the original scope (when attaching a detachable scope) public IScope OrigScope { get; set; } + private IsolationLevel IsolationLevel + { + get + { + if (_isolationLevel != IsolationLevel.Unspecified) return _isolationLevel; + if (ParentScope != null) return ParentScope.IsolationLevel; + return DefaultIsolationLevel; + } + } + /// public UmbracoDatabase Database { get { EnsureNotDisposed(); - if (ParentScope != null) return ParentScope.Database; + if (ParentScope != null) + { + var database = ParentScope.Database; + if (_isolationLevel > IsolationLevel.Unspecified && database.CurrentTransactionIsolationLevel < _isolationLevel) + throw new Exception("Scope requires isolation level " + _isolationLevel + ", but got " + database.CurrentTransactionIsolationLevel + " from parent."); + } + if (_database != null) { - if (_database.InTransaction == false) // stolen from noScope - _database.BeginTransaction(); // a scope implies a transaction, always - // fixme - what-if exception? + // if the database has been created by a Scope instance it has to be + // in a transaction, however it can be a database that was stolen from + // a NoScope instance, in which case we need to enter a transaction, as + // a scope implies a transaction, always + if (_database.InTransaction) + return _database; + } + else + { + // create a new database + _database = _scopeProvider.DatabaseFactory.CreateNewDatabase(); + } + + // enter a transaction, as a scope implies a transaction, always + try + { + _database.BeginTransaction(IsolationLevel); return _database; } - var database = _scopeProvider.DatabaseFactory.CreateNewDatabase(); - database.BeginTransaction(); // a scope implies a transaction, always - // fixme - should dispose db on exception? - return _database = database; + catch + { + _database.Dispose(); + _database = null; + throw; + } } } diff --git a/src/Umbraco.Core/Scoping/ScopeProvider.cs b/src/Umbraco.Core/Scoping/ScopeProvider.cs index 7f43b7208e..7843b4f92d 100644 --- a/src/Umbraco.Core/Scoping/ScopeProvider.cs +++ b/src/Umbraco.Core/Scoping/ScopeProvider.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Data; using System.Linq; using System.Runtime.Remoting.Messaging; using System.Web; @@ -113,9 +114,9 @@ namespace Umbraco.Core.Scoping } /// - public IScope CreateDetachedScope() + public IScope CreateDetachedScope(IsolationLevel isolationLevel = IsolationLevel.Unspecified) { - return new Scope(this, true); + return new Scope(this, isolationLevel, true); } /// @@ -156,11 +157,11 @@ namespace Umbraco.Core.Scoping } /// - public IScope CreateScope() + public IScope CreateScope(IsolationLevel isolationLevel = IsolationLevel.Unspecified) { var ambient = AmbientScope; if (ambient == null) - return AmbientScope = new Scope(this); + return AmbientScope = new Scope(this, isolationLevel); // replace noScope with a real one var noScope = ambient as NoScope; @@ -173,13 +174,13 @@ namespace Umbraco.Core.Scoping var database = noScope.DatabaseOrNull; if (database != null && database.InTransaction) throw new Exception("NoScope is in a transaction."); - return AmbientScope = new Scope(this, noScope); + return AmbientScope = new Scope(this, noScope, isolationLevel); } var scope = ambient as Scope; if (scope == null) throw new Exception("Ambient scope is not a Scope instance."); - return AmbientScope = new Scope(this, scope); + return AmbientScope = new Scope(this, scope, isolationLevel); } /// diff --git a/src/Umbraco.Tests/Persistence/LocksTests.cs b/src/Umbraco.Tests/Persistence/LocksTests.cs index ae008e1e63..358e98e797 100644 --- a/src/Umbraco.Tests/Persistence/LocksTests.cs +++ b/src/Umbraco.Tests/Persistence/LocksTests.cs @@ -5,14 +5,8 @@ using System.Data.SqlServerCe; using System.Threading; using NUnit.Framework; using Umbraco.Core; -using Umbraco.Core.Events; using Umbraco.Core.Models.Rdbms; using Umbraco.Core.Persistence; -using Umbraco.Core.Persistence.SqlSyntax; -using Umbraco.Core.Persistence.UnitOfWork; -using Umbraco.Core.Publishing; -using Umbraco.Core.Scoping; -using Umbraco.Core.Services; using Umbraco.Tests.Services; using Umbraco.Tests.TestHelpers; using Ignore = NUnit.Framework.IgnoreAttribute; @@ -24,81 +18,32 @@ namespace Umbraco.Tests.Persistence [Ignore("Takes too much time.")] public class LocksTests : BaseDatabaseFactoryTest { - private ThreadSafetyServiceTest.PerThreadPetaPocoUnitOfWorkProvider _uowProvider; - private ThreadSafetyServiceTest.PerThreadDatabaseFactory _dbFactory; - [SetUp] public override void Initialize() { base.Initialize(); - //we need to use our own custom IDatabaseFactory for the DatabaseContext because we MUST ensure that - //a Database instance is created per thread, whereas the default implementation which will work in an HttpContext - //threading environment, or a single apartment threading environment will not work for this test because - //it is multi-threaded. - _dbFactory = new ThreadSafetyServiceTest.PerThreadDatabaseFactory(Logger); - var scopeProvider = new ScopeProvider(_dbFactory); - //overwrite the local object - ApplicationContext.DatabaseContext = new DatabaseContext(scopeProvider, Logger, new SqlCeSyntaxProvider(), Constants.DatabaseProviders.SqlCe); - - //disable cache - var cacheHelper = CacheHelper.CreateDisabledCacheHelper(); - - //here we are going to override the ServiceContext because normally with our test cases we use a - //global Database object but this is NOT how it should work in the web world or in any multi threaded scenario. - //we need a new Database object for each thread. - var repositoryFactory = new RepositoryFactory(cacheHelper, Logger, SqlSyntax, SettingsForTests.GenerateMockSettings()); - _uowProvider = new ThreadSafetyServiceTest.PerThreadPetaPocoUnitOfWorkProvider(_dbFactory); - var evtMsgs = new TransientMessagesFactory(); - ApplicationContext.Services = new ServiceContext( - repositoryFactory, - _uowProvider, - new FileUnitOfWorkProvider(), - new PublishingStrategy(evtMsgs, Logger), - cacheHelper, - Logger, - evtMsgs); - // create a few lock objects - var database = DatabaseContext.Database; - database.BeginTransaction(IsolationLevel.RepeatableRead); - try + using (var scope = ApplicationContext.ScopeProvider.CreateScope(IsolationLevel.RepeatableRead)) { + var database = scope.Database; database.Execute("SET IDENTITY_INSERT umbracoLock ON"); database.Insert("umbracoLock", "id", false, new LockDto { Id = 1, Name = "Lock.1" }); database.Insert("umbracoLock", "id", false, new LockDto { Id = 2, Name = "Lock.2" }); database.Insert("umbracoLock", "id", false, new LockDto { Id = 3, Name = "Lock.3" }); database.Execute("SET IDENTITY_INSERT umbracoLock OFF"); - database.CompleteTransaction(); + scope.Complete(); } - catch - { - database.AbortTransaction(); - } - } - - [TearDown] - public override void TearDown() - { - //dispose! - _dbFactory.Dispose(); - _uowProvider.Dispose(); - - base.TearDown(); } [Test] public void Test() { - var database = DatabaseContext.Database; - database.BeginTransaction(IsolationLevel.RepeatableRead); - try + using (var scope = ApplicationContext.ScopeProvider.CreateScope(IsolationLevel.RepeatableRead)) { + var database = scope.Database; database.AcquireLockNodeReadLock(Constants.Locks.Servers); - } - finally - { - database.CompleteTransaction(); + scope.Complete(); } } @@ -108,40 +53,39 @@ namespace Umbraco.Tests.Persistence var threads = new List(); var locker = new object(); var acquired = 0; - var maxAcquired = 0; + var m2 = new ManualResetEventSlim(false); + var m1 = new ManualResetEventSlim(false); for (var i = 0; i < 5; i++) { threads.Add(new Thread(() => { - var database = DatabaseContext.Database; - database.BeginTransaction(IsolationLevel.RepeatableRead); - try + // each thread gets its own scope, because it has its own LCC, hence its own database + using (var scope = ApplicationContext.ScopeProvider.CreateScope(IsolationLevel.RepeatableRead)) { + var database = scope.Database; database.AcquireLockNodeReadLock(Constants.Locks.Servers); lock (locker) { acquired++; + if (acquired == 5) m2.Set(); } - Thread.Sleep(500); - lock (locker) - { - if (maxAcquired < acquired) maxAcquired = acquired; - } - Thread.Sleep(500); + m1.Wait(); lock (locker) { acquired--; } - } - finally - { - database.CompleteTransaction(); + scope.Complete(); } })); } foreach (var thread in threads) thread.Start(); + m2.Wait(); + // all threads have locked in parallel + var maxAcquired = acquired; + m1.Set(); foreach (var thread in threads) thread.Join(); Assert.AreEqual(5, maxAcquired); + Assert.AreEqual(0, acquired); } [Test] @@ -149,41 +93,51 @@ namespace Umbraco.Tests.Persistence { var threads = new List(); var locker = new object(); + var entered = 0; + var ms = new AutoResetEvent[5]; + for (var i = 0; i < 5; i++) ms[i] = new AutoResetEvent(false); + var m1 = new ManualResetEventSlim(false); var acquired = 0; - var maxAcquired = 0; for (var i = 0; i < 5; i++) { + var ic = i; threads.Add(new Thread(() => { - var database = DatabaseContext.Database; - database.BeginTransaction(IsolationLevel.RepeatableRead); - try + // each thread gets its own scope, because it has its own LCC, hence its own database + using (var scope = ApplicationContext.ScopeProvider.CreateScope(IsolationLevel.RepeatableRead)) { + var database = scope.Database; + lock (locker) + { + entered++; + if (entered == 5) m1.Set(); + } + ms[ic].WaitOne(); database.AcquireLockNodeWriteLock(Constants.Locks.Servers); lock (locker) { acquired++; } - Thread.Sleep(500); - lock (locker) - { - if (maxAcquired < acquired) maxAcquired = acquired; - } - Thread.Sleep(500); + ms[ic].WaitOne(); lock (locker) { acquired--; } - } - finally - { - database.CompleteTransaction(); + scope.Complete(); } })); } foreach (var thread in threads) thread.Start(); + m1.Wait(); + // all threads have entered + ms[0].Set(); // let 0 go + Thread.Sleep(100); + for (var i = 1; i < 5; i++) ms[i].Set(); // let others go + Thread.Sleep(500); + // only 1 thread has locked + Assert.AreEqual(1, acquired); + for (var i = 0; i < 5; i++) ms[i].Set(); // let all go foreach (var thread in threads) thread.Join(); - Assert.AreEqual(1, maxAcquired); } [Test] @@ -193,52 +147,56 @@ namespace Umbraco.Tests.Persistence var thread1 = new Thread(() => { - var database = DatabaseContext.Database; - database.BeginTransaction(IsolationLevel.RepeatableRead); - try + // each thread gets its own scope, because it has its own LCC, hence its own database + using (var scope = ApplicationContext.ScopeProvider.CreateScope(IsolationLevel.RepeatableRead)) { - database.AcquireLockNodeWriteLock(1); - Thread.Sleep(1000); - database.AcquireLockNodeWriteLock(2); - Thread.Sleep(1000); - } - catch (Exception e) - { - e1 = e; - } - finally - { - database.CompleteTransaction(); + var database = scope.Database; + Console.WriteLine("Thread 1 db " + database.InstanceSid); + try + { + database.AcquireLockNodeWriteLock(1); + Thread.Sleep(100); + database.AcquireLockNodeWriteLock(2); + Thread.Sleep(1000); + } + catch (Exception e) + { + e1 = e; + } + scope.Complete(); } }); var thread2 = new Thread(() => { - var database = DatabaseContext.Database; - database.BeginTransaction(IsolationLevel.RepeatableRead); - try + // each thread gets its own scope, because it has its own LCC, hence its own database + using (var scope = ApplicationContext.ScopeProvider.CreateScope(IsolationLevel.RepeatableRead)) { - database.AcquireLockNodeWriteLock(2); - Thread.Sleep(1000); - database.AcquireLockNodeWriteLock(1); - Thread.Sleep(1000); - } - catch (Exception e) - { - e2 = e; - } - finally - { - database.CompleteTransaction(); + var database = scope.Database; + Console.WriteLine("Thread 2 db " + database.InstanceSid); + try + { + database.AcquireLockNodeWriteLock(2); + Thread.Sleep(100); + database.AcquireLockNodeWriteLock(1); + Thread.Sleep(1000); + } + catch (Exception e) + { + e2 = e; + } + scope.Complete(); } }); thread1.Start(); thread2.Start(); thread1.Join(); thread2.Join(); - Assert.IsNotNull(e1); - Assert.IsNotNull(e2); - Assert.IsInstanceOf(e1); - Assert.IsInstanceOf(e2); + var oneIsNotNull = e1 != null || e2 != null; + Assert.IsTrue(oneIsNotNull); + if (e1 != null) + Assert.IsInstanceOf(e1); + if (e2 != null) + Assert.IsInstanceOf(e2); } [Test] @@ -248,51 +206,51 @@ namespace Umbraco.Tests.Persistence var thread1 = new Thread(() => { - var database = DatabaseContext.Database; - database.BeginTransaction(IsolationLevel.RepeatableRead); - try + // each thread gets its own scope, because it has its own LCC, hence its own database + using (var scope = ApplicationContext.ScopeProvider.CreateScope(IsolationLevel.RepeatableRead)) { - database.AcquireLockNodeWriteLock(1); - var info = database.Query("SELECT * FROM sys.lock_information;"); - Console.WriteLine("LOCKS:"); - foreach (var row in info) - Console.WriteLine(string.Format("> {0} {1} {2} {3} {4} {5} {6}", row.request_spid, - row.resource_type, row.resource_description, row.request_mode, row.resource_table, - row.resource_table_id, row.request_status)); - Thread.Sleep(6000); - } - catch (Exception e) - { - e1 = e; - } - finally - { - database.CompleteTransaction(); + var database = scope.Database; + try + { + database.AcquireLockNodeWriteLock(1); + var info = database.Query("SELECT * FROM sys.lock_information;"); + Console.WriteLine("LOCKS:"); + foreach (var row in info) + Console.WriteLine(string.Format("> {0} {1} {2} {3} {4} {5} {6}", row.request_spid, + row.resource_type, row.resource_description, row.request_mode, row.resource_table, + row.resource_table_id, row.request_status)); + Thread.Sleep(6000); + } + catch (Exception e) + { + e1 = e; + } + scope.Complete(); } }); var thread2 = new Thread(() => { - var database = DatabaseContext.Database; - database.BeginTransaction(IsolationLevel.RepeatableRead); - try + // each thread gets its own scope, because it has its own LCC, hence its own database + using (var scope = ApplicationContext.ScopeProvider.CreateScope(IsolationLevel.RepeatableRead)) { - Thread.Sleep(1000); - database.AcquireLockNodeWriteLock(2); - var info = database.Query("SELECT * FROM sys.lock_information;"); - Console.WriteLine("LOCKS:"); - foreach (var row in info) - Console.WriteLine(string.Format("> {0} {1} {2} {3} {4} {5} {6}", row.request_spid, - row.resource_type, row.resource_description, row.request_mode, row.resource_table, - row.resource_table_id, row.request_status)); - Thread.Sleep(1000); - } - catch (Exception e) - { - e2 = e; - } - finally - { - database.CompleteTransaction(); + var database = scope.Database; + try + { + Thread.Sleep(1000); + database.AcquireLockNodeWriteLock(2); + var info = database.Query("SELECT * FROM sys.lock_information;"); + Console.WriteLine("LOCKS:"); + foreach (var row in info) + Console.WriteLine(string.Format("> {0} {1} {2} {3} {4} {5} {6}", row.request_spid, + row.resource_type, row.resource_description, row.request_mode, row.resource_table, + row.resource_table_id, row.request_status)); + Thread.Sleep(1000); + } + catch (Exception e) + { + e2 = e; + } + scope.Complete(); } }); thread1.Start(); diff --git a/src/Umbraco.Tests/Persistence/Repositories/LockedRepositoryTests.cs b/src/Umbraco.Tests/Persistence/Repositories/LockedRepositoryTests.cs new file mode 100644 index 0000000000..7307be9d52 --- /dev/null +++ b/src/Umbraco.Tests/Persistence/Repositories/LockedRepositoryTests.cs @@ -0,0 +1,149 @@ +using System; +using System.Data; +using System.Linq; +using NUnit.Framework; +using Umbraco.Core; +using Umbraco.Core.Cache; +using Umbraco.Core.Logging; +using Umbraco.Core.Models; +using Umbraco.Core.Persistence; +using Umbraco.Core.Persistence.Repositories; +using Umbraco.Core.Persistence.SqlSyntax; +using Umbraco.Core.Persistence.UnitOfWork; +using Umbraco.Core.Scoping; +using Umbraco.Tests.TestHelpers; + +namespace Umbraco.Tests.Persistence.Repositories +{ + [DatabaseTestBehavior(DatabaseBehavior.NewDbFileAndSchemaPerTest)] + [TestFixture] + public class LockedRepositoryTests : BaseDatabaseFactoryTest + { + private static IServerRegistrationRepository CreateRepository(IDatabaseUnitOfWork uow, ILogger logger, CacheHelper cacheHelper, ISqlSyntaxProvider sqlSyntax) + { + return new ServerRegistrationRepository( + uow, + cacheHelper.StaticCache, + logger, sqlSyntax); + } + + protected override CacheHelper CreateCacheHelper() + { + // ServerRegistrationRepository wants a real cache else weird things happen + //return CacheHelper.CreateDisabledCacheHelper(); + return new CacheHelper( + new ObjectCacheRuntimeCacheProvider(), + new StaticCacheProvider(), + new NullCacheProvider(), + new IsolatedRuntimeCache(type => new ObjectCacheRuntimeCacheProvider())); + } + + [Test] + public void NoOuterScopeJustWorks() + { + var uowProvider = new PetaPocoUnitOfWorkProvider(Logger); + var sqlSyntax = ApplicationContext.DatabaseContext.SqlSyntax; + + var lrepo = new LockingRepository(uowProvider, + x => CreateRepository(x, Logger, CacheHelper, sqlSyntax), + new[] { Constants.Locks.Servers }, new[] { Constants.Locks.Servers }); + + IServerRegistration reg = null; + + lrepo.WithWriteLocked(xrepo => + { + xrepo.Repository.AddOrUpdate(reg = new ServerRegistration("a1234", "i1234", DateTime.Now)); + + // no need - autocommit by default + //xrepo.UnitOfWork.Commit(); + }); + + Assert.IsNull(((ScopeProvider) ApplicationContext.ScopeProvider).AmbientScope); + + Assert.AreNotEqual(0, reg.Id); + + // that's cheating somehow because it will not really hit the DB because of the cache + var reg2 = lrepo.WithReadLocked(xrepo => + { + return xrepo.Repository.Get(reg.Id); + }); + + Assert.IsNull(((ScopeProvider) ApplicationContext.ScopeProvider).AmbientScope); + + Assert.IsNotNull(reg2); + Assert.AreEqual("a1234", reg2.ServerAddress); + Assert.AreEqual("i1234", reg2.ServerIdentity); + + // this really makes sure there's something in database + using (var scope = ApplicationContext.ScopeProvider.CreateScope()) + { + var reg3 = scope.Database.Fetch("SELECT * FROM umbracoServer WHERE id=@id", new { id = reg2.Id }).FirstOrDefault(); + Assert.IsNotNull(reg3); + Assert.AreEqual("a1234", reg3.address); + Assert.AreEqual("i1234", reg3.computerName); + } + + Assert.IsNull(((ScopeProvider) ApplicationContext.ScopeProvider).AmbientScope); + } + + [Test] + public void OuterScopeBadIsolationLevel() + { + var uowProvider = new PetaPocoUnitOfWorkProvider(Logger); + var sqlSyntax = ApplicationContext.DatabaseContext.SqlSyntax; + + var lrepo = new LockingRepository(uowProvider, + x => CreateRepository(x, Logger, CacheHelper, sqlSyntax), + new[] { Constants.Locks.Servers }, new[] { Constants.Locks.Servers }); + + // this creates a IsolationLevel.Unspecified scope + using (var scope = ApplicationContext.ScopeProvider.CreateScope()) + { + // so outer scope creates IsolationLevel.ReadCommitted (default) transaction + // then WithReadLocked creates a IsolationLevel.RepeatableRead scope, which + // fails - levels conflict + + try + { + lrepo.WithReadLocked(xrepo => + { + xrepo.Repository.DeactiveStaleServers(TimeSpan.Zero); + }); + Assert.Fail("Expected: Exception."); + } + catch (Exception e) + { + Assert.AreEqual("Scope requires isolation level RepeatableRead, but got ReadCommitted from parent.", e.Message); + } + + scope.Complete(); + } + } + + [Test] + public void OuterScopeGoodIsolationLevel() + { + var uowProvider = new PetaPocoUnitOfWorkProvider(Logger); + var sqlSyntax = ApplicationContext.DatabaseContext.SqlSyntax; + + var lrepo = new LockingRepository(uowProvider, + x => CreateRepository(x, Logger, CacheHelper, sqlSyntax), + new[] { Constants.Locks.Servers }, new[] { Constants.Locks.Servers }); + + // this creates a IsolationLevel.RepeatableRead scope + using (var scope = ApplicationContext.ScopeProvider.CreateScope(IsolationLevel.RepeatableRead)) + { + // so outer scope creates IsolationLevel.RepeatableRead transaction + // then WithReadLocked creates a IsolationLevel.RepeatableRead scope, which + // suceeds - no level conflict + + lrepo.WithReadLocked(xrepo => + { + xrepo.Repository.DeactiveStaleServers(TimeSpan.Zero); + }); + + scope.Complete(); + } + } + } +} diff --git a/src/Umbraco.Tests/Scoping/LeakTests.cs b/src/Umbraco.Tests/Scoping/LeakTests.cs index 94d1f2a295..ae8da89b06 100644 --- a/src/Umbraco.Tests/Scoping/LeakTests.cs +++ b/src/Umbraco.Tests/Scoping/LeakTests.cs @@ -24,11 +24,6 @@ namespace Umbraco.Tests.Scoping { base.Initialize(); - //// initialization leaves a NoScope around, remove it - //var scope = DatabaseContext.ScopeProvider.AmbientScope; - //Assert.IsNotNull(scope); - //Assert.IsInstanceOf(scope); - //scope.Dispose(); Assert.IsNull(DatabaseContext.ScopeProvider.AmbientScope); // gone } diff --git a/src/Umbraco.Tests/Scoping/ScopeTests.cs b/src/Umbraco.Tests/Scoping/ScopeTests.cs index 6de011c28c..249ecd2081 100644 --- a/src/Umbraco.Tests/Scoping/ScopeTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeTests.cs @@ -16,11 +16,6 @@ namespace Umbraco.Tests.Scoping { base.Initialize(); - //// initialization leaves a NoScope around, remove it - //var scope = DatabaseContext.ScopeProvider.AmbientScope; - //Assert.IsNotNull(scope); - //Assert.IsInstanceOf(scope); - //scope.Dispose(); Assert.IsNull(DatabaseContext.ScopeProvider.AmbientScope); // gone } diff --git a/src/Umbraco.Tests/Services/ThreadSafetyServiceTest.cs b/src/Umbraco.Tests/Services/ThreadSafetyServiceTest.cs index 2a67a8194c..bd6a9df19b 100644 --- a/src/Umbraco.Tests/Services/ThreadSafetyServiceTest.cs +++ b/src/Umbraco.Tests/Services/ThreadSafetyServiceTest.cs @@ -1,25 +1,14 @@ using System; -using System.Collections.Concurrent; using System.Collections.Generic; using System.Diagnostics; using System.Linq; using System.Threading; using NUnit.Framework; using Umbraco.Core; -using Umbraco.Core.Cache; -using Umbraco.Core.Logging; using Umbraco.Core.Models; -using Umbraco.Core.Persistence; -using Umbraco.Core.Persistence.SqlSyntax; -using Umbraco.Core.Persistence.UnitOfWork; -using Umbraco.Core.Publishing; using Umbraco.Core.Services; using Umbraco.Tests.TestHelpers; using Umbraco.Tests.TestHelpers.Entities; -using umbraco.editorControls.tinyMCE3; -using umbraco.interfaces; -using Umbraco.Core.Events; -using Umbraco.Core.Scoping; namespace Umbraco.Tests.Services { @@ -27,40 +16,10 @@ namespace Umbraco.Tests.Services [TestFixture, RequiresSTA] public class ThreadSafetyServiceTest : BaseDatabaseFactoryTest { - private PerThreadPetaPocoUnitOfWorkProvider _uowProvider; - private PerThreadDatabaseFactory _dbFactory; - [SetUp] public override void Initialize() - { + { base.Initialize(); - - //we need to use our own custom IDatabaseFactory for the DatabaseContext because we MUST ensure that - //a Database instance is created per thread, whereas the default implementation which will work in an HttpContext - //threading environment, or a single apartment threading environment will not work for this test because - //it is multi-threaded. - _dbFactory = new PerThreadDatabaseFactory(Logger); - var scopeProvider = new ScopeProvider(_dbFactory); - //overwrite the local object - ApplicationContext.DatabaseContext = new DatabaseContext(scopeProvider, Logger, new SqlCeSyntaxProvider(), Constants.DatabaseProviders.SqlCe); - - //disable cache - var cacheHelper = CacheHelper.CreateDisabledCacheHelper(); - - //here we are going to override the ServiceContext because normally with our test cases we use a - //global Database object but this is NOT how it should work in the web world or in any multi threaded scenario. - //we need a new Database object for each thread. - var repositoryFactory = new RepositoryFactory(cacheHelper, Logger, SqlSyntax, SettingsForTests.GenerateMockSettings()); - _uowProvider = new PerThreadPetaPocoUnitOfWorkProvider(_dbFactory); - var evtMsgs = new TransientMessagesFactory(); - ApplicationContext.Services = new ServiceContext( - repositoryFactory, - _uowProvider, - new FileUnitOfWorkProvider(), - new PublishingStrategy(evtMsgs, Logger), - cacheHelper, - Logger, - evtMsgs); CreateTestData(); } @@ -70,10 +29,6 @@ namespace Umbraco.Tests.Services { _error = null; - //dispose! - _dbFactory.Dispose(); - _uowProvider.Dispose(); - base.TearDown(); } @@ -89,10 +44,10 @@ namespace Umbraco.Tests.Services { //we will mimick the ServiceContext in that each repository in a service (i.e. ContentService) is a singleton var contentService = (ContentService)ServiceContext.ContentService; - + var threads = new List(); - - Debug.WriteLine("Starting test..."); + + Debug.WriteLine("Starting..."); for (var i = 0; i < MaxThreadCount; i++) { @@ -100,27 +55,33 @@ namespace Umbraco.Tests.Services { try { - Debug.WriteLine("Created content on thread: " + Thread.CurrentThread.ManagedThreadId); - - //create 2 content items + Debug.WriteLine("[{0}] Running...", Thread.CurrentThread.ManagedThreadId); + + using (var scope = ApplicationContext.Current.ScopeProvider.CreateScope()) + { + var database = scope.Database; + Debug.WriteLine("[{0}] Database {1}.", Thread.CurrentThread.ManagedThreadId, database.InstanceSid); + } + + //create 2 content items string name1 = "test" + Guid.NewGuid(); var content1 = contentService.CreateContent(name1, -1, "umbTextpage", 0); - - Debug.WriteLine("Saving content1 on thread: " + Thread.CurrentThread.ManagedThreadId); + + Debug.WriteLine("[{0}] Saving content #1.", Thread.CurrentThread.ManagedThreadId); contentService.Save(content1); Thread.Sleep(100); //quick pause for maximum overlap! string name2 = "test" + Guid.NewGuid(); var content2 = contentService.CreateContent(name2, -1, "umbTextpage", 0); - Debug.WriteLine("Saving content2 on thread: " + Thread.CurrentThread.ManagedThreadId); - contentService.Save(content2); + Debug.WriteLine("[{0}] Saving content #2.", Thread.CurrentThread.ManagedThreadId); + contentService.Save(content2); } catch(Exception e) - { + { _error = e; - } + } }); threads.Add(t); } @@ -144,7 +105,7 @@ namespace Umbraco.Tests.Services { throw new Exception("Error!", _error); } - + } [Test] @@ -155,7 +116,7 @@ namespace Umbraco.Tests.Services var threads = new List(); - Debug.WriteLine("Starting test..."); + Debug.WriteLine("Starting..."); for (var i = 0; i < MaxThreadCount; i++) { @@ -163,21 +124,27 @@ namespace Umbraco.Tests.Services { try { - Debug.WriteLine("Created content on thread: " + Thread.CurrentThread.ManagedThreadId); + Debug.WriteLine("[{0}] Running...", Thread.CurrentThread.ManagedThreadId); - //create 2 content items + using (var scope = ApplicationContext.Current.ScopeProvider.CreateScope()) + { + var database = scope.Database; + Debug.WriteLine("[{0}] Database {1}.", Thread.CurrentThread.ManagedThreadId, database.InstanceSid); + } + + //create 2 content items string name1 = "test" + Guid.NewGuid(); var folder1 = mediaService.CreateMedia(name1, -1, Constants.Conventions.MediaTypes.Folder, 0); - Debug.WriteLine("Saving folder1 on thread: " + Thread.CurrentThread.ManagedThreadId); - mediaService.Save(folder1, 0); + Debug.WriteLine("[{0}] Saving content #1.", Thread.CurrentThread.ManagedThreadId); + mediaService.Save(folder1, 0); Thread.Sleep(100); //quick pause for maximum overlap! string name = "test" + Guid.NewGuid(); var folder2 = mediaService.CreateMedia(name, -1, Constants.Conventions.MediaTypes.Folder, 0); - Debug.WriteLine("Saving folder2 on thread: " + Thread.CurrentThread.ManagedThreadId); - mediaService.Save(folder2, 0); + Debug.WriteLine("[{0}] Saving content #2.", Thread.CurrentThread.ManagedThreadId); + mediaService.Save(folder2, 0); } catch (Exception e) { @@ -208,69 +175,13 @@ namespace Umbraco.Tests.Services } } - + public void CreateTestData() { //Create and Save ContentType "umbTextpage" -> 1045 ContentType contentType = MockedContentTypes.CreateSimpleContentType("umbTextpage", "Textpage"); contentType.Key = new Guid("1D3A8E6E-2EA9-4CC1-B229-1AEE19821522"); - ServiceContext.ContentTypeService.Save(contentType); - } - - /// - /// Creates a Database object per thread, this mimics the web context which is per HttpContext and is required for the multi-threaded test - /// - internal class PerThreadDatabaseFactory : DisposableObject, IDatabaseFactory2 - { - private readonly ILogger _logger; - - public PerThreadDatabaseFactory(ILogger logger) - { - _logger = logger; - } - - private readonly ConcurrentDictionary _databases = new ConcurrentDictionary(); - - public UmbracoDatabase CreateDatabase() - { - var db = _databases.GetOrAdd( - Thread.CurrentThread.ManagedThreadId, - i => new UmbracoDatabase(Constants.System.UmbracoConnectionName, _logger)); - return db; - } - - public UmbracoDatabase CreateNewDatabase() - { - return new UmbracoDatabase(Constants.System.UmbracoConnectionName, _logger); - } - - protected override void DisposeResources() - { - //dispose the databases - _databases.ForEach(x => x.Value.Dispose()); - } - } - - /// - /// Creates a UOW with a Database object per thread - /// - internal class PerThreadPetaPocoUnitOfWorkProvider : DisposableObject, IDatabaseUnitOfWorkProvider - { - private readonly ScopeProvider _scopeProvider; - - public PerThreadPetaPocoUnitOfWorkProvider(PerThreadDatabaseFactory dbFactory) - { - _scopeProvider = new ScopeProvider(dbFactory); - } - - public IDatabaseUnitOfWork GetUnitOfWork() - { - //Create or get a database instance for this thread. - return new PetaPocoUnitOfWork(_scopeProvider); - } - - protected override void DisposeResources() - { } + ServiceContext.ContentTypeService.Save(contentType); } } } \ No newline at end of file diff --git a/src/Umbraco.Tests/Umbraco.Tests.csproj b/src/Umbraco.Tests/Umbraco.Tests.csproj index 8681e2f80f..bdfcac3420 100644 --- a/src/Umbraco.Tests/Umbraco.Tests.csproj +++ b/src/Umbraco.Tests/Umbraco.Tests.csproj @@ -165,6 +165,7 @@ + diff --git a/src/Umbraco.Web/Scheduling/LogScrubber.cs b/src/Umbraco.Web/Scheduling/LogScrubber.cs index 187d0c55a2..f18f5ad603 100644 --- a/src/Umbraco.Web/Scheduling/LogScrubber.cs +++ b/src/Umbraco.Web/Scheduling/LogScrubber.cs @@ -77,7 +77,8 @@ namespace Umbraco.Web.Scheduling return false; // do NOT repeat, going down } - // running on a background task, requires its own (safe) scope + // running on a background task, and Log.CleanLogs uses the old SqlHelper, + // better wrap in a scope and ensure it's all cleaned up and nothing leaks using (ApplicationContext.Current.ScopeProvider.CreateScope()) using (DisposableTimer.DebugDuration("Log scrubbing executing", "Log scrubbing complete")) { diff --git a/src/Umbraco.Web/Scheduling/ScheduledPublishing.cs b/src/Umbraco.Web/Scheduling/ScheduledPublishing.cs index 461abc984a..5ff7963f50 100644 --- a/src/Umbraco.Web/Scheduling/ScheduledPublishing.cs +++ b/src/Umbraco.Web/Scheduling/ScheduledPublishing.cs @@ -84,6 +84,7 @@ namespace Umbraco.Web.Scheduling // running on a background task, requires its own (safe) scope // (GetAuthenticationHeaderValue uses UserService to load the current user, hence requires a database) + // (might not need a scope but we don't know really) using (ApplicationContext.Current.ScopeProvider.CreateScope()) { //pass custom the authorization header diff --git a/src/Umbraco.Web/Strategies/ServerRegistrationEventHandler.cs b/src/Umbraco.Web/Strategies/ServerRegistrationEventHandler.cs index 720dc0b20e..a25deac1d9 100644 --- a/src/Umbraco.Web/Strategies/ServerRegistrationEventHandler.cs +++ b/src/Umbraco.Web/Strategies/ServerRegistrationEventHandler.cs @@ -137,11 +137,9 @@ namespace Umbraco.Web.Strategies { try { - // running on a background task, requires its own (safe) scope - using (ApplicationContext.Current.ScopeProvider.CreateScope()) - { - _svc.TouchServer(_serverAddress, _svc.CurrentServerIdentity, _registrar.Options.StaleServerTimeout); - } + // TouchServer uses a proper unit of work etc underneath so even in a + // background task it is safe to call it without dealing with any scope + _svc.TouchServer(_serverAddress, _svc.CurrentServerIdentity, _registrar.Options.StaleServerTimeout); return true; // repeat }