Bugfix: GetReminder throws exception when reminder don't exists #1167

src/OrleansRuntime/ReminderService/InMemoryRemindersTable.cs

@@ -25,7 +25,6 @@ The above copyright notice and this permission notice shall be included in all c
 using System.Collections.Generic;
 using System.Linq;
 
-
 namespace Orleans.Runtime.ReminderService
 {
     [Serializable]
@@ -41,8 +40,8 @@ internal class InMemoryRemindersTable
         // enable after adding updates ... even then, you will probably only need etags per row, not the whole
         // table version, as each read/insert/update should touch & depend on only one row at a time
         //internal TableVersion TableVersion;
-        
-        [NonSerialized] 
+
+        [NonSerialized]
         private readonly TraceLogger logger = TraceLogger.GetLogger("InMemoryReminderTable", TraceLogger.LoggerType.Runtime);
 
         public InMemoryRemindersTable()
@@ -54,10 +53,10 @@ public ReminderTableData ReadRows(GrainReference grainRef)
         {
             Dictionary<string, ReminderEntry> reminders;
             reminderTable.TryGetValue(grainRef, out reminders);
-            return reminders == null ? new ReminderTableData() : 
+            return reminders == null ? new ReminderTableData() :
                 new ReminderTableData(reminders.Values.ToList());
         }
-        
+
         /// <summary>
         /// Return all rows that have their GrainReference's.GetUniformHashCode() in the range (start, end]
         /// </summary>
@@ -68,18 +67,18 @@ public ReminderTableData ReadRows(uint begin, uint end)
         {
             var range = RangeFactory.CreateRange(begin, end);
             IEnumerable<GrainReference> keys = reminderTable.Keys.Where(range.InRange);
-      
+
             // is there a sleaker way of doing this in C#?
             var list = new List<ReminderEntry>();
             foreach (GrainReference k in keys)
                 list.AddRange(reminderTable[k].Values);
-            
-            if (logger.IsVerbose3) logger.Verbose3("Selected {0} out of {1} reminders from memory for {2}. List is: {3}{4}", list.Count, reminderTable.Count, range.ToString(), 
+
+            if (logger.IsVerbose3) logger.Verbose3("Selected {0} out of {1} reminders from memory for {2}. List is: {3}{4}", list.Count, reminderTable.Count, range.ToString(),
                 Environment.NewLine, Utils.EnumerableToString(list, e => e.ToString()));
 
             return new ReminderTableData(list);
         }
-        
+
         /// <summary>
         /// Return all rows that have their GrainReference's.GetUniformHashCode() in the range (start, end]
         /// </summary>
@@ -88,9 +87,21 @@ public ReminderTableData ReadRows(uint begin, uint end)
         /// <returns></returns>
         public ReminderEntry ReadRow(GrainReference grainRef, string reminderName)
         {
-            ReminderEntry r = reminderTable[grainRef][reminderName];
-            if (logger.IsVerbose3) logger.Verbose3("Read for grain {0} reminder {1} row {2}", grainRef, reminderName, r.ToString());
-            return r;
+            ReminderEntry result = null;
+            Dictionary<string, ReminderEntry> reminders;
+            if (reminderTable.TryGetValue(grainRef, out reminders))
+            {
+                reminders.TryGetValue(reminderName, out result);
+            }
+
+            if (logger.IsVerbose3)
+            {
+                if (result == null)
+                    logger.Verbose3("Reminder not found for grain {0} reminder {1} ", grainRef, reminderName);
+                else
+                    logger.Verbose3("Read for grain {0} reminder {1} row {2}", grainRef, reminderName, result.ToString());
+            }
+            return result;
         }
 
         public string UpsertRow(ReminderEntry entry)
@@ -111,7 +122,7 @@ public string UpsertRow(ReminderEntry entry)
             if (logger.IsVerbose3) logger.Verbose3("Upserted entry {0}, replaced {1}", entry, old);
             return entry.ETag;
         }
-        
+
         /// <summary>
         /// Remove a row from the table
         /// </summary>
@@ -124,7 +135,7 @@ public bool RemoveRow(GrainReference grainRef, string reminderName, string eTag)
             Dictionary<string, ReminderEntry> data = null;
             ReminderEntry e = null;
 
-            // assuming the calling grain executes one call at a time, so no need to lock 
+            // assuming the calling grain executes one call at a time, so no need to lock
             if (!reminderTable.TryGetValue(grainRef, out data))
             {
                 logger.Info("1");
@@ -160,7 +171,7 @@ public ReminderTableData ReadAll()
             foreach (GrainReference k in reminderTable.Keys)
             {
                 list.AddRange(reminderTable[k].Values);
-            }            
+            }
             return new ReminderTableData(list);
         }
 

