Merge pull request #10123 from umbraco/netcore/bugfix/only-process-cache-notification-once

Netcore: Only process each cache instruction from DatabaseServerMessenger once.

src/Umbraco.Core/Services/ICacheInstructionService.cs

@@ -39,6 +39,7 @@ namespace Umbraco.Cms.Core.Services
         /// <param name="released">Flag indicating if process is shutting now and operations should exit.</param>
         /// <param name="localIdentity">Local identity of the executing AppDomain.</param>
         /// <param name="lastPruned">Date of last prune operation.</param>
-        CacheInstructionServiceProcessInstructionsResult ProcessInstructions(bool released, string localIdentity, DateTime lastPruned);
+        /// <param name="lastId">Id of the latest processed instruction</param>
+        CacheInstructionServiceProcessInstructionsResult ProcessInstructions(bool released, string localIdentity, DateTime lastPruned, int lastId);
     }
 }

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));
 
         /// <inheritdoc/>
-        public CacheInstructionServiceProcessInstructionsResult ProcessInstructions(bool released, string localIdentity, DateTime lastPruned)
+        public CacheInstructionServiceProcessInstructionsResult ProcessInstructions(bool released, string localIdentity, DateTime lastPruned, int lastId)
         {
             using (_profilingLogger.DebugDuration<CacheInstructionService>("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.
         /// </remarks>
         /// <returns>Number of instructions processed.</returns>
-        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<CacheInstruction> 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.

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();
             }
-        }        
+        }
-        
+
         /// <summary>
         /// Reads the last-synced id from file into memory.
         /// </summary>

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<ICacheInstructionService>();
+            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<ICacheInstructionService>();
+            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<RefreshInstruction> 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<ICacheInstructionService>();
+            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<RefreshInstruction> 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++)