diff --git a/src/Umbraco.Core/Services/ICacheInstructionService.cs b/src/Umbraco.Core/Services/ICacheInstructionService.cs index cca80a0b0e..faf05f2237 100644 --- a/src/Umbraco.Core/Services/ICacheInstructionService.cs +++ b/src/Umbraco.Core/Services/ICacheInstructionService.cs @@ -39,6 +39,7 @@ namespace Umbraco.Cms.Core.Services /// Flag indicating if process is shutting now and operations should exit. /// Local identity of the executing AppDomain. /// Date of last prune operation. - CacheInstructionServiceProcessInstructionsResult ProcessInstructions(bool released, string localIdentity, DateTime lastPruned); + /// Id of the latest processed instruction + CacheInstructionServiceProcessInstructionsResult ProcessInstructions(bool released, string localIdentity, DateTime lastPruned, int lastId); } } diff --git a/src/Umbraco.Infrastructure/Services/Implement/CacheInstructionService.cs b/src/Umbraco.Infrastructure/Services/Implement/CacheInstructionService.cs index 0968c3897c..0a6e945a23 100644 --- a/src/Umbraco.Infrastructure/Services/Implement/CacheInstructionService.cs +++ b/src/Umbraco.Infrastructure/Services/Implement/CacheInstructionService.cs @@ -133,12 +133,12 @@ namespace Umbraco.Cms.Core.Services.Implement new CacheInstruction(0, DateTime.UtcNow, JsonConvert.SerializeObject(instructions, Formatting.None), localIdentity, instructions.Sum(x => x.JsonIdCount)); /// - public CacheInstructionServiceProcessInstructionsResult ProcessInstructions(bool released, string localIdentity, DateTime lastPruned) + public CacheInstructionServiceProcessInstructionsResult ProcessInstructions(bool released, string localIdentity, DateTime lastPruned, int lastId) { using (_profilingLogger.DebugDuration("Syncing from database...")) using (IScope scope = ScopeProvider.CreateScope()) { - var numberOfInstructionsProcessed = ProcessDatabaseInstructions(released, localIdentity, out int lastId); + var numberOfInstructionsProcessed = ProcessDatabaseInstructions(released, localIdentity, ref lastId); // Check for pruning throttling. if (released || (DateTime.UtcNow - lastPruned) <= _globalSettings.DatabaseServerMessenger.TimeBetweenPruneOperations) @@ -172,7 +172,7 @@ namespace Umbraco.Cms.Core.Services.Implement /// Thread safety: this is NOT thread safe. Because it is NOT meant to run multi-threaded. /// /// Number of instructions processed. - private int ProcessDatabaseInstructions(bool released, string localIdentity, out int lastId) + private int ProcessDatabaseInstructions(bool released, string localIdentity, ref int lastId) { // NOTE: // We 'could' recurse to ensure that no remaining instructions are pending in the table before proceeding but I don't think that @@ -199,8 +199,9 @@ namespace Umbraco.Cms.Core.Services.Implement // It would have been nice to do this in a Query instead of Fetch using a data reader to save // some memory however we cannot do that because inside of this loop the cache refreshers are also // performing some lookups which cannot be done with an active reader open. + IEnumerable pendingInstructions = _cacheInstructionRepository.GetPendingInstructions(lastId, MaxInstructionsToRetrieve); lastId = 0; - foreach (CacheInstruction instruction in _cacheInstructionRepository.GetPendingInstructions(lastId, MaxInstructionsToRetrieve)) + foreach (CacheInstruction instruction in pendingInstructions) { // If this flag gets set it means we're shutting down! In this case, we need to exit asap and cannot // continue processing anything otherwise we'll hold up the app domain shutdown. diff --git a/src/Umbraco.Infrastructure/Sync/DatabaseServerMessenger.cs b/src/Umbraco.Infrastructure/Sync/DatabaseServerMessenger.cs index 2bd66d4954..0b2076a3a7 100644 --- a/src/Umbraco.Infrastructure/Sync/DatabaseServerMessenger.cs +++ b/src/Umbraco.Infrastructure/Sync/DatabaseServerMessenger.cs @@ -286,7 +286,7 @@ namespace Umbraco.Cms.Infrastructure.Sync try { - CacheInstructionServiceProcessInstructionsResult result = CacheInstructionService.ProcessInstructions(_released, LocalIdentity, _lastPruned); + CacheInstructionServiceProcessInstructionsResult result = CacheInstructionService.ProcessInstructions(_released, LocalIdentity, _lastPruned, _lastId); if (result.InstructionsWerePruned) { _lastPruned = _lastSync; @@ -307,8 +307,8 @@ namespace Umbraco.Cms.Infrastructure.Sync _syncIdle.Set(); } - } - + } + /// /// Reads the last-synced id from file into memory. /// diff --git a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/CacheInstructionServiceTests.cs b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/CacheInstructionServiceTests.cs index 53496e32a6..08fe94b2e6 100644 --- a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/CacheInstructionServiceTests.cs +++ b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/CacheInstructionServiceTests.cs @@ -152,7 +152,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services // Create three instruction records, each with two instructions. First two records are for a different identity. CreateAndDeliveryMultipleInstructions(sut); - CacheInstructionServiceProcessInstructionsResult result = sut.ProcessInstructions(false, LocalIdentity, DateTime.UtcNow.AddSeconds(-1)); + CacheInstructionServiceProcessInstructionsResult result = sut.ProcessInstructions(false, LocalIdentity, DateTime.UtcNow.AddSeconds(-1), -1); Assert.Multiple(() => { @@ -171,7 +171,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services CreateAndDeliveryMultipleInstructions(sut); - CacheInstructionServiceProcessInstructionsResult result = sut.ProcessInstructions(false, LocalIdentity, DateTime.UtcNow.AddHours(-1)); + CacheInstructionServiceProcessInstructionsResult result = sut.ProcessInstructions(false, LocalIdentity, DateTime.UtcNow.AddHours(-1), -1); Assert.IsTrue(result.InstructionsWerePruned); } @@ -183,7 +183,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services CreateAndDeliveryMultipleInstructions(sut); - CacheInstructionServiceProcessInstructionsResult result = sut.ProcessInstructions(true, LocalIdentity, DateTime.UtcNow.AddSeconds(-1)); + CacheInstructionServiceProcessInstructionsResult result = sut.ProcessInstructions(true, LocalIdentity, DateTime.UtcNow.AddSeconds(-1), -1); Assert.Multiple(() => { @@ -193,6 +193,92 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services }); } + [Test] + public void Processes_Instructions_Only_Once() + { + // This test shows what's happening in issue #10112 + // The DatabaseServerMessenger will run its sync operation every five seconds which calls CacheInstructionService.ProcessInstructions, + // which is why the CacheRefresherNotification keeps dispatching, because the cache instructions gets constantly processed. + var sut = (CacheInstructionService)GetRequiredService(); + CreateAndDeliveryMultipleInstructions(sut); + + var lastId = -1; + // Run once + CacheInstructionServiceProcessInstructionsResult result = sut.ProcessInstructions(false, LocalIdentity, DateTime.UtcNow.AddSeconds(-1), lastId); + + Assert.Multiple(() => + { + Assert.AreEqual(3, result.LastId); // 3 records found. + Assert.AreEqual(2, result.NumberOfInstructionsProcessed); // 2 records processed (as one is for the same identity). + Assert.IsFalse(result.InstructionsWerePruned); + }); + + // DatabaseServerMessenger stores the LastID after ProcessInstructions has been run. + lastId = result.LastId; + + // The instructions has now been processed and shouldn't be processed on the next call... + // Run again. + CacheInstructionServiceProcessInstructionsResult secondResult = sut.ProcessInstructions(false, LocalIdentity, DateTime.UtcNow.AddSeconds(-1), lastId); + Assert.Multiple(() => + { + Assert.AreEqual(0, secondResult.LastId); // No instructions was processed so LastId is 0, this is consistent with behavior from V8 + Assert.AreEqual(0, secondResult.NumberOfInstructionsProcessed); // Nothing was processed. + Assert.IsFalse(secondResult.InstructionsWerePruned); + }); + } + + [Test] + public void Processes_New_Instructions() + { + var sut = (CacheInstructionService)GetRequiredService(); + CreateAndDeliveryMultipleInstructions(sut); + + var lastId = -1; + CacheInstructionServiceProcessInstructionsResult result = sut.ProcessInstructions(false, LocalIdentity, DateTime.UtcNow.AddSeconds(-1), lastId); + + Assert.AreEqual(3, result.LastId); // Make sure LastId is 3, the rest is tested in other test. + lastId = result.LastId; + + // Add new instruction + List instructions = CreateInstructions(); + sut.DeliverInstructions(instructions, AlternateIdentity); + + CacheInstructionServiceProcessInstructionsResult secondResult = sut.ProcessInstructions(false, LocalIdentity, DateTime.UtcNow.AddSeconds(-1), lastId); + + Assert.Multiple(() => + { + Assert.AreEqual(4, secondResult.LastId); + Assert.AreEqual(1, secondResult.NumberOfInstructionsProcessed); + Assert.IsFalse(secondResult.InstructionsWerePruned); + }); + } + + [Test] + public void Correct_ID_For_Instruction_With_Same_Identity() + { + var sut = (CacheInstructionService)GetRequiredService(); + CreateAndDeliveryMultipleInstructions(sut); + + var lastId = -1; + CacheInstructionServiceProcessInstructionsResult result = sut.ProcessInstructions(false, LocalIdentity, DateTime.UtcNow.AddSeconds(-1), lastId); + + Assert.AreEqual(3, result.LastId); // Make sure LastId is 3, the rest is tested in other test. + lastId = result.LastId; + + // Add new instruction + List instructions = CreateInstructions(); + sut.DeliverInstructions(instructions, LocalIdentity); + + CacheInstructionServiceProcessInstructionsResult secondResult = sut.ProcessInstructions(false, LocalIdentity, DateTime.UtcNow.AddSeconds(-1), lastId); + + Assert.Multiple(() => + { + Assert.AreEqual(4, secondResult.LastId); + Assert.AreEqual(0, secondResult.NumberOfInstructionsProcessed); + Assert.IsFalse(secondResult.InstructionsWerePruned); + }); + } + private void CreateAndDeliveryMultipleInstructions(CacheInstructionService sut) { for (int i = 0; i < 3; i++)