From patchwork Wed Nov 27 08:49:05 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2379 Return-Path: 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 156CB60BB8 for ; Wed, 27 Nov 2019 09:49:23 +0100 (CET) Received: from pendragon.tok.corp.google.com (unknown [104.132.253.101]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id F1C0B9F4 for ; Wed, 27 Nov 2019 09:49:21 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1574844562; bh=hu6mz7QBMbOxyC/hhxvc7fqXBXf/RNmYMr9lBCCW/Bk=; h=From:To:Subject:Date:From; b=ceunxkubhPAqyz7tYPLtPo/kRA0ghchLFVWVqKq9p2ySLiqec0CH0V+VeL4nPSGHp caQ6lsFSUld2tJSpnUiBaSlSsVq+3vRfX2mCeXi0ZrGObrVDNyYu7mxMkBWk6/sYaj NVKFny94T43o+KeXJ8mNYh0GU/hkxsbz0q1FOQsI= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 27 Nov 2019 10:49:05 +0200 Message-Id: <20191127084909.10612-1-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.23.0 MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 1/5] test: message: Fix message handling in MessageReceiver 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: , X-List-Received-Date: Wed, 27 Nov 2019 08:49:23 -0000 Forward messages that we don't handle to the base Object class, to avoid both blocking the ThreadMove message and mistaking it as the test message. Signed-off-by: Laurent Pinchart Reviewed-by: Jacopo Mondi Reviewed-by: Niklas Söderlund --- test/message.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/message.cpp b/test/message.cpp index 3775c30a20b3..cf21d5ca50d1 100644 --- a/test/message.cpp +++ b/test/message.cpp @@ -37,6 +37,11 @@ public: protected: void message(Message *msg) { + if (msg->type() != Message::None) { + Object::message(msg); + return; + } + if (thread() != Thread::current()) status_ = InvalidThread; else From patchwork Wed Nov 27 08:49:06 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2380 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 98A196136B for ; Wed, 27 Nov 2019 09:49:24 +0100 (CET) Received: from pendragon.tok.corp.google.com (unknown [104.132.253.101]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 5392E9F4 for ; Wed, 27 Nov 2019 09:49:23 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1574844564; bh=+tpnpNuEmXB3XPxl4jcSTh2xdJ+b2KJSVHjtw6pzFPQ=; h=From:To:Subject:Date:In-Reply-To:References:From; b=kwqSz6BCKtjtoObXiMJLJM9iDgB7Tfn11m6f9jVKtB5QJyFlwuNfSrZZ9iIYufzdo L8L6xtU3K/y0/l9Xxczop5d3/0D2AGGAP+ZC60DuEhL559t2T2CeWLHGu9gT0+hcdR nN2nwUtHEDe58s0HvBW4XCJr1L+c09bknaa/zZ3M= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 27 Nov 2019 10:49:06 +0200 Message-Id: <20191127084909.10612-2-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.23.0 In-Reply-To: <20191127084909.10612-1-laurent.pinchart@ideasonboard.com> References: <20191127084909.10612-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 2/5] test: message: Add slow receiver test 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: , X-List-Received-Date: Wed, 27 Nov 2019 08:49:25 -0000 There's a race in the message delivery against object deletion. Add a test that triggers it reliably. This test is expected to fail with an assertion error. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- test/message.cpp | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/test/message.cpp b/test/message.cpp index cf21d5ca50d1..7ebedb557502 100644 --- a/test/message.cpp +++ b/test/message.cpp @@ -52,6 +52,25 @@ private: Status status_; }; +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: @@ -88,6 +107,19 @@ 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(utils::make_unique(Message::None)); + + this_thread::sleep_for(chrono::milliseconds(10)); + + delete slowReceiver; + return TestPass; } From patchwork Wed Nov 27 08:49:07 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2381 Return-Path: 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 2C82160BB8 for ; Wed, 27 Nov 2019 09:49:26 +0100 (CET) Received: from pendragon.tok.corp.google.com (unknown [104.132.253.101]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id F3A269F4 for ; Wed, 27 Nov 2019 09:49:24 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1574844565; bh=XdmYqbeOJzAczJosIpJ/e4IR/lacPzAmhE0GxY0CLyg=; h=From:To:Subject:Date:In-Reply-To:References:From; b=riZS7kcV3m0k6MZZtfaaEQSpRixZzMCh+tTAxHy3KMJWJoW3tWVDi8HLvWxz8wXK+ 9ow2t8/ms/kuq0ZD+V5bWvSmPHOgmFdQJx5NNplx0pR55DHn5paKZa/zUxA/jAxzEz MEQWECWT6Rf3QYoIUukd6r9NH1HTcK3QN2bZ7RrU= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 27 Nov 2019 10:49:07 +0200 Message-Id: <20191127084909.10612-3-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.23.0 In-Reply-To: <20191127084909.10612-1-laurent.pinchart@ideasonboard.com> References: <20191127084909.10612-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 3/5] libcamera: thread: Fix race condition when dispatching messages 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: , X-List-Received-Date: Wed, 27 Nov 2019 08:49:26 -0000 The Object class stores the number of pending messages that have been posted for it, while the actual messages are stored in a per-thread list in the ThreadData class. When dispatching messages, the message is removed from the list, passed to the receiver, and the number of pending messages is updated. In order to avoid too much contention on the list lock, the lock is dropped to pass the message to the receiver and then taken again. This creates a race condition window with Thread::removeMessages(), as the number of pending messages for a receiver is briefly out of sync with the pending messages list. The assertion at the end of removeMessages() thus sometimes fails. Fix this by decrementing the pending messages counter before releasing the lock in Thread::dispatchMessages(). This fixes the slow message receiver test in MessageTest. Fixes: 01b930964acd ("libcamera: thread: Add a messaging passing API") Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- src/libcamera/thread.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp index 029a0e8fddd5..18ebd16a7e2f 100644 --- a/src/libcamera/thread.cpp +++ b/src/libcamera/thread.cpp @@ -439,11 +439,11 @@ void Thread::dispatchMessages() Object *receiver = msg->receiver_; ASSERT(data_ == receiver->thread()->data_); + receiver->pendingMessages_--; + locker.unlock(); receiver->message(msg.get()); locker.lock(); - - receiver->pendingMessages_--; } } From patchwork Wed Nov 27 08:49:08 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2382 Return-Path: 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 B51D06136B for ; Wed, 27 Nov 2019 09:49:27 +0100 (CET) Received: from pendragon.tok.corp.google.com (unknown [104.132.253.101]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id B7044A4B for ; Wed, 27 Nov 2019 09:49:26 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1574844567; bh=X15X8SXF65v7Cq9Gi4Wh3gw23f0cVIhwEcOEFKzQuM4=; h=From:To:Subject:Date:In-Reply-To:References:From; b=uL4853olEttDQthx8keJxYBG3jO06AHUHJbegiQyQSci4h/E+QVdOgOlEyUUIb2LV 8dipTLyOX3arGX0ajkYcpqWI4u0+b4y11b5363QuaK4l+QBrygU8yIr6woWW3EvkFO b9Og15dFKr0YgBAN8Ap6GGYtRJPiYoWZgq+2UpZM= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 27 Nov 2019 10:49:08 +0200 Message-Id: <20191127084909.10612-4-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.23.0 In-Reply-To: <20191127084909.10612-1-laurent.pinchart@ideasonboard.com> References: <20191127084909.10612-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 4/5] libcamera: object: Document danger of deleting object from other thread 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: , X-List-Received-Date: Wed, 27 Nov 2019 08:49:28 -0000 Object instances receive messages dispatched from the event loop of the thread they belong to. Deleting an object from a different thread is thus dangerous, unless the caller ensures that no message delivery is in progress. Document this in the Object class documentation. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- src/libcamera/object.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp index 1f787271f782..e76faf48b8ed 100644 --- a/src/libcamera/object.cpp +++ b/src/libcamera/object.cpp @@ -40,6 +40,10 @@ LOG_DEFINE_CATEGORY(Object) * implementing easy message passing between threads by inheriting from the * 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. + * * 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 * in another thread. From patchwork Wed Nov 27 08:49:09 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 2383 Return-Path: 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 3AFD161C5C for ; Wed, 27 Nov 2019 09:49:29 +0100 (CET) Received: from pendragon.tok.corp.google.com (unknown [104.132.253.101]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 154419F4 for ; Wed, 27 Nov 2019 09:49:27 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1574844569; bh=dggPb64g4G4E36RnOUlFO9IBTdilVFJ4kqh4Q74w8AU=; h=From:To:Subject:Date:In-Reply-To:References:From; b=RvOa/w7ogY9Fu6UF7KtMSBgg6gYsIpPE8LJ21HE9Lh2x0HEDShAwcec0m7Ex8U+2I uwGGO0ucuqvod6FEUiM17P7ldbuWpgl5I3hVAjJ+TwcZAj0jyTXWKyF/JZmKn8H7J0 +XfUBMocG8qurtfru7NSkDGNAKpKXamhJkm8Kp74= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Wed, 27 Nov 2019 10:49:09 +0200 Message-Id: <20191127084909.10612-5-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.23.0 In-Reply-To: <20191127084909.10612-1-laurent.pinchart@ideasonboard.com> References: <20191127084909.10612-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 5/5] test: object-invoke: Delete InvokeObject after thread termination 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: , X-List-Received-Date: Wed, 27 Nov 2019 08:49:29 -0000 The InvokeObject instance is created on the stack in the run() method, and is thus destroyed before the thread the object is bound to terminates. This creates a race condition as the object message handler could be running when the object is deleted. Fix this by moving the InvokeObject to a member field. Signed-off-by: Laurent Pinchart Reviewed-by: Niklas Söderlund --- test/object-invoke.cpp | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/test/object-invoke.cpp b/test/object-invoke.cpp index 6582fa75ae11..560adee14e3a 100644 --- a/test/object-invoke.cpp +++ b/test/object-invoke.cpp @@ -60,23 +60,22 @@ protected: int run() { EventDispatcher *dispatcher = Thread::current()->eventDispatcher(); - InvokedObject object; /* * Test that queued method invocation in the same thread goes * through the event dispatcher. */ - object.invokeMethod(&InvokedObject::method, - ConnectionTypeQueued, 42); + object_.invokeMethod(&InvokedObject::method, + ConnectionTypeQueued, 42); - if (object.status() != InvokedObject::NoCall) { + if (object_.status() != InvokedObject::NoCall) { cerr << "Method not invoked asynchronously" << endl; return TestFail; } dispatcher->processEvents(); - switch (object.status()) { + switch (object_.status()) { case InvokedObject::NoCall: cout << "Method not invoked for main thread" << endl; return TestFail; @@ -87,7 +86,7 @@ protected: break; } - if (object.value() != 42) { + if (object_.value() != 42) { cout << "Method invoked with incorrect value for main thread" << endl; return TestFail; } @@ -96,15 +95,15 @@ protected: * Move the object to a thread and verify that auto method * invocation is delivered in the correct thread. */ - object.reset(); - object.moveToThread(&thread_); + object_.reset(); + object_.moveToThread(&thread_); thread_.start(); - object.invokeMethod(&InvokedObject::method, - ConnectionTypeBlocking, 42); + object_.invokeMethod(&InvokedObject::method, + ConnectionTypeBlocking, 42); - switch (object.status()) { + switch (object_.status()) { case InvokedObject::NoCall: cout << "Method not invoked for custom thread" << endl; return TestFail; @@ -115,7 +114,7 @@ protected: break; } - if (object.value() != 42) { + if (object_.value() != 42) { cout << "Method invoked with incorrect value for custom thread" << endl; return TestFail; } @@ -131,6 +130,7 @@ protected: private: Thread thread_; + InvokedObject object_; }; TEST_REGISTER(ObjectInvokeTest)