src/OrleansRuntime/ReminderService/LocalReminderService.cs

@@ -59,10 +59,10 @@ private enum ReminderServiceStatus
         private readonly TimeSpan initTimeout;
 
         internal LocalReminderService(
-            SiloAddress addr, 
-            GrainId id, 
-            IConsistentRingProvider ring, 
-            OrleansTaskScheduler localScheduler, 
+            SiloAddress addr,
+            GrainId id,
+            IConsistentRingProvider ring,
+            OrleansTaskScheduler localScheduler,
             IReminderTable reminderTable,
             GlobalConfiguration config,
             TimeSpan initTimeout)
@@ -128,7 +128,7 @@ public Task Stop()
             }
             foreach (LocalReminderData r in localReminders.Values)
                 r.StopReminder(logger);
-            
+
             // for a graceful shutdown, also handover reminder responsibilities to new owner, and update the ReminderTable
             // currently, this is taken care of by periodically reading the reminder table
             return TaskDone.Done;
@@ -178,7 +178,7 @@ public async Task UnregisterReminder(IGrainReminder reminder)
 
             await DoResponsibilitySanityCheck(grainRef, "RemoveReminder");
 
-            // it may happen that we dont have this reminder locally ... even then, we attempt to remove the reminder from the reminder 
+            // it may happen that we dont have this reminder locally ... even then, we attempt to remove the reminder from the reminder
             // table ... the periodic mechanism will stop this reminder at any silo's LocalReminderService that might have this reminder locally
 
             // remove from persistent/memory store
