indmanager: Force thread cancellation on im_stop_ind()

This fixes thread leak due to im_stop_ind() not cleaning up running thread.
This change also brings some guarantees, see README for details.

2 files changed, 50 insertions, 17 deletions

diff --git a/src/indmanager/README b/src/indmanager/README
index ddc5569..4f9654d 100644
--- a/src/indmanager/README
+++ b/src/indmanager/README
@@ -66,11 +66,18 @@ bool im_start_ind(IMManager *manager, const CMPIContext *ctx, IMError *err);
 Informs the manager to start sending indications. It is recursively calling
 IMEventWatcher and IMInstGather (if using polling, the default gather is used).
 Returns true if everything started correctly, false otherwise and err is set.
+NOTE: Callbacks are called within a thread that can be forcefully interrupted
+by the im_stop_ind() call without any chance for cleanup. For that reason, any
+dynamically allocated memory will get leaked. Please keep that in mind and use
+as much stack memory as possible. If needed, please perform cleanup at the same
+place the im_stop_ind() is called. Only one thread is executed within each
+indication manager instance, no need for extra thread safety.
 
 * im_stop_ind
 bool im_stop_ind(IMManager *manager, const CMPIContext *ctx, IMError *err);
-Informs the manager to stop sending indications. Returns true when everything
-is done correctly, false otherwise and err is set.
+Informs the manager to stop sending indications. Internally this call cancels
+the thread function, see the note above. Returns true when everything is done
+correctly, false otherwise and err is set.
 
 * IMFilterChecker
 typedef bool (*IMFilterChecker) (const CMPISelectExp *filter);
@@ -88,9 +95,13 @@ is informed that event hasn't occured and will not send indication and will
 retart the loop - the callback will be executed again soon.
 User can use data to transfer any information from this callback
 to IMInstGather.
+This is called within a thread and is advised that any blocking call should be
+a pthread cancellation point (see pthreads(7)) for easy interruption (see the
+note at im_start_ind()).
 
 * IMInstGather
-typedef bool (*IMInstGather) (CMPIInstance **old, CMPIInstance **new, void *data);
+typedef bool (*IMInstGather) (const IMManager *manager, CMPIInstance **old,
+                              CMPIInstance **new, void *data);
 This callback has to be defined only when not using default polling mechanism.
 data are passed from IMEventWatcher and are supposed to help creating instances.
 User should create and set instance and/or instances. When type of indications
@@ -101,6 +112,13 @@ In other words, the return value means "There is another indication
 with instances to be send". When false is returned the manager will send
 the last indication and starts another round in the loop.
 
+Cancellation, indications enabling and disabling
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Indications should be enabled or disabled in relation with EnableIndications()
+and DisableIndications() CIM class method calls. These are called typically when
+subscription is added or removed. Note that enabling or disabling indications
+multiple times has no effect.
+
 Polling
 ~~~~~~~
 IM supports basic polling mechanism. When enabled the IM remembers instances
diff --git a/src/indmanager/ind_manager.c b/src/indmanager/ind_manager.c
index c60b1f4..426cc0c 100644
--- a/src/indmanager/ind_manager.c
+++ b/src/indmanager/ind_manager.c
@@ -659,9 +659,7 @@ bool send_indication(CMPIInstance *old, CMPIInstance *new, IMManager *manager)
  */
 static void *manage(void *data)
 {
-    //TODO - make this thread's cancel state and type more specific?
-    // Set the type to PTHREAD_CANCEL_ASYNCHRONOUS?
-    // Switch Enable/Disable state on some places?
+    // TODO: Switch Enable/Disable state on some places?
     IMManager *manager = (IMManager *)data;
     IMError err = IM_ERR_OK; // TODO - How to handle potential errors?
     DEBUG("manage thread started");
@@ -669,6 +667,9 @@ static void *manage(void *data)
     IMPolledDiffs diffs = {NULL};
     CMPIInstance *iold = NULL, *inew = NULL;
 
+    pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL);
+    pthread_setcanceltype(PTHREAD_CANCEL_DEFERRED, NULL);
+
     DEBUG("manage thread - attaching thread manager context = %p", manager->ctx_manage);
     CBAttachThread(manager->broker, manager->ctx_manage);
 
@@ -827,6 +828,7 @@ IMManager* im_create_manager(IMInstGather gather, IMFilterChecker f_checker,
     manager->enums = NULL;
     manager->ctx_main = NULL;
     manager->ctx_manage = NULL;
+    manager->data = NULL;
     DEBUG("Manager created");
 
     if (pthread_mutex_init(&_t_mutex, NULL) ||
@@ -850,22 +852,12 @@ bool im_destroy_manager(IMManager *manager, const CMPIContext *ctx,
         *err = IM_ERR_CONTEXT;
         return false;
     }
+    im_stop_ind(manager, ctx, err);
     pthread_mutex_lock(&_t_mutex);
-    manager->ctx_main = ctx;
-    if (!im_stop_ind(manager, ctx, err)) {
-        pthread_mutex_unlock(&_t_mutex);
-        return false;
-    }
     if (!remove_all_filters(manager, err)) {
         pthread_mutex_unlock(&_t_mutex);
         return false;
     }
-    if (pthread_cancel(_t_manage)) {
-        *err = IM_ERR_THREAD;
-        pthread_mutex_unlock(&_t_mutex);
-        return false;
-    }
-
     pthread_mutex_unlock(&_t_mutex);
     DEBUG("Destroying manager");
     if (pthread_mutex_destroy(&_t_mutex)) {
@@ -1107,6 +1099,11 @@ bool im_start_ind(IMManager *manager, const CMPIContext *ctx, IMError *err)
         *err = IM_ERR_CONTEXT;
         return false;
     }
+    if (manager->running) {
+        DEBUG("WARNING: thread already exists");
+        *err = IM_ERR_THREAD;
+        return false;
+    }
     manager->ctx_main = ctx;
     manager->ctx_manage = CBPrepareAttachThread(manager->broker,
                                                 manager->ctx_main);
@@ -1135,6 +1132,24 @@ bool im_stop_ind(IMManager *manager, const CMPIContext *ctx, IMError *err)
     manager->ctx_main = ctx;
     DEBUG("Stopping indications");
     manager->running = false;
+
+    /* No locking here due to potential deadlock */
+    if (pthread_cancel(_t_manage)) {
+        *err = IM_ERR_THREAD;
+        return false;
+    }
+    if (pthread_join(_t_manage, NULL)) {
+        *err = IM_ERR_THREAD;
+        return false;
+    }
+
+    /* Cleanup */
+    remove_all_enums(manager, err);
+    manager->data = NULL;
+    manager->ctx_manage = NULL;
+    /* Mutex could have been in locked state when the thread was canceled */
+    pthread_mutex_unlock(&_t_mutex);
+
     return true;
 }