fixed some flaky tests

corelib/test/test_imuthread.cpp

@@ -6,6 +6,7 @@
 #include <rtabmap/utilite/UEventsManager.h>
 #include <rtabmap/utilite/UFile.h>
 #include <rtabmap/utilite/UConversion.h>
+#include <rtabmap/utilite/UMutex.h>
 #include <rtabmap/utilite/UTimer.h>
 #include <fstream>
 #include <cmath>
@@ -74,8 +75,36 @@ public:
 		bool valid;
 	};
 
-	void clear() { samples_.clear(); }
+	// Dispatched on UEventsManager thread; reads (size/snapshot) come from the test
-	const std::vector<Sample> & samples() const { return samples_; }
+	// thread, so all access to samples_ goes through mutex_.
+	void clear()
+	{
+		UScopeMutex lock(mutex_);
+		samples_.clear();
+	}
+	std::vector<Sample> snapshot() const
+	{
+		UScopeMutex lock(mutex_);
+		return samples_;
+	}
+	size_t size() const
+	{
+		UScopeMutex lock(mutex_);
+		return samples_.size();
+	}
+	size_t validCount() const
+	{
+		UScopeMutex lock(mutex_);
+		size_t count = 0;
+		for(size_t i = 0; i < samples_.size(); ++i)
+		{
+			if(samples_[i].valid)
+			{
+				++count;
+			}
+		}
+		return count;
+	}
 
 protected:
 	virtual bool handleEvent(UEvent * event)
@@ -87,12 +116,14 @@ protected:
 			sample.data = imuEvent->getData();
 			sample.stamp = imuEvent->getStamp();
 			sample.valid = !imuEvent->getData().empty();
+			UScopeMutex lock(mutex_);
 			samples_.push_back(sample);
 		}
 		return false;
 	}
 
 private:
+	mutable UMutex mutex_;
 	std::vector<Sample> samples_;
 };
 
@@ -112,20 +143,9 @@ static std::vector<IMUEventCollector::Sample> runThread(
 		{
 			break;
 		}
-		if(minValidSamples > 0)
+		if(minValidSamples > 0 && collector.validCount() >= minValidSamples)
 		{
-			size_t validCount = 0;
+			break;
-			for(size_t i = 0; i < collector.samples().size(); ++i)
-			{
-				if(collector.samples()[i].valid)
-				{
-					++validCount;
-				}
-			}
-			if(validCount >= minValidSamples)
-			{
-				break;
-			}
 		}
 		uSleep(5);
 	}
@@ -135,8 +155,11 @@ static std::vector<IMUEventCollector::Sample> runThread(
 		thread.kill();
 	}
 	thread.join(true);
+	// Remove handler before snapshot. removeHandler does not block in-flight
+	// dispatches, so take the snapshot under the collector's mutex to avoid a
+	// race with a still-running dispatch posting one final event.
 	UEventsManager::removeHandler(&collector);
-	return collector.samples();
+	return collector.snapshot();
 }
 
 } // namespace

corelib/test/test_rtabmap_integration.cpp

@@ -541,11 +541,13 @@ TEST_F(RtabmapIntegrationFixture, NetherdroneLidar3D)
 
 #ifdef RTABMAP_OCTOMAP
 	// Grid/RayTracing requires OctoMap support; verify the 3D map was
-	// actually assembled when the build has it. ICP-only replay is fully
+	// actually assembled when the build has it. ICP-only replay is
-	// deterministic so the leaf counts are exact across runs.
+	// deterministic on a given platform but absolute leaf counts can shift
-	EXPECT_EQ(21372, result.octomapNodes);
+	// slightly across PCL/Eigen/OpenMP configurations, so use a small
-	EXPECT_EQ(16178, result.octomapEmptyCells);
+	// tolerance (~1-3%) rather than exact equality.
-	EXPECT_EQ(1845, result.octomapObstacleCells);
+	EXPECT_NEAR(21372, result.octomapNodes, 300);
+	EXPECT_NEAR(16178, result.octomapEmptyCells, 300);
+	EXPECT_NEAR(1845, result.octomapObstacleCells, 50);
 #endif
 
 	// Replay is deterministic; matching golden GT was captured from a clean
@@ -585,11 +587,14 @@ TEST_F(RtabmapIntegrationFixture, PR2_Scan2D_Stereo)
 	EXPECT_GE(result.gridObstacleCells, 4900);
 	EXPECT_LE(result.gridObstacleCells, 5300);
 #ifdef RTABMAP_OCTOMAP
-	// Observed across 5 runs: empty 1834-1857, obstacle 21502-21671.
+	// Observed: empty 1805-1902, obstacle 21502-22383. Bounds are wide
+	// because without g2o (OdomF2M/BundleAdjustment disabled) visual
+	// odometry drifts a bit differently run-to-run, which propagates into
+	// the assembled occupancy grid.
 	EXPECT_GE(result.octomapEmptyCells, 1700);
-	EXPECT_LE(result.octomapEmptyCells, 2000);
+	EXPECT_LE(result.octomapEmptyCells, 2100);
 	EXPECT_GE(result.octomapObstacleCells, 21000);
-	EXPECT_LE(result.octomapObstacleCells, 22000);
+	EXPECT_LE(result.octomapObstacleCells, 23000);
 #endif
 	// Stereo F2M visual odom + visual loop closure -- observed RMSE ~3 cm,
 	// 5 cm bound gives ~50% headroom for run-to-run feature variance.
@@ -628,10 +633,13 @@ TEST_F(RtabmapIntegrationFixture, PR2_Scan2D_RGBD)
 	EXPECT_GE(result.gridObstacleCells, 4400);
 	EXPECT_LE(result.gridObstacleCells, 4900);
 #ifdef RTABMAP_OCTOMAP
-	// Observed across 5 runs: empty 6452-7474, obstacle 41125-42883.
+	// Observed: empty 6072-7474, obstacle 39924-42883. Bounds are wide
-	EXPECT_GE(result.octomapEmptyCells, 6000);
+	// because without g2o (OdomF2M/BundleAdjustment disabled) visual
+	// odometry drifts a bit differently run-to-run, which propagates into
+	// the assembled occupancy grid.
+	EXPECT_GE(result.octomapEmptyCells, 5500);
 	EXPECT_LE(result.octomapEmptyCells, 8000);
-	EXPECT_GE(result.octomapObstacleCells, 40000);
+	EXPECT_GE(result.octomapObstacleCells, 38000);
 	EXPECT_LE(result.octomapObstacleCells, 44000);
 #endif
 	// RGB-D F2M visual odom + visual loop closure -- observed RMSE ~13 cm

utilite/src/UEventsManager.cpp

@@ -225,17 +225,18 @@ bool UEventsManager::dispatchEvent(UEvent * event, const UEventsSender * sender)
 		if(std::find(handlers_.begin(), handlers_.end(), *it) != handlers_.end())
 		{
 			UEventsHandler * handler = *it;
-			handlersMutex_.unlock();
 
 			// Don't process event if the handler is the same as the sender
 			if(handler != sender)
 			{
-				// To be able to add/remove an handler in a handleEvent call (without a deadlock)
+				// Keep handlersMutex_ held across handleEvent() so a concurrent
-				// @see _addHandler(), _removeHandler()
+				// removeHandler() in another thread cannot return while this
+				// dispatch is in flight (which would let the handler be
+				// destroyed under us). The mutex is recursive, so a handler
+				// that calls addHandler()/removeHandler() from within
+				// handleEvent() still works.
 				handled = handler->handleEvent(event);
 			}
-
-			handlersMutex_.lock();
 		}
 	}
 	handlersMutex_.unlock();

utilite/test/test_uevents.cpp

@@ -462,17 +462,146 @@ TEST(UEventsTest, HandlerReceivesCorrectEvent)
 {
     TestHandler handler;
     UEventsManager::addHandler(&handler);
-    
+
     handler.reset();
-    
+
     TestEvent* event = new TestEvent(42);
     UEventsManager::post(event, true);
-    
+
     std::this_thread::sleep_for(std::chrono::milliseconds(50));
-    
+
     EXPECT_EQ(handler.getLastEventCode(), 42);
     EXPECT_STREQ(handler.getLastEventClassName().c_str(), "TestEvent");
-    
+
     UEventsManager::removeHandler(&handler);
 }
 
+// Handler that blocks inside handleEvent until released, used to observe
+// whether removeHandler() waits for an in-flight dispatch to finish.
+class BlockingHandler : public UEventsHandler
+{
+public:
+    BlockingHandler() : inHandler_(false), finishedHandler_(false), release_(false) {}
+
+    bool inHandler() const { return inHandler_.load(); }
+    bool finishedHandler() const { return finishedHandler_.load(); }
+    void release() { release_ = true; }
+
+protected:
+    virtual bool handleEvent(UEvent *)
+    {
+        inHandler_ = true;
+        while(!release_.load())
+        {
+            std::this_thread::sleep_for(std::chrono::milliseconds(1));
+        }
+        finishedHandler_ = true;
+        return false;
+    }
+
+private:
+    std::atomic<bool> inHandler_;
+    std::atomic<bool> finishedHandler_;
+    std::atomic<bool> release_;
+};
+
+// Regression test: removeHandler() must not return while another thread is
+// inside handleEvent() for that handler. Otherwise the caller can destroy the
+// handler (or data it points at) while the dispatcher still uses it, causing
+// heap corruption.
+TEST(UEventsTest, RemoveHandlerBlocksUntilHandleEventCompletes)
+{
+    BlockingHandler handler;
+    UEventsManager::addHandler(&handler);
+
+    UEventsManager::post(new TestEvent(), true);
+
+    // Wait until the dispatcher thread is inside handleEvent.
+    UTimer waitEnter;
+    while(!handler.inHandler() && waitEnter.ticks() < 1.0)
+    {
+        std::this_thread::sleep_for(std::chrono::milliseconds(1));
+    }
+    ASSERT_TRUE(handler.inHandler())
+        << "Dispatcher never entered handleEvent within 1s";
+
+    // Call removeHandler from a separate thread; it must block until the
+    // dispatcher exits handleEvent.
+    std::atomic<bool> removeReturned(false);
+    std::thread remover([&]() {
+        UEventsManager::removeHandler(&handler);
+        removeReturned = true;
+    });
+
+    // Give the remover thread time to call into removeHandler and (correctly)
+    // get stuck waiting for the dispatcher.
+    std::this_thread::sleep_for(std::chrono::milliseconds(50));
+    EXPECT_FALSE(removeReturned.load())
+        << "removeHandler returned while handleEvent was still running";
+    EXPECT_FALSE(handler.finishedHandler());
+
+    // Let the dispatcher finish; removeHandler should now complete promptly.
+    handler.release();
+
+    UTimer waitReturn;
+    while(!removeReturned.load() && waitReturn.ticks() < 1.0)
+    {
+        std::this_thread::sleep_for(std::chrono::milliseconds(1));
+    }
+    EXPECT_TRUE(removeReturned.load())
+        << "removeHandler did not return after handleEvent completed";
+    EXPECT_TRUE(handler.finishedHandler());
+    remover.join();
+}
+
+// Handler that removes itself from within handleEvent, exercising the
+// recursive-mutex behavior of handlersMutex_.
+class SelfRemovingHandler : public UEventsHandler
+{
+public:
+    SelfRemovingHandler() : called_(false) {}
+    bool called() const { return called_.load(); }
+
+protected:
+    virtual bool handleEvent(UEvent *)
+    {
+        called_ = true;
+        // Must not deadlock: handlersMutex_ is recursive, so the dispatcher
+        // thread can re-enter it via removeHandler() while still holding it
+        // for the surrounding dispatch.
+        UEventsManager::removeHandler(this);
+        return false;
+    }
+
+private:
+    std::atomic<bool> called_;
+};
+
+TEST(UEventsTest, HandlerCanRemoveItselfFromWithinHandleEvent)
+{
+    SelfRemovingHandler handler;
+    UEventsManager::addHandler(&handler);
+
+    UEventsManager::post(new TestEvent(), true);
+
+    UTimer t;
+    while(!handler.called() && t.ticks() < 1.0)
+    {
+        std::this_thread::sleep_for(std::chrono::milliseconds(1));
+    }
+    EXPECT_TRUE(handler.called())
+        << "Handler appears to be deadlocked inside handleEvent";
+
+    // After self-removal, further events must not reach the handler. Since
+    // handleEvent flipped 'called_' once, post again and confirm no further
+    // calls are observed (we can only verify via a second handler that the
+    // dispatcher is still alive afterwards).
+    TestHandler sentinel;
+    UEventsManager::addHandler(&sentinel);
+    UEventsManager::post(new TestEvent(), true);
+    std::this_thread::sleep_for(std::chrono::milliseconds(50));
+    EXPECT_GT(sentinel.getEventCount(), 0)
+        << "Dispatcher is no longer delivering events after self-removal";
+    UEventsManager::removeHandler(&sentinel);
+}
+