changed termination of periodic timer to be callback based

Author: dvirsky

rmutil/periodic.c

@@ -5,6 +5,7 @@
 
 typedef struct RMUtilTimer {
   RMutilTimerFunc cb;
+  RMUtilTimerTerminationFunc onTerm;
   void *privdata;
   struct timespec interval;
   pthread_t thread;
@@ -45,16 +46,34 @@ static void *rmutilTimer_Loop(void *ctx) {
       // It's up to the user to decide whether automemory is active there
       if (rctx) RedisModule_FreeThreadSafeContext(rctx);
     }
+    if (rc == EINVAL) {
+      perror("Error waiting for condition");
+      break;
+    }
+  }
+
+  // call the termination callback if needed
+  if (tm->onTerm != NULL) {
+    tm->onTerm(tm->privdata);
   }
-  //  RedisModule_Log(tm->redisCtx, "notice", "Timer cancelled");
+
+  // free resources associated with the timer
+  pthread_cond_destroy(&tm->cond);
+  free(tm);
 
   return NULL;
 }
 
-RMUtilTimer *RMUtil_NewPeriodicTimer(RMutilTimerFunc cb, void *privdata, struct timespec interval) {
+/* set a new frequency for the timer. This will take effect AFTER the next trigger */
+void RMUtilTimer_SetInterval(struct RMUtilTimer *t, struct timespec newInterval) {
+  t->interval = newInterval;
+}
+
+RMUtilTimer *RMUtil_NewPeriodicTimer(RMutilTimerFunc cb, RMUtilTimerTerminationFunc onTerm,
+                                     void *privdata, struct timespec interval) {
   RMUtilTimer *ret = malloc(sizeof(*ret));
   *ret = (RMUtilTimer){
-      .privdata = privdata, .interval = interval, .cb = cb,
+      .privdata = privdata, .interval = interval, .cb = cb, .onTerm = onTerm,
   };
   pthread_cond_init(&ret->cond, NULL);
   pthread_mutex_init(&ret->lock, NULL);
@@ -63,14 +82,6 @@ RMUtilTimer *RMUtil_NewPeriodicTimer(RMutilTimerFunc cb, void *privdata, struct
   return ret;
 }
 
-int RMUtilTimer_Stop(RMUtilTimer *t) {
-  int rc;
-  if (0 == (rc = pthread_cond_signal(&t->cond))) {
-    rc = pthread_join(t->thread, NULL);
-  }
-  return rc;
-}
-
-void RMUtilTimer_Free(RMUtilTimer *t) {
-  free(t);
+int RMUtilTimer_Terminate(struct RMUtilTimer *t) {
+  return pthread_cond_signal(&t->cond);
 }

rmutil/periodic.h

@@ -13,15 +13,34 @@ struct RMUtilTimer;
  * pre-existing private data */
 typedef void (*RMutilTimerFunc)(RedisModuleCtx *ctx, void *privdata);
 
+typedef void (*RMUtilTimerTerminationFunc)(void *privdata);
+
 /* Create and start a new periodic timer. Each timer has its own thread and can only be run and
  * stopped once. The timer runs `cb` every `interval` with `privdata` passed to the callback. */
-struct RMUtilTimer *RMUtil_NewPeriodicTimer(RMutilTimerFunc cb, void *privdata,
-                                            struct timespec interval);
+struct RMUtilTimer *RMUtil_NewPeriodicTimer(RMutilTimerFunc cb, RMUtilTimerTerminationFunc onTerm,
+                                            void *privdata, struct timespec interval);
+
+/* set a new frequency for the timer. This will take effect AFTER the next trigger */
+void RMUtilTimer_SetInterval(struct RMUtilTimer *t, struct timespec newInterval);
 
-/* Stop the timer loop. This should return immediately and join the thread */
-int RMUtilTimer_Stop(struct RMUtilTimer *t);
+/* Stop the timer loop, call the termination callbck to free up any resources linked to the timer,
+ * and free the timer after stopping.
+ *
+ * This function doesn't wait for the thread to terminate, as it may cause a race condition if the
+ * timer's callback is waiting for the redis global lock.
+ * Instead you should make sure any resources are freed by the callback after the thread loop is
+ * finished.
+ *
+ * The timer is freed automatically, so the callback doesn't need to do anything about it.
+ * The callback gets the timer's associated privdata as its argument.
+ *
+ * If no callback is specified we do not free up privdata. If privdata is NULL we still call the
+ * callback, as it may log stuff or free global resources.
+ */
+int RMUtilTimer_Terminate(struct RMUtilTimer *t);
 
-/* Free the timer context. The caller should be responsible for freeing the private data at this
+/* DEPRECATED - do not use this function (well now you can't), use terminate instead
+    Free the timer context. The caller should be responsible for freeing the private data at this
  * point */
-void RMUtilTimer_Free(struct RMUtilTimer *t);
+// void RMUtilTimer_Free(struct RMUtilTimer *t);
 #endif