[libcamera-devel,10/12] test: timer-thread: Destroy Object from correct thread context
diff mbox series

Message ID 20240121035948.4226-11-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: Hardening against thread race conditions
Related show

Commit Message

Laurent Pinchart Jan. 21, 2024, 3:59 a.m. UTC
The TimeoutHandler used in the test is destroyed from the main thread,
which is invalid for a thread-bound object bound to a different thread.
Fix it by destroying it with deleteLater().

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 test/timer-fail.cpp   | 16 +++++++++++-----
 test/timer-thread.cpp | 14 ++++++++++----
 2 files changed, 21 insertions(+), 9 deletions(-)

Comments

Milan Zamazal Jan. 22, 2024, 8:21 p.m. UTC | #1
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> The TimeoutHandler used in the test is destroyed from the main thread,
> which is invalid for a thread-bound object bound to a different thread.
> Fix it by destroying it with deleteLater().
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

> ---
>  test/timer-fail.cpp   | 16 +++++++++++-----
>  test/timer-thread.cpp | 14 ++++++++++----
>  2 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/test/timer-fail.cpp b/test/timer-fail.cpp
> index e9a3b1b61bcb..b89c85f2ee72 100644
> --- a/test/timer-fail.cpp
> +++ b/test/timer-fail.cpp
> @@ -54,7 +54,9 @@ protected:
>  	int init()
>  	{
>  		thread_.start();
> -		timeout_.moveToThread(&thread_);
> +
> +		timeout_ = new TimeoutHandler();
> +		timeout_->moveToThread(&thread_);
>  
>  		return TestPass;
>  	}
> @@ -67,7 +69,7 @@ protected:
>  		 * event dispatcher to make sure we don't succeed simply because
>  		 * the event dispatcher hasn't noticed the timer restart.
>  		 */
> -		timeout_.start();
> +		timeout_->start();
>  		thread_.eventDispatcher()->interrupt();
>  
>  		this_thread::sleep_for(chrono::milliseconds(200));
> @@ -77,7 +79,7 @@ protected:
>  		 * TestPass in the unexpected (usually known as "fail"), case,
>  		 * and TestFail otherwise.
>  		 */
> -		if (timeout_.timeout()) {
> +		if (timeout_->timeout()) {
>  			cout << "Timer start from wrong thread succeeded unexpectedly"
>  			     << endl;
>  			return TestPass;
> @@ -88,13 +90,17 @@ protected:
>  
>  	void cleanup()
>  	{
> -		/* Must stop thread before destroying timeout. */
> +		/*
> +		 * Object class instances must be destroyed from the thread
> +		 * they live in.
> +		 */
> +		timeout_->deleteLater();
>  		thread_.exit(0);
>  		thread_.wait();
>  	}
>  
>  private:
> -	TimeoutHandler timeout_;
> +	TimeoutHandler *timeout_;
>  	Thread thread_;
>  };
>  
> diff --git a/test/timer-thread.cpp b/test/timer-thread.cpp
> index 4caf4e33b2ba..8675e2480aa9 100644
> --- a/test/timer-thread.cpp
> +++ b/test/timer-thread.cpp
> @@ -50,7 +50,9 @@ protected:
>  	int init()
>  	{
>  		thread_.start();
> -		timeout_.moveToThread(&thread_);
> +
> +		timeout_ = new TimeoutHandler();
> +		timeout_->moveToThread(&thread_);
>  
>  		return TestPass;
>  	}
> @@ -63,7 +65,7 @@ protected:
>  		 */
>  		this_thread::sleep_for(chrono::milliseconds(200));
>  
> -		if (!timeout_.timeout()) {
> +		if (!timeout_->timeout()) {
>  			cout << "Timer expiration test failed" << endl;
>  			return TestFail;
>  		}
> @@ -73,13 +75,17 @@ protected:
>  
>  	void cleanup()
>  	{
> -		/* Must stop thread before destroying timeout. */
> +		/*
> +		 * Object class instances must be destroyed from the thread
> +		 * they live in.
> +		 */
> +		timeout_->deleteLater();
>  		thread_.exit(0);
>  		thread_.wait();
>  	}
>  
>  private:
> -	TimeoutHandler timeout_;
> +	TimeoutHandler *timeout_;
>  	Thread thread_;
>  };

Patch
diff mbox series

diff --git a/test/timer-fail.cpp b/test/timer-fail.cpp
index e9a3b1b61bcb..b89c85f2ee72 100644
--- a/test/timer-fail.cpp
+++ b/test/timer-fail.cpp
@@ -54,7 +54,9 @@  protected:
 	int init()
 	{
 		thread_.start();
-		timeout_.moveToThread(&thread_);
+
+		timeout_ = new TimeoutHandler();
+		timeout_->moveToThread(&thread_);
 
 		return TestPass;
 	}
@@ -67,7 +69,7 @@  protected:
 		 * event dispatcher to make sure we don't succeed simply because
 		 * the event dispatcher hasn't noticed the timer restart.
 		 */
-		timeout_.start();
+		timeout_->start();
 		thread_.eventDispatcher()->interrupt();
 
 		this_thread::sleep_for(chrono::milliseconds(200));
@@ -77,7 +79,7 @@  protected:
 		 * TestPass in the unexpected (usually known as "fail"), case,
 		 * and TestFail otherwise.
 		 */
-		if (timeout_.timeout()) {
+		if (timeout_->timeout()) {
 			cout << "Timer start from wrong thread succeeded unexpectedly"
 			     << endl;
 			return TestPass;
@@ -88,13 +90,17 @@  protected:
 
 	void cleanup()
 	{
-		/* Must stop thread before destroying timeout. */
+		/*
+		 * Object class instances must be destroyed from the thread
+		 * they live in.
+		 */
+		timeout_->deleteLater();
 		thread_.exit(0);
 		thread_.wait();
 	}
 
 private:
-	TimeoutHandler timeout_;
+	TimeoutHandler *timeout_;
 	Thread thread_;
 };
 
diff --git a/test/timer-thread.cpp b/test/timer-thread.cpp
index 4caf4e33b2ba..8675e2480aa9 100644
--- a/test/timer-thread.cpp
+++ b/test/timer-thread.cpp
@@ -50,7 +50,9 @@  protected:
 	int init()
 	{
 		thread_.start();
-		timeout_.moveToThread(&thread_);
+
+		timeout_ = new TimeoutHandler();
+		timeout_->moveToThread(&thread_);
 
 		return TestPass;
 	}
@@ -63,7 +65,7 @@  protected:
 		 */
 		this_thread::sleep_for(chrono::milliseconds(200));
 
-		if (!timeout_.timeout()) {
+		if (!timeout_->timeout()) {
 			cout << "Timer expiration test failed" << endl;
 			return TestFail;
 		}
@@ -73,13 +75,17 @@  protected:
 
 	void cleanup()
 	{
-		/* Must stop thread before destroying timeout. */
+		/*
+		 * Object class instances must be destroyed from the thread
+		 * they live in.
+		 */
+		timeout_->deleteLater();
 		thread_.exit(0);
 		thread_.wait();
 	}
 
 private:
-	TimeoutHandler timeout_;
+	TimeoutHandler *timeout_;
 	Thread thread_;
 };