@@ -209,7 +209,7 @@ public async Task<IGrainReminder> GetReminder(GrainReference grainRef, string re
         {
             if(logger.IsVerbose) logger.Verbose(ErrorCode.RS_GetReminder,"GetReminder: GrainReference={0} ReminderName={1}", grainRef.ToDetailedString(), reminderName);
             var entry = await reminderTable.ReadRow(grainRef, reminderName);
-            return entry.ToIGrainReminder();
+            return entry == null ? null : entry.ToIGrainReminder();
         }
 
         public async Task<List<IGrainReminder>> GetReminders(GrainReference grainRef)
@@ -278,7 +278,7 @@ private async Task ReadTableAndStartTimers(IRingRange range)
                 ReminderTableData table = await reminderTable.ReadRows(srange.Begin, srange.End); // get all reminders, even the ones we already have
 
                 // if null is a valid value, it means that there's nothing to do.
-                if (null == table && reminderTable is MockReminderTable) return; 
+                if (null == table && reminderTable is MockReminderTable) return;
 
                 var remindersNotInTable = new Dictionary<ReminderIdentity, LocalReminderData>(localReminders); // shallow copy
                 if (logger.IsVerbose) logger.Verbose("For range {0}, I read in {1} reminders from table. LocalTableSequence {2}, CachedSequence {3}", range.ToString(), table.Reminders.Count, localTableSequence, cachedSequence);
@@ -411,7 +411,7 @@ private async Task AsyncTimerCallback(object rem)
         {
             var reminder = (LocalReminderData)rem;
 
-            if (!localReminders.ContainsKey(reminder.Identity) // we have already stopped this timer 
+            if (!localReminders.ContainsKey(reminder.Identity) // we have already stopped this timer
                 || reminder.Timer == null) // this timer was unregistered, and is waiting to be gc-ied
                 return;
 
@@ -425,7 +425,7 @@ private async Task DoResponsibilitySanityCheck(GrainReference grainRef, string d
         {
             if (status != ReminderServiceStatus.Started)
                 await startedTask.Task;
-            
+
             if (!myRange.InRange(grainRef))
             {
                 logger.Warn(ErrorCode.RS_NotResponsible, "I shouldn't have received request '{0}' for {1}. It is not in my responsibility range: {2}",
@@ -455,12 +455,12 @@ private class LocalReminderData
             private readonly TimeSpan period;
             private GrainReference GrainRef {  get { return Identity.GrainRef; } }
             private string ReminderName { get { return Identity.ReminderName; } }
-            
+
             internal ReminderIdentity Identity { get; private set; }
             internal string ETag;
             internal GrainTimer Timer;
             internal long LocalSequenceNumber; // locally, we use this for resolving races between the periodic table reader, and any concurrent local register/unregister requests
-            
+
             internal LocalReminderData(ReminderEntry entry)
             {
                 Identity = new ReminderIdentity(entry.GrainRef, entry.ReminderName);
@@ -484,7 +484,7 @@ public void StopReminder(TraceLogger logger)
             {
                 if (Timer != null)
                     Timer.Dispose();
-                
+
                 Timer = null;
             }
 
@@ -536,8 +536,8 @@ public async Task OnTimerTick(AverageTimeSpanStatistic tardinessStat, TraceLogge
                     stopwatch.Restart();
 
                     var after = DateTime.UtcNow;
-                    if (logger.IsVerbose2) 
-                        logger.Verbose2("Tick triggered for {0}, dt {1} sec, next@~ {2}", this.ToString(), (after - before).TotalSeconds, 
+                    if (logger.IsVerbose2)
+                        logger.Verbose2("Tick triggered for {0}, dt {1} sec, next@~ {2}", this.ToString(), (after - before).TotalSeconds,
                             // the next tick isn't actually scheduled until we return control to
                             // AsyncSafeTimer but we can approximate it by adding the period of the reminder
                             // to the after time.
@@ -547,7 +547,7 @@ public async Task OnTimerTick(AverageTimeSpanStatistic tardinessStat, TraceLogge
                 {
                     var after = DateTime.UtcNow;
                     logger.Error(
-                        ErrorCode.RS_Tick_Delivery_Error, 
+                        ErrorCode.RS_Tick_Delivery_Error,
                         String.Format("Could not deliver reminder tick for {0}, next {1}.",  this.ToString(), after + period),
                             exc);
                     // What to do with repeated failures to deliver a reminder's ticks?

src/TestGrainInterfaces/IReminderTestGrain.cs

@@ -0,0 +1,10 @@
+using Orleans;
+using System.Threading.Tasks;
+
+namespace UnitTests.GrainInterfaces
+{
+    public interface IReminderTestGrain : IGrainWithIntegerKey
+    {
+        Task<bool> IsReminderExists(string reminderName);
+    }
+}

src/TestGrainInterfaces/TestGrainInterfaces.csproj

@@ -60,6 +60,7 @@
     <Compile Include="GrainInterfaceHierarchyIGrains.cs" />
     <Compile Include="IProxyGrain.cs" />
     <Compile Include="IReentrantStressTestGrain.cs" />
+    <Compile Include="IReminderTestGrain.cs" />
     <Compile Include="ISampleStreamingGrain.cs" />
     <Compile Include="ISimpleDIGrain.cs" />
     <Compile Include="ISimpleGenericGrain.cs" />

src/TestGrains/ReminderTestGrain.cs

@@ -0,0 +1,15 @@
+using Orleans;
+using System.Threading.Tasks;
+using UnitTests.GrainInterfaces;
+
+namespace UnitTests.Grains
+{
+    internal class ReminderTestGrain : Grain, IReminderTestGrain
+    {
+        public async Task<bool> IsReminderExists(string reminderName)
+        {
+            var reminder = await this.GetReminder(reminderName);
+            return reminder != null;
+        }
+    }
+}

src/TestGrains/TestGrains.csproj

@@ -68,6 +68,7 @@
     <Compile Include="EventSourcing\JournaledPerson_Events.cs" />
     <Compile Include="MyObserverSubscriptionManager.cs" />
     <Compile Include="ProxyGrain.cs" />
+    <Compile Include="ReminderTestGrain.cs" />
     <Compile Include="SerializationGenerationGrain.cs" />
     <Compile Include="SimpleGenericGrain.cs" />
     <Compile Include="MultipleSubscriptionConsumerGrain.cs" />

src/Tester/ReminderTest.cs

@@ -0,0 +1,31 @@
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using Orleans.TestingHost;
+using System.Threading.Tasks;
+using UnitTests.GrainInterfaces;
+using UnitTests.Tester;
+
+namespace Tester
+{
+    [TestClass]
+    public class ReminderTest : UnitTestSiloHost
+    {
+        public ReminderTest()
+            : base(new TestingSiloOptions { StartPrimary = true, StartSecondary = false })
+        {
+        }
+
+        [ClassCleanup]
+        public static void MyClassCleanup()
+        {
+            StopAllSilos();
+        }
+
+        [TestMethod, TestCategory("BVT"), TestCategory("Functional"), TestCategory("Reminders")]
+        public async Task SimpleGrainGetGrain()
+        {
+            IReminderTestGrain grain = GrainFactory.GetGrain<IReminderTestGrain>(0);
+            bool notExists = await grain.IsReminderExists("not exists");
+            Assert.IsFalse(notExists);
+        }
+    }
+}

src/Tester/Tester.csproj

@@ -123,6 +123,7 @@
     <Compile Include="CodeGenTests\RuntimeCodeGenerationTests.cs" />
     <Compile Include="ConcreteStateClassTests.cs" />
     <Compile Include="ConfigurationTests\SerializationProviderTests.cs" />
+    <Compile Include="ReminderTest.cs" />
     <Compile Include="DeactivationTests.cs" />
     <Compile Include="JsonFallbackSerializationTests.cs" />
     <Compile Include="GenericGrainTests.cs" />