From patchwork Tue Jan 23 01:12:38 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 19449 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 7A69FC324D for ; Tue, 23 Jan 2024 01:12:50 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id D979662944; Tue, 23 Jan 2024 02:12:48 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="T0HJcGJO"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 789CA61D46 for ; Tue, 23 Jan 2024 02:12:47 +0100 (CET) Received: from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi [89.27.53.110]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 2319F1574; Tue, 23 Jan 2024 02:11:34 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1705972294; bh=wMpLozaP/xKqQlEJAwOaeetDCtxiB+1F5J2rq9uRqGw=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=T0HJcGJO/49dHhI9p7cf6qlKRSJvNbw5WeSLn1qZkYzKZR00CKU9AuAdJEoox6iDS 33iV0G+pdd5YEIeV4fuWD2e37ErYDSqya7YBrlWFVGPD8zSGhZiRZT6DicZvlNRu8+ e18wd37baYPmTAVRa8XLP93rGKxyTUlbCMuMHCho= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Subject: [PATCH v2 01/12] libcamera: object: Fix thread-bound reference in documentation Date: Tue, 23 Jan 2024 03:12:38 +0200 Message-ID: <20240123011249.22716-2-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> References: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" The Object::message() function is documented as thread-bound without using the correct \threadbound reference. Fix it to ensure it gets included in the thread safety context list. Signed-off-by: Laurent Pinchart Reviewed-by: Milan Zamazal --- src/libcamera/base/object.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcamera/base/object.cpp b/src/libcamera/base/object.cpp index 92cecd22fbe9..1fce5a2af9af 100644 --- a/src/libcamera/base/object.cpp +++ b/src/libcamera/base/object.cpp @@ -259,7 +259,7 @@ void Object::message(Message *msg) * Moving an object that has a parent is not allowed, and causes undefined * behaviour. * - * \context This function is thread-bound. + * \context This function is \threadbound. */ void Object::moveToThread(Thread *thread) { From patchwork Tue Jan 23 01:12:39 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 19450 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 4F750C32BC for ; Tue, 23 Jan 2024 01:12:53 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 7CF2C6294F; Tue, 23 Jan 2024 02:12:52 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="Um+tJSxE"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 28D936294A for ; Tue, 23 Jan 2024 02:12:49 +0100 (CET) Received: from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi [89.27.53.110]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 666C61593; Tue, 23 Jan 2024 02:11:35 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1705972295; bh=WizYAfAkOLHPMkVocllD4Y1dKtYd2x2CsN/2zqXAyHA=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Um+tJSxENxapXw1uh0zwVxHCwEXoUeFEZATdAUFlCIYO450jXnGri8HoCAMYe0hXm XfFC7JDGgBjEsqpeVuTbOSSZLBX+eMEJLvfn7mq6GaZB7cyMqP9//kBFjoD1GZBsjE GssTrFqQXAfHlA0eIK6ct0kaZuK97BFLfpRyT8sc= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Subject: [PATCH v2 02/12] libcamera: signal: Replace object.h inclusion with forward declatation Date: Tue, 23 Jan 2024 03:12:39 +0200 Message-ID: <20240123011249.22716-3-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> References: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" The signal.h header doesn't need to include object.h. Replace it with a forward declaration, and instead include object.h in source files that require it. It can speed up compilation a little bit, but more importantly avoids unintended dependencies from the Signal class to the Object class to be added later as the compiler will catch them. Signed-off-by: Laurent Pinchart Reviewed-by: Milan Zamazal --- Changes since v1: - Fix typo in commit message --- include/libcamera/base/signal.h | 3 ++- src/libcamera/base/bound_method.cpp | 1 + src/libcamera/base/signal.cpp | 1 + src/libcamera/base/thread.cpp | 1 + test/event-thread.cpp | 1 + test/ipa/ipa_interface_test.cpp | 1 + test/message.cpp | 1 + test/signal-threads.cpp | 1 + test/timer-thread.cpp | 1 + .../ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl | 1 + 10 files changed, 11 insertions(+), 1 deletion(-) diff --git a/include/libcamera/base/signal.h b/include/libcamera/base/signal.h index 841e4b4ca15c..444997b4525d 100644 --- a/include/libcamera/base/signal.h +++ b/include/libcamera/base/signal.h @@ -13,10 +13,11 @@ #include #include -#include namespace libcamera { +class Object; + class SignalBase { public: diff --git a/src/libcamera/base/bound_method.cpp b/src/libcamera/base/bound_method.cpp index 3ecec51c4b75..c83d623f107d 100644 --- a/src/libcamera/base/bound_method.cpp +++ b/src/libcamera/base/bound_method.cpp @@ -7,6 +7,7 @@ #include #include +#include #include #include diff --git a/src/libcamera/base/signal.cpp b/src/libcamera/base/signal.cpp index 02290ad779ee..f1018b37038c 100644 --- a/src/libcamera/base/signal.cpp +++ b/src/libcamera/base/signal.cpp @@ -8,6 +8,7 @@ #include #include +#include /** * \file base/signal.h diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp index b96951ac19ba..75693c92a0b1 100644 --- a/src/libcamera/base/thread.cpp +++ b/src/libcamera/base/thread.cpp @@ -18,6 +18,7 @@ #include #include #include +#include /** * \page thread Thread Support diff --git a/test/event-thread.cpp b/test/event-thread.cpp index ef8a52c3de55..88a8c07ef9f0 100644 --- a/test/event-thread.cpp +++ b/test/event-thread.cpp @@ -11,6 +11,7 @@ #include #include +#include #include #include diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp index 051ef96e7ed2..56f3cd6d57ba 100644 --- a/test/ipa/ipa_interface_test.cpp +++ b/test/ipa/ipa_interface_test.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include diff --git a/test/message.cpp b/test/message.cpp index d148a13d6c7a..0e76f323e3b9 100644 --- a/test/message.cpp +++ b/test/message.cpp @@ -11,6 +11,7 @@ #include #include +#include #include #include "test.h" diff --git a/test/signal-threads.cpp b/test/signal-threads.cpp index d5e2eb662df2..8c550eb014d8 100644 --- a/test/signal-threads.cpp +++ b/test/signal-threads.cpp @@ -10,6 +10,7 @@ #include #include +#include #include #include diff --git a/test/timer-thread.cpp b/test/timer-thread.cpp index 618217538779..0bcd0d8ce194 100644 --- a/test/timer-thread.cpp +++ b/test/timer-thread.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl index ed270f5cd49c..6e823598930e 100644 --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl @@ -18,6 +18,7 @@ #include #include +#include #include #include "libcamera/internal/control_serializer.h" From patchwork Tue Jan 23 01:12:40 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 19451 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id B4C0EC32BD for ; Tue, 23 Jan 2024 01:12:53 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 0020862950; Tue, 23 Jan 2024 02:12:52 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="kYEnCJGr"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 3DA7B628B7 for ; Tue, 23 Jan 2024 02:12:50 +0100 (CET) Received: from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi [89.27.53.110]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id D1F54188E; Tue, 23 Jan 2024 02:11:36 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1705972297; bh=kTtTbmkIWNTNAxqLFky+NCKArqRqxUteFktiBMHbrW4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=kYEnCJGreGmfXSQb798z5QSNgT/buKIkmqBfL/IZhb+6Uqlv0lUjI6s0B4yrmD6Pb WOANG37pnpfqrDaDYrYE3jvOY5o1BDmzFPScfN0ctFjjcgxINAHTD8zQF7BywvRCMR 4C+i0KvF62DtdRE1ZCuV505zfdx97fAngd7VPty4= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Subject: [PATCH v2 03/12] test: object-delete: Test deferred delete just before thread stops Date: Tue, 23 Jan 2024 03:12:40 +0200 Message-ID: <20240123011249.22716-4-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> References: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" The Object::deleteLater() function is expected to not race with stopping the thread the object is bound to. Add a test for this. The test currently fails, demonstrating a bug in libcamera. Signed-off-by: Laurent Pinchart Reviewed-by: Milan Zamazal --- Changes since v1: - Fix typo in commit message --- test/object-delete.cpp | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/test/object-delete.cpp b/test/object-delete.cpp index eabefe935974..80b7dc41cd37 100644 --- a/test/object-delete.cpp +++ b/test/object-delete.cpp @@ -33,10 +33,10 @@ public: unsigned int *deleteCount_; }; -class NewThread : public Thread +class DeleterThread : public Thread { public: - NewThread(Object *obj) + DeleterThread(Object *obj) : object_(obj) { } @@ -63,9 +63,9 @@ protected: unsigned int count = 0; TestObject *obj = new TestObject(&count); - NewThread thread(obj); - thread.start(); - thread.wait(); + DeleterThread delThread(obj); + delThread.start(); + delThread.wait(); Thread::current()->dispatchMessages(Message::Type::DeferredDelete); @@ -89,6 +89,26 @@ protected: return TestFail; } + /* + * Test that deleteLater() works properly when called just + * before the object's thread exits. + */ + Thread boundThread; + boundThread.start(); + + count = 0; + obj = new TestObject(&count); + obj->moveToThread(&boundThread); + + obj->deleteLater(); + boundThread.exit(); + boundThread.wait(); + + if (count != 1) { + cout << "Object deletion right before thread exit failed (" << count << ")" << endl; + return TestFail; + } + return TestPass; } }; From patchwork Tue Jan 23 01:12:41 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 19452 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id BB774C32BE for ; Tue, 23 Jan 2024 01:12:54 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 89C2F62955; Tue, 23 Jan 2024 02:12:53 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="XRUGVev6"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 793F06294A for ; Tue, 23 Jan 2024 02:12:51 +0100 (CET) Received: from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi [89.27.53.110]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 33CB4E4; Tue, 23 Jan 2024 02:11:38 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1705972298; bh=v+MZe0izlr8b1qKd6q3sOfkw+6ac0wk5IMNQfH6J0LY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=XRUGVev6EsMac/5eQqO8SZ5CZcZdSLaVTaW/tB+zMKzgelWOzBqJ3ywCJy9ZgzLLG QUfJxbXvxR5HIttnaX7gQtuMtNIAysFdZKHsJsQ6FiE+s0eAx/uuXVh1o7L6/2W0bu cYvIZGdt14Wm5KayDnlEhPknq4oB42oFpQ0R4KI0= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Subject: [PATCH v2 04/12] libcamera: thread: Ensure deferred deletion of all objects before stopping Date: Tue, 23 Jan 2024 03:12:41 +0200 Message-ID: <20240123011249.22716-5-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> References: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Objects can be scheduled for deletion with Object::deleteLater(), which queues a deferred deletion to the thread's event loop. As the deleteLater() function is meant to be called from a different thread, this may race with thread termination, and deferred deletions queued just before calling Thread::exit() may not be processed by the event loop. Make sure they get processed when finishing the thread, before stopping. This eliminates the race condition that occurs when calling Object::deleteLater() followed by Thread::exit() from the same thread. Calling deleteLater() from neither the thread the object is bound to or the thread calling Thread::exit() is still inherently racy. The change fixes a failure in the object-delete unit test. Signed-off-by: Laurent Pinchart Reviewed-by: Milan Zamazal --- Changes since v1: - Expand commit message - Update the Object::deleteLater() documentation --- src/libcamera/base/object.cpp | 5 +++-- src/libcamera/base/thread.cpp | 6 ++++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/libcamera/base/object.cpp b/src/libcamera/base/object.cpp index 1fce5a2af9af..14399d750e03 100644 --- a/src/libcamera/base/object.cpp +++ b/src/libcamera/base/object.cpp @@ -116,8 +116,9 @@ Object::~Object() * event loop that the object belongs to. This ensures the object is destroyed * from the right context, as required by the libcamera threading model. * - * If this function is called before the thread's event loop is started, the - * object will be deleted when the event loop starts. + * If this function is called before the thread's event loop is started or after + * it has stopped, the object will be deleted when the event loop (re)starts. If + * this never occurs, the object will be leaked. * * Deferred deletion can be used to control the destruction context with shared * pointers. An object managed with shared pointers is deleted when the last diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp index 75693c92a0b1..4ac72036aa69 100644 --- a/src/libcamera/base/thread.cpp +++ b/src/libcamera/base/thread.cpp @@ -371,6 +371,12 @@ void Thread::run() void Thread::finishThread() { + /* + * Objects may have been scheduled for deletion right before the thread + * exited. Ensure they get deleted now, before the thread stops. + */ + dispatchMessages(Message::Type::DeferredDelete); + data_->mutex_.lock(); data_->running_ = false; data_->mutex_.unlock(); From patchwork Tue Jan 23 01:12:42 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 19453 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 8B33CC32BF for ; Tue, 23 Jan 2024 01:12:57 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id AEC796295B; Tue, 23 Jan 2024 02:12:56 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="ciuIJC/V"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id C254962956 for ; Tue, 23 Jan 2024 02:12:52 +0100 (CET) Received: from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi [89.27.53.110]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 6A2271574; Tue, 23 Jan 2024 02:11:39 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1705972299; bh=IVMacLkoa85MRjROikzLeyC8lLdihWPwnn4z5mYbekQ=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=ciuIJC/VyZFOW4lq8NG57XtDjCom+BMnBo7lwTonzUaaTjVXW+JBifKxYW6mHCnCh 1/hTtEQ5u0Xp8WTH0MNNzT/kaBeL+zN90sBlHOtkeXZyraMZAjVNK+wu/8ezGzV6cU wSht3CDyrcJhCfADUtfjRo/pucnoCWovZ2rHUpVQ= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Subject: [PATCH v2 05/12] test: event-thread: Destroy Object from correct thread context Date: Tue, 23 Jan 2024 03:12:42 +0200 Message-ID: <20240123011249.22716-6-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> References: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" The EventHandler 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 Reviewed-by: Milan Zamazal --- test/event-thread.cpp | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/test/event-thread.cpp b/test/event-thread.cpp index 88a8c07ef9f0..d6e5d27af185 100644 --- a/test/event-thread.cpp +++ b/test/event-thread.cpp @@ -85,10 +85,17 @@ private: class EventThreadTest : public Test { protected: + int init() + { + thread_.start(); + + handler_ = new EventHandler(); + + return TestPass; + } + int run() { - Thread thread; - thread.start(); /* * Fire the event notifier and then move the notifier to a @@ -98,23 +105,33 @@ protected: * different thread will correctly process already pending * events in the new thread. */ - EventHandler handler; - handler.notify(); - handler.moveToThread(&thread); + handler_->notify(); + handler_->moveToThread(&thread_); this_thread::sleep_for(chrono::milliseconds(100)); - /* Must stop thread before destroying the handler. */ - thread.exit(0); - thread.wait(); - - if (!handler.notified()) { + if (!handler_->notified()) { cout << "Thread event handling test failed" << endl; return TestFail; } return TestPass; } + + void cleanup() + { + /* + * Object class instances must be destroyed from the thread + * they live in. + */ + handler_->deleteLater(); + thread_.exit(0); + thread_.wait(); + } + +private: + EventHandler *handler_; + Thread thread_; }; TEST_REGISTER(EventThreadTest) From patchwork Tue Jan 23 01:12:43 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 19454 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 745A5C32C0 for ; Tue, 23 Jan 2024 01:12:58 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id A4E5362957; Tue, 23 Jan 2024 02:12:57 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="JWcqnKJi"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 4217A62954 for ; Tue, 23 Jan 2024 02:12:54 +0100 (CET) Received: from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi [89.27.53.110]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id DACD91B9A; Tue, 23 Jan 2024 02:11:40 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1705972301; bh=1vKKV4M7kENbh/o8JW9ET2qZ2bxeYrGjnJIthe49NUg=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=JWcqnKJiaCAn9OV9U0Yc7QBICL5oMu2m5a/W15ni0Rgg+F4iDpVHHfLBcQSuVnKTg lCKbw3vZZ6xT8s2/vWnlSggfJ/lOgSH41zepBFFBmRBrLUdVB4FGNMOivrY/w4VF8U TO9N0sJAt9cJWNLVSuALvnRAfTKAsUsP79JA8Hws= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Subject: [PATCH v2 06/12] test: message: Remove incorrect slow receiver test Date: Tue, 23 Jan 2024 03:12:43 +0200 Message-ID: <20240123011249.22716-7-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> References: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" The slow receiver test verifies there's no race condition between concurrent message delivery and object deletion. This is not a valid use case in the first place, as objects are not allowed to be deleted from a different thread than the one they are bound to. Remove the incorrect test. Signed-off-by: Laurent Pinchart Reviewed-by: Milan Zamazal --- Changes since v1: - Fix typo in commit message --- test/message.cpp | 34 ---------------------------------- 1 file changed, 34 deletions(-) diff --git a/test/message.cpp b/test/message.cpp index 0e76f323e3b9..a34e0f0b5e10 100644 --- a/test/message.cpp +++ b/test/message.cpp @@ -93,25 +93,6 @@ private: bool success_; }; -class SlowMessageReceiver : public Object -{ -protected: - void message(Message *msg) - { - if (msg->type() != Message::None) { - Object::message(msg); - return; - } - - /* - * Don't access any member of the object here (including the - * vtable) as the object will be deleted by the main thread - * while we're sleeping. - */ - this_thread::sleep_for(chrono::milliseconds(100)); - } -}; - class MessageTest : public Test { protected: @@ -148,21 +129,6 @@ protected: break; } - /* - * Test for races between message delivery and object deletion. - * Failures result in assertion errors, there is no need for - * explicit checks. - */ - SlowMessageReceiver *slowReceiver = new SlowMessageReceiver(); - slowReceiver->moveToThread(&thread_); - slowReceiver->postMessage(std::make_unique(Message::None)); - - this_thread::sleep_for(chrono::milliseconds(10)); - - delete slowReceiver; - - this_thread::sleep_for(chrono::milliseconds(100)); - /* * Test recursive calls to Thread::dispatchMessages(). Messages * should be delivered correctly, without crashes or memory From patchwork Tue Jan 23 01:12:44 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 19455 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 4EFCBC32C1 for ; Tue, 23 Jan 2024 01:12:59 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 416346297C; Tue, 23 Jan 2024 02:12:59 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="CIS2lh7+"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D8836295B for ; Tue, 23 Jan 2024 02:12:55 +0100 (CET) Received: from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi [89.27.53.110]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 39AB31BB1; Tue, 23 Jan 2024 02:11:42 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1705972302; bh=z5mE57OfFaXiiOtYjcBi8iI7wh84D6GwTqAyVRToKmE=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=CIS2lh7+eNt57TTDUyt5PJAcrsDx8pp/u7dzc+pUZD8Y1Y5RUFxf4PA2aR/rBqe4h D/I4Lu3Q5bjsNc0hfBQ1AXBb/4mHFSraCWz3Rgs56fWk2DyuL/oSYzXGabN/LfiPBY fUHxnzDHNXFZCXykmz1FkPp+4J9WROpeDnGkfBeE= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Subject: [PATCH v2 07/12] test: message: Destroy Object from correct thread context Date: Tue, 23 Jan 2024 03:12:44 +0200 Message-ID: <20240123011249.22716-8-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> References: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" The MessageReceiver and RecursiveMessageReceiver used in the test are destroyed from the main thread, which is invalid for a thread-bound object bound to a different thread. Fix it by destroying them with deleteLater(). Signed-off-by: Laurent Pinchart Reviewed-by: Milan Zamazal --- test/message.cpp | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/test/message.cpp b/test/message.cpp index a34e0f0b5e10..2f9f281c5101 100644 --- a/test/message.cpp +++ b/test/message.cpp @@ -109,16 +109,19 @@ protected: return TestFail; } - MessageReceiver receiver; - receiver.moveToThread(&thread_); + MessageReceiver *receiver = new MessageReceiver(); + receiver->moveToThread(&thread_); thread_.start(); - receiver.postMessage(std::make_unique(Message::None)); + receiver->postMessage(std::make_unique(Message::None)); this_thread::sleep_for(chrono::milliseconds(100)); - switch (receiver.status()) { + MessageReceiver::Status status = receiver->status(); + receiver->deleteLater(); + + switch (status) { case MessageReceiver::NoMessage: cout << "No message received" << endl; return TestFail; @@ -135,8 +138,7 @@ protected: * leaks. Two messages need to be posted to ensure we don't only * test the simple case of a queue containing a single message. */ - std::unique_ptr recursiveReceiver = - std::make_unique(); + RecursiveMessageReceiver *recursiveReceiver = new RecursiveMessageReceiver(); recursiveReceiver->moveToThread(&thread_); recursiveReceiver->postMessage(std::make_unique(Message::None)); @@ -144,7 +146,10 @@ protected: this_thread::sleep_for(chrono::milliseconds(10)); - if (!recursiveReceiver->success()) { + bool success = recursiveReceiver->success(); + recursiveReceiver->deleteLater(); + + if (!success) { cout << "Recursive message delivery failed" << endl; return TestFail; } From patchwork Tue Jan 23 01:12:45 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 19456 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 0ECF2C323E for ; Tue, 23 Jan 2024 01:13:01 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 927316296F; Tue, 23 Jan 2024 02:13:00 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="NyVlstF3"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id D30F862962 for ; Tue, 23 Jan 2024 02:12:56 +0100 (CET) Received: from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi [89.27.53.110]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 7BED5E4; Tue, 23 Jan 2024 02:11:43 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1705972303; bh=5yuWptYz8PW21sw9SNhY5AR3DffrUrhkhXBqzoNDBu4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=NyVlstF3lqFtspZNrpjr38V9h0SoIOORwioCa3GnmzqMKnMP8MbPP73uR3tXOoGjc 8MZvYjIkJuMSf2bPm0XoUB5zHAUx3cIxoEhPlG8KPqfb1TNsQHBfuHcyE1Hp9If2Qh OEyo9KsM2v8KPlHmoVRHRmCy3SckQ70SgM6pITL0= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Subject: [PATCH v2 08/12] test: signal-threads: Destroy Object from correct thread context Date: Tue, 23 Jan 2024 03:12:45 +0200 Message-ID: <20240123011249.22716-9-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> References: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" The SignalReceiver 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 Reviewed-by: Milan Zamazal --- test/signal-threads.cpp | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/test/signal-threads.cpp b/test/signal-threads.cpp index 8c550eb014d8..8c212b6f9ade 100644 --- a/test/signal-threads.cpp +++ b/test/signal-threads.cpp @@ -59,15 +59,20 @@ private: class SignalThreadsTest : public Test { protected: + int init() + { + receiver_ = new SignalReceiver(); + signal_.connect(receiver_, &SignalReceiver::slot); + + return TestPass; + } + int run() { - SignalReceiver receiver; - signal_.connect(&receiver, &SignalReceiver::slot); - /* Test that a signal is received in the main thread. */ signal_.emit(0); - switch (receiver.status()) { + switch (receiver_->status()) { case SignalReceiver::NoSignal: cout << "No signal received for direct connection" << endl; return TestFail; @@ -83,8 +88,8 @@ protected: * Move the object to a thread and verify that the signal is * correctly delivered, with the correct data. */ - receiver.reset(); - receiver.moveToThread(&thread_); + receiver_->reset(); + receiver_->moveToThread(&thread_); thread_.start(); @@ -92,7 +97,7 @@ protected: this_thread::sleep_for(chrono::milliseconds(100)); - switch (receiver.status()) { + switch (receiver_->status()) { case SignalReceiver::NoSignal: cout << "No signal received for message connection" << endl; return TestFail; @@ -104,7 +109,7 @@ protected: break; } - if (receiver.value() != 42) { + if (receiver_->value() != 42) { cout << "Signal received with incorrect value" << endl; return TestFail; } @@ -114,11 +119,13 @@ protected: void cleanup() { + receiver_->deleteLater(); thread_.exit(0); thread_.wait(); } private: + SignalReceiver *receiver_; Thread thread_; Signal signal_; From patchwork Tue Jan 23 01:12:46 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 19457 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id B7802C32C2 for ; Tue, 23 Jan 2024 01:13:01 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 5AAA962916; Tue, 23 Jan 2024 02:13:01 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="WC7T0zma"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 2FBE662956 for ; Tue, 23 Jan 2024 02:12:58 +0100 (CET) Received: from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi [89.27.53.110]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id C96DE1593; Tue, 23 Jan 2024 02:11:44 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1705972305; bh=V7i+CEzlTUd2+3RHPPykckvdjZ2Kf7qJ5QFDTsyn/lI=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=WC7T0zmazMaLGMX0Jq04hi0REZhmdxn2wNWU14euer17IFrlHOrfe4sGW2hGCEUS5 aeFvNo6Y7dka3uo7bptxg9joRHlyUucZs4BOzUTMoa/dUjpjrUaY1g7oIoZ54zNAM+ aTyQLGBZWOA4q0vaHkr8EIWwePSBPFMdv693hkgw= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Subject: [PATCH v2 09/12] test: timer-thread: Move timer start from wrong thread to separate test Date: Tue, 23 Jan 2024 03:12:46 +0200 Message-ID: <20240123011249.22716-10-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> References: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Starting a timer from the wrong thread is expected to fail, and we test this in the timer-thread unit test. This is however not something that a caller is allowed to do, and libcamera will get assertion failures to catch this invalid usage. The unit test will then fail. To prepare for this, split the unit test in two, with a test that is expected by meson to succeed, and one that is expected to fail. The assertion will then cause an expected failure, making the test suite succeed. Signed-off-by: Laurent Pinchart Reviewed-by: Milan Zamazal --- Changes since v1: - Improve comment explaining why the test is expected to fail --- test/meson.build | 9 ++-- test/timer-fail.cpp | 103 ++++++++++++++++++++++++++++++++++++++++++ test/timer-thread.cpp | 22 --------- 3 files changed, 109 insertions(+), 25 deletions(-) create mode 100644 test/timer-fail.cpp diff --git a/test/meson.build b/test/meson.build index 189e1428485a..8b6057d4e800 100644 --- a/test/meson.build +++ b/test/meson.build @@ -69,6 +69,7 @@ internal_tests = [ {'name': 'signal-threads', 'sources': ['signal-threads.cpp']}, {'name': 'threads', 'sources': 'threads.cpp', 'dependencies': [libthreads]}, {'name': 'timer', 'sources': ['timer.cpp']}, + {'name': 'timer-fail', 'sources': ['timer-fail.cpp'], 'should_fail': true}, {'name': 'timer-thread', 'sources': ['timer-thread.cpp']}, {'name': 'unique-fd', 'sources': ['unique-fd.cpp']}, {'name': 'utils', 'sources': ['utils.cpp']}, @@ -91,7 +92,7 @@ foreach test : public_tests link_with : test_libraries, include_directories : test_includes_public) - test(test['name'], exe) + test(test['name'], exe, should_fail : test.get('should_fail', false)) endforeach foreach test : internal_tests @@ -105,7 +106,7 @@ foreach test : internal_tests link_with : test_libraries, include_directories : test_includes_internal) - test(test['name'], exe) + test(test['name'], exe, should_fail : test.get('should_fail', false)) endforeach foreach test : internal_non_parallel_tests @@ -119,5 +120,7 @@ foreach test : internal_non_parallel_tests link_with : test_libraries, include_directories : test_includes_internal) - test(test['name'], exe, is_parallel : false) + test(test['name'], exe, + is_parallel : false, + should_fail : test.get('should_fail', false)) endforeach diff --git a/test/timer-fail.cpp b/test/timer-fail.cpp new file mode 100644 index 000000000000..2c622ca3410d --- /dev/null +++ b/test/timer-fail.cpp @@ -0,0 +1,103 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2024, Ideas on Board Oy + * + * timer-fail.cpp - Threaded timer failure test + */ + +#include +#include + +#include +#include +#include +#include + +#include "test.h" + +using namespace libcamera; +using namespace std; +using namespace std::chrono_literals; + +class TimeoutHandler : public Object +{ +public: + TimeoutHandler() + : timer_(this), timeout_(false) + { + timer_.timeout.connect(this, &TimeoutHandler::timeoutHandler); + } + + void start() + { + timer_.start(100ms); + } + + bool timeout() const + { + return timeout_; + } + +private: + void timeoutHandler() + { + timeout_ = true; + } + + Timer timer_; + bool timeout_; +}; + +class TimerFailTest : public Test +{ +protected: + int init() + { + thread_.start(); + timeout_.moveToThread(&thread_); + + return TestPass; + } + + int run() + { + /* + * Test that the forbidden operation of starting the timer from + * another thread results in a failure. We need to interrupt the + * event dispatcher to make sure we don't succeed simply because + * the event dispatcher hasn't noticed the timer restart. + */ + timeout_.start(); + thread_.eventDispatcher()->interrupt(); + + this_thread::sleep_for(chrono::milliseconds(200)); + + /* + * The wrong start() call should result in an assertion in debug + * builds, and a timeout in release builds. The test is + * therefore marked in meson.build as expected to fail. We need + * to return TestPass in the unexpected (usually known as + * "fail") case, and TestFail otherwise. + */ + if (timeout_.timeout()) { + cout << "Timer start from wrong thread succeeded unexpectedly" + << endl; + return TestPass; + } + + return TestFail; + } + + void cleanup() + { + /* Must stop thread before destroying timeout. */ + thread_.exit(0); + thread_.wait(); + } + +private: + TimeoutHandler timeout_; + Thread thread_; +}; + +TEST_REGISTER(TimerFailTest) diff --git a/test/timer-thread.cpp b/test/timer-thread.cpp index 0bcd0d8ce194..4caf4e33b2ba 100644 --- a/test/timer-thread.cpp +++ b/test/timer-thread.cpp @@ -29,12 +29,6 @@ public: timer_.start(100ms); } - void restart() - { - timeout_ = false; - timer_.start(100ms); - } - bool timeout() const { return timeout_; @@ -74,22 +68,6 @@ protected: return TestFail; } - /* - * Test that starting the timer from another thread fails. We - * need to interrupt the event dispatcher to make sure we don't - * succeed simply because the event dispatcher hasn't noticed - * the timer restart. - */ - timeout_.restart(); - thread_.eventDispatcher()->interrupt(); - - this_thread::sleep_for(chrono::milliseconds(200)); - - if (timeout_.timeout()) { - cout << "Timer restart test failed" << endl; - return TestFail; - } - return TestPass; } From patchwork Tue Jan 23 01:12:47 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 19458 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 737E1C32C3 for ; Tue, 23 Jan 2024 01:13:03 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 164F46294F; Tue, 23 Jan 2024 02:13:03 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="EPVvuIgQ"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 78A926297F for ; Tue, 23 Jan 2024 02:12:59 +0100 (CET) Received: from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi [89.27.53.110]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 23690E4; Tue, 23 Jan 2024 02:11:46 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1705972306; bh=4VK46g9gxjQEAtaVW9zeOvVEgCmYZf55aXihFgS9XTM=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=EPVvuIgQt+EMk2JlswtN83qGO3l2SNFSScizZ4lxxHXntCBmZaLNtBJsN5CzVv2Sk oVkCk5kNQ2J54/F1P1YTkCCsnPn/K2zUCr5dc2OJch6Tr+eL9BXwt8te5RBNB5ptYt YEZJIdNMxCGAcLTGfqUzX6Fms+O2qPuxzQeFaZm4= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Subject: [PATCH v2 10/12] test: timer-thread: Destroy Object from correct thread context Date: Tue, 23 Jan 2024 03:12:47 +0200 Message-ID: <20240123011249.22716-11-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> References: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" 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 Reviewed-by: Milan Zamazal --- 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 2c622ca3410d..82854b89630d 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)); @@ -79,7 +81,7 @@ protected: * to return 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; @@ -90,13 +92,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_; }; From patchwork Tue Jan 23 01:12:48 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 19459 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 66702C32C4 for ; Tue, 23 Jan 2024 01:13:05 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 9335A62955; Tue, 23 Jan 2024 02:13:04 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="SetXkB8W"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id B185362981 for ; Tue, 23 Jan 2024 02:13:00 +0100 (CET) Received: from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi [89.27.53.110]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 579661574; Tue, 23 Jan 2024 02:11:47 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1705972307; bh=yXzNLkAmP8fw3TYEP74DzvSAHQQJcmQkyhNtpFZydJM=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=SetXkB8Wb28GD/4sS1Cdl1D01jBoC5YQa6EcPKWV2b00sY9kUkAjM2IvgnHPUPKvU o941w1qyrt2fa6uzqCAGKy/y7donBWkmNG74JoWbsdCO9qEfCZfFu8T/5SZLaDGccq i8N6znhNobvgh3cAFNtz71VxrBHigoHmQwhnsGSo= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Subject: [PATCH v2 11/12] libcamera: object: Document and ensure Object deletion constraints Date: Tue, 23 Jan 2024 03:12:48 +0200 Message-ID: <20240123011249.22716-12-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> References: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Object instances are meant to be deleted from the thread they are bound to, and this requirement is documented. There are however exceptions to the rule, as threads may be stopped and restarted, with objects bound to them not being deleted and recreated for every stop/restart cycle. Bound objects may therefore need to be deleted after the thread has stopped, making it impossible to use Object::deleteLater(). Document the lifetime requirements more precisely, and enforce them with an assertion. Signed-off-by: Laurent Pinchart Reviewed-by: Milan Zamazal --- src/libcamera/base/object.cpp | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/libcamera/base/object.cpp b/src/libcamera/base/object.cpp index 14399d750e03..c6040fc6a78b 100644 --- a/src/libcamera/base/object.cpp +++ b/src/libcamera/base/object.cpp @@ -40,8 +40,9 @@ LOG_DEFINE_CATEGORY(Object) * Object class. * * Deleting an object from a thread other than the one the object is bound to is - * unsafe, unless the caller ensures that the object isn't processing any - * message concurrently. + * unsafe, unless the caller ensures that the object's thread is stopped and no + * parent or child of the object gets deleted concurrently. See + * Object::~Object() for more information. * * Object slots connected to signals will also run in the context of the * object's thread, regardless of whether the signal is emitted in the same or @@ -84,9 +85,20 @@ Object::Object(Object *parent) * Object instances shall be destroyed from the thread they are bound to, * otherwise undefined behaviour may occur. If deletion of an Object needs to * be scheduled from a different thread, deleteLater() shall be used. + * + * As an exception to this rule, Object instances may be deleted from a + * different thread if the thread the instance is bound to is stopped through + * the whole duration of the object's destruction, *and* the parent and children + * of the object do not get deleted concurrently. The caller is responsible for + * fulfilling those requirements. + * + * In all cases Object instances shall be deleted before the Thread they are + * bound to. */ Object::~Object() { + ASSERT(Thread::current() == thread_ || !thread_->isRunning()); + /* * Move signals to a private list to avoid concurrent iteration and * deletion of items from Signal::disconnect(). From patchwork Tue Jan 23 01:12:49 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 19460 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 7612FC324D for ; Tue, 23 Jan 2024 01:13:06 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 0A92562950; Tue, 23 Jan 2024 02:13:06 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="K5SE5yQB"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 0D65A62973 for ; Tue, 23 Jan 2024 02:13:02 +0100 (CET) Received: from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi [89.27.53.110]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id B6143188E; Tue, 23 Jan 2024 02:11:48 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1705972308; bh=vWwEaa5AmxWhUWEB93AXIdNgpOby95jk/FitnDlkli8=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=K5SE5yQBa/kX2VQviFIaUSEoaH3ZczUHzWDzF4bJYwx3xRCjHe5QW3ZbdK/Xj4FzM y8EJJ/wztUWK7HjR4Yr4I8p9SDPESUscpYLhhFVYYhToH89m+6AwdW7X4xODn6/440 2z+eqDOWrm50ttuaL0swETP0chQLjHJ3/vc2QT7I= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Subject: [PATCH v2 12/12] libcamera: object: Add and use thread-bound assertion Date: Tue, 23 Jan 2024 03:12:49 +0200 Message-ID: <20240123011249.22716-13-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> References: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Several functions in libcamera classes are marked as thread-bound, restricting the contexts in which those functions can be called. There is no infrastructure to enforce these restrictions, causing difficult to debug race conditions when they are not met by callers. As a first step to solve this, add an assertThreadBound() protected function to the Object class to test if the calling thread context is valid, and use it in member functions of Object subclasses marked as thread-bound. This replaces manual tests in a few locations. The thread-bound member functions of classes that do not inherit from Object are not checked, and neither are the functions of classes marked as thread-bound at the class level. These issue should be addressed in the future. Signed-off-by: Laurent Pinchart Reviewed-by: Milan Zamazal --- Changes since v1: - Fix typo in commit message --- include/libcamera/base/object.h | 2 ++ src/libcamera/base/event_notifier.cpp | 6 +++++ src/libcamera/base/object.cpp | 32 ++++++++++++++++++++++++++- src/libcamera/base/timer.cpp | 10 +++------ 4 files changed, 42 insertions(+), 8 deletions(-) diff --git a/include/libcamera/base/object.h b/include/libcamera/base/object.h index 933336361155..cb7e0a132be2 100644 --- a/include/libcamera/base/object.h +++ b/include/libcamera/base/object.h @@ -49,6 +49,8 @@ public: protected: virtual void message(Message *msg); + bool assertThreadBound(const char *message); + private: friend class SignalBase; friend class Thread; diff --git a/src/libcamera/base/event_notifier.cpp b/src/libcamera/base/event_notifier.cpp index fd93c0878c6f..a519aec38efb 100644 --- a/src/libcamera/base/event_notifier.cpp +++ b/src/libcamera/base/event_notifier.cpp @@ -8,6 +8,7 @@ #include #include +#include #include #include @@ -20,6 +21,8 @@ namespace libcamera { +LOG_DECLARE_CATEGORY(Event) + /** * \class EventNotifier * \brief Notify of activity on a file descriptor @@ -104,6 +107,9 @@ EventNotifier::~EventNotifier() */ void EventNotifier::setEnabled(bool enable) { + if (!assertThreadBound("EventNotifier can't be enabled from another thread")) + return; + if (enabled_ == enable) return; diff --git a/src/libcamera/base/object.cpp b/src/libcamera/base/object.cpp index c6040fc6a78b..81054b5833a3 100644 --- a/src/libcamera/base/object.cpp +++ b/src/libcamera/base/object.cpp @@ -225,6 +225,35 @@ void Object::message(Message *msg) } } +/** + * \fn Object::assertThreadBound() + * \brief Check if the caller complies with thread-bound constraints + * \param[in] message The message to be printed on error + * + * This function verifies the calling constraints required by the \threadbound + * definition. It shall be called at the beginning of member functions of an + * Object subclass that are explicitly marked as thread-bound in their + * documentation. + * + * If the thread-bound constraints are not met, the function prints \a message + * as an error message. For debug builds, it additionally causes an assertion + * error. + * + * \todo Verify the thread-bound requirements for functions marked as + * thread-bound at the class level. + * + * \return True if the call is thread-bound compliant, false otherwise + */ +bool Object::assertThreadBound(const char *message) +{ + if (Thread::current() == thread_) + return true; + + LOG(Object, Error) << message; + ASSERT(false); + return false; +} + /** * \fn R Object::invokeMethod() * \brief Invoke a method asynchronously on an Object instance @@ -276,7 +305,8 @@ void Object::message(Message *msg) */ void Object::moveToThread(Thread *thread) { - ASSERT(Thread::current() == thread_); + if (!assertThreadBound("Object can't be moved from another thread")) + return; if (thread_ == thread) return; diff --git a/src/libcamera/base/timer.cpp b/src/libcamera/base/timer.cpp index 74b060af32ff..24dbf1e892c3 100644 --- a/src/libcamera/base/timer.cpp +++ b/src/libcamera/base/timer.cpp @@ -85,10 +85,8 @@ void Timer::start(std::chrono::milliseconds duration) */ void Timer::start(std::chrono::steady_clock::time_point deadline) { - if (Thread::current() != thread()) { - LOG(Timer, Error) << "Timer " << this << " << can't be started from another thread"; + if (!assertThreadBound("Timer can't be started from another thread")) return; - } deadline_ = deadline; @@ -114,13 +112,11 @@ void Timer::start(std::chrono::steady_clock::time_point deadline) */ void Timer::stop() { - if (!isRunning()) + if (!assertThreadBound("Timer can't be stopped from another thread")) return; - if (Thread::current() != thread()) { - LOG(Timer, Error) << "Timer " << this << " can't be stopped from another thread"; + if (!isRunning()) return; - } unregisterTimer(); }