From 3927a8db670d51f396740d835f473c8b4b6ffc78 Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 14 Dec 2020 12:04:19 +1100 Subject: [PATCH] Ensure we capture exceptions that occur when trying to create a transaction --- src/Umbraco.Core/Runtime/MainDom.cs | 12 ++- src/Umbraco.Core/Runtime/SqlMainDomLock.cs | 96 +++++++++++++++------- 2 files changed, 76 insertions(+), 32 deletions(-) diff --git a/src/Umbraco.Core/Runtime/MainDom.cs b/src/Umbraco.Core/Runtime/MainDom.cs index e6780ec876b4..02f37f654ea4 100644 --- a/src/Umbraco.Core/Runtime/MainDom.cs +++ b/src/Umbraco.Core/Runtime/MainDom.cs @@ -144,8 +144,16 @@ private bool Acquire() _logger.Info("Acquiring."); - // Get the lock - var acquired = _mainDomLock.AcquireLockAsync(LockTimeoutMilliseconds).GetAwaiter().GetResult(); + // Get the lock + var acquired = false; + try + { + acquired = _mainDomLock.AcquireLockAsync(LockTimeoutMilliseconds).GetAwaiter().GetResult(); + } + catch (Exception ex) + { + _logger.Error(ex, "Error while acquiring"); + } if (!acquired) { diff --git a/src/Umbraco.Core/Runtime/SqlMainDomLock.cs b/src/Umbraco.Core/Runtime/SqlMainDomLock.cs index 209b99aef1a6..84d98775d959 100644 --- a/src/Umbraco.Core/Runtime/SqlMainDomLock.cs +++ b/src/Umbraco.Core/Runtime/SqlMainDomLock.cs @@ -1,4 +1,5 @@ -using System; +using NPoco; +using System; using System.Data; using System.Data.SqlClient; using System.Diagnostics; @@ -48,7 +49,9 @@ public async Task AcquireLockAsync(int millisecondsTimeout) } if (!(_dbFactory.SqlContext.SqlSyntax is SqlServerSyntaxProvider sqlServerSyntaxProvider)) + { throw new NotSupportedException("SqlMainDomLock is only supported for Sql Server"); + } _sqlServerSyntax = sqlServerSyntaxProvider; @@ -56,11 +59,13 @@ public async Task AcquireLockAsync(int millisecondsTimeout) var tempId = Guid.NewGuid().ToString(); - using var db = _dbFactory.CreateDatabase(); - using var transaction = db.GetTransaction(IsolationLevel.ReadCommitted); + IUmbracoDatabase db = null; try { + db = _dbFactory.CreateDatabase(); + db.BeginTransaction(IsolationLevel.ReadCommitted); + try { // wait to get a write lock @@ -101,7 +106,8 @@ public async Task AcquireLockAsync(int millisecondsTimeout) } finally { - transaction.Complete(); + db?.CompleteTransaction(); + db?.Dispose(); } @@ -154,11 +160,11 @@ private void ListeningLoop() // new MainDom will just take over. if (_cancellationTokenSource.IsCancellationRequested) return; - - using var db = _dbFactory.CreateDatabase(); - using var transaction = db.GetTransaction(IsolationLevel.ReadCommitted); + IUmbracoDatabase db = null; try { + db = _dbFactory.CreateDatabase(); + db.BeginTransaction(IsolationLevel.ReadCommitted); // get a read lock _sqlServerSyntax.ReadLock(db, Constants.Locks.MainDom); @@ -182,7 +188,8 @@ private void ListeningLoop() } finally { - transaction.Complete(); + db?.CompleteTransaction(); + db?.Dispose(); } } @@ -201,34 +208,47 @@ private Task WaitForExistingAsync(string tempId, int millisecondsTimeout) return Task.Run(() => { - using var db = _dbFactory.CreateDatabase(); - - var watch = new Stopwatch(); - watch.Start(); - while (true) + try { - // poll very often, we need to take over as fast as we can - // local testing shows the actual query to be executed from client/server is approx 300ms but would change depending on environment/IO - Thread.Sleep(1000); - - var acquired = TryAcquire(db, tempId, updatedTempId); - if (acquired.HasValue) - return acquired.Value; + using var db = _dbFactory.CreateDatabase(); - if (watch.ElapsedMilliseconds >= millisecondsTimeout) + var watch = new Stopwatch(); + watch.Start(); + while (true) { - return AcquireWhenMaxWaitTimeElapsed(db); + // poll very often, we need to take over as fast as we can + // local testing shows the actual query to be executed from client/server is approx 300ms but would change depending on environment/IO + Thread.Sleep(1000); + + var acquired = TryAcquire(db, tempId, updatedTempId); + if (acquired.HasValue) + return acquired.Value; + + if (watch.ElapsedMilliseconds >= millisecondsTimeout) + { + return AcquireWhenMaxWaitTimeElapsed(db); + } } } + catch (Exception ex) + { + _logger.Error(ex, "An error occurred trying to acquire and waiting for existing SqlMainDomLock to shutdown"); + return false; + } + }, _cancellationTokenSource.Token); } private bool? TryAcquire(IUmbracoDatabase db, string tempId, string updatedTempId) { - using var transaction = db.GetTransaction(IsolationLevel.ReadCommitted); + // Creates a separate transaction to the DB instance so we aren't allocating tons of new DB instances for each transaction + // since this is executed in a tight loop + + ITransaction transaction = null; try { + transaction = db.GetTransaction(IsolationLevel.ReadCommitted); // get a read lock _sqlServerSyntax.ReadLock(db, Constants.Locks.MainDom); @@ -274,7 +294,8 @@ private Task WaitForExistingAsync(string tempId, int millisecondsTimeout) } finally { - transaction.Complete(); + transaction?.Complete(); + transaction?.Dispose(); } return null; // continue @@ -282,6 +303,9 @@ private Task WaitForExistingAsync(string tempId, int millisecondsTimeout) private bool AcquireWhenMaxWaitTimeElapsed(IUmbracoDatabase db) { + // Creates a separate transaction to the DB instance so we aren't allocating tons of new DB instances for each transaction + // since this is executed in a tight loop + // if the timeout has elapsed, it either means that the other main dom is taking too long to shutdown, // or it could mean that the previous appdomain was terminated and didn't clear out the main dom SQL row // and it's just been left as an orphan row. @@ -291,10 +315,12 @@ private bool AcquireWhenMaxWaitTimeElapsed(IUmbracoDatabase db) _logger.Debug("Timeout elapsed, assuming orphan row, acquiring MainDom."); - using var transaction = db.GetTransaction(IsolationLevel.ReadCommitted); + ITransaction transaction = null; try { + transaction = db.GetTransaction(IsolationLevel.ReadCommitted); + _sqlServerSyntax.WriteLock(db, Constants.Locks.MainDom); // so now we update the row with our appdomain id @@ -317,7 +343,8 @@ private bool AcquireWhenMaxWaitTimeElapsed(IUmbracoDatabase db) } finally { - transaction.Complete(); + transaction?.Complete(); + transaction?.Dispose(); } } @@ -368,11 +395,12 @@ protected virtual void Dispose(bool disposing) if (_dbFactory.Configured) { - using var db = _dbFactory.CreateDatabase(); - using var transaction = db.GetTransaction(IsolationLevel.ReadCommitted); - + IUmbracoDatabase db = null; try { + db = _dbFactory.CreateDatabase(); + db.BeginTransaction(IsolationLevel.ReadCommitted); + // get a write lock _sqlServerSyntax.WriteLock(db, Constants.Locks.MainDom); @@ -399,7 +427,15 @@ protected virtual void Dispose(bool disposing) } finally { - transaction.Complete(); + try + { + db?.CompleteTransaction(); + db?.Dispose(); + } + catch (Exception ex) + { + _logger.Error(ex, "Unexpected error during dispose when completing transaction."); + } } } }