Merge pull request #3 from meyerj/master-update-hook-vs-callback-queue

Fixed blocking calls to ExecutionEngine::process()
psoetens · Apr 13, 2015 · 8d53590 · 8d53590
2 parents d0f7919 + 58cb4bb
commit 8d53590
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 12 deletions.
diff --git a/rtt/Activity.cpp b/rtt/Activity.cpp
@@ -103,6 +103,10 @@ namespace RTT
     Activity::~Activity()
     {
         stop();
+
+        // We need to join the activity's thread before destruction as the thread function might still
+        // access member variables. Activity::stop() does not guarantuee to stop the underlying thread.
+        terminate();
     }
 
     os::ThreadInterface* Activity::thread() {
@@ -142,7 +146,10 @@ namespace RTT
         if ( ! Thread::isActive() )
             return false;
         //a trigger is always allowed when active
-        msg_cond.broadcast();
+        {
+            os::MutexLock lock(msg_lock);
+            msg_cond.broadcast();
+        }
         Thread::start();
         return true;
     }
@@ -158,7 +165,6 @@ namespace RTT
             return false;
         }
         mtimeout = true;
-        msg_cond.broadcast();
         Thread::start();
         return true;
     }

diff --git a/rtt/ExecutionEngine.cpp b/rtt/ExecutionEngine.cpp
@@ -107,6 +107,7 @@ namespace RTT
             assert(foo);
             if ( foo->execute() == false ){
                 foo->unloaded();
+                os::MutexLock lock(msg_lock);
                 msg_cond.broadcast(); // required for waitForFunctions() (3rd party thread)
             } else {
                 f_queue->enqueue( foo );
@@ -211,16 +212,16 @@ namespace RTT
                 assert( com );
                 com->executeAndDispose();
             }
-            // there's no need to hold the lock during
-            // emptying the queue. But we must hold the
-            // lock once between excuteAndDispose and the
-            // broadcast to avoid the race condition in
-            // waitForMessages().
-            // This allows us to recurse into processMessages.
-            MutexLock locker( msg_lock );
         }
-        if ( com )
-            msg_cond.broadcast(); // required for waitForMessages() (3rd party thread)
+
+        // there's no need to hold the lock during
+        // emptying the queue. But we must hold the
+        // lock once between excuteAndDispose and the
+        // broadcast to avoid the race condition in
+        // waitForMessages().
+        // This allows us to recurse into processMessages.
+        MutexLock locker( msg_lock );
+        msg_cond.broadcast(); // required for waitForMessages() (3rd party thread)
     }
 
     bool ExecutionEngine::process( DisposableInterface* c )
@@ -237,7 +238,10 @@ namespace RTT
 
             bool result = mqueue->enqueue( c );
             this->getActivity()->trigger();
-            msg_cond.broadcast(); // required for waitAndProcessMessages() (EE thread)
+            {
+                os::MutexLock lock(msg_lock);
+                msg_cond.broadcast(); // required for waitAndProcessMessages() (EE thread)
+            }
             return result;
         }
         return false;

diff --git a/rtt/os/Thread.cpp b/rtt/os/Thread.cpp
@@ -626,6 +626,7 @@ namespace RTT {
             rtos_sem_signal(&sem);
 
             rtos_task_delete(&rtos_task); // this must join the thread.
+            active = false;
         }
 
         const char* Thread::getName() const