[{"id":17129,"web_url":"https://patchwork.libcamera.org/comment/17129/","msgid":"","date":"2021-05-23T06:47:10","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: thread: Document race\n\tcondition at stop time","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2021-05-23 05:31:55 +0300, Laurent Pinchart wrote:\n> When a thread stops, messages may be left in its message queue. Document\n> this in details, with a way to force processing of pending messages when\n> the thread is stopped.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Niklas Söderlund \n\n> ---\n> src/libcamera/object.cpp | 8 ++++++++\n> src/libcamera/thread.cpp | 44 ++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 52 insertions(+)\n> \n> diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp\n> index cd83c684b989..5e6b73f9af84 100644\n> --- a/src/libcamera/object.cpp\n> +++ b/src/libcamera/object.cpp\n> @@ -155,6 +155,10 @@ void Object::deleteLater()\n> * running its event loop the message will not be delivered until the event\n> * loop gets started.\n> *\n> + * Due to their asynchronous nature, threads do not provide any guarantee that\n> + * all posted messages are delivered before the thread is stopped. See\n> + * \\ref thread-stop for additional information.\n> + *\n> * \\context This function is \\threadsafe.\n> */\n> void Object::postMessage(std::unique_ptr msg)\n> @@ -212,6 +216,10 @@ void Object::message(Message *msg)\n> * are passed untouched. The caller shall ensure that any pointer argument\n> * remains valid until the method is invoked.\n> *\n> + * Due to the asynchronous nature of threads, functions invoked asynchronously\n> + * with the ConnectionTypeQueued type are not guaranteed to be called before\n> + * the thread is stopped. See \\ref thread-stop for additional information.\n> + *\n> * \\context This function is \\threadsafe.\n> *\n> * \\return For connection types ConnectionTypeDirect and\n> diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp\n> index d59e43966d26..91e4737ad032 100644\n> --- a/src/libcamera/thread.cpp\n> +++ b/src/libcamera/thread.cpp\n> @@ -221,6 +221,47 @@ ThreadData *ThreadData::current()\n> * called. The event loop dispatches events (messages, notifiers and timers)\n> * sent to the objects living in the thread. This behaviour can be modified by\n> * overriding the run() function.\n> + *\n> + * \\section thread-stop Stopping Threads\n> + *\n> + * Threads can't be forcibly stopped. Instead, a thread user first requests the\n> + * thread to exit and then waits for the thread's main function to react to the\n> + * request and return, at which points the thread will stop.\n> + *\n> + * For threads running exec(), the exit() function is used to request the thread\n> + * to exit. For threads subclassing the Thread class and implementing a custom\n> + * run() function, a subclass-specific mechanism shall be provided. In either\n> + * case, the wait() function shall be called to wait for the thread to stop.\n> + *\n> + * Due to their asynchronous nature, threads are subject to race conditions when\n> + * they stop. This is of particular importance for messages posted to the thread\n> + * with postMessage() (and the other mechanisms that rely on it, such as\n> + * Object::invokeMethod() or asynchronous signal delivery). To understand the\n> + * issues, three contexts need to be considered:\n> + *\n> + * - The worker is the Thread performing work and being instructed to stop.\n> + * - The controller is the context which instructs the worker thread to stop.\n> + * - The other contexts are any threads other than the worker and controller\n> + * that interact with the worker thread.\n> + *\n> + * Messages posted to the worker thread from the controller context before\n> + * calling exit() are queued to the thread's message queue, and the Thread class\n> + * offers no guarantee that those messages will be processed before the thread\n> + * stops. This allows threads to stop fast.\n> + *\n> + * A thread that requires delivery of messages posted from the controller\n> + * context before exit() should reimplement the run() function and call\n> + * dispatchMessages() after exec().\n> + *\n> + * Messages posted to the worker thread from the other contexts are asynchronous\n> + * with respect to the exit() call from the controller context. There is no\n> + * guarantee as to whether those messages will be processed or not before the\n> + * thread stops.\n> + *\n> + * Messages that are not processed will stay in the queue, in the exact same way\n> + * as messages posted after the thread has stopped. They will be processed when\n> + * the thread is restarted. If the thread is never restarted, they will be\n> + * deleted without being processed when the Thread instance is destroyed.\n> */\n> \n> /**\n> @@ -480,6 +521,9 @@ EventDispatcher *Thread::eventDispatcher()\n> * running its event loop the message will not be delivered until the event\n> * loop gets started.\n> *\n> + * When the thread is stopped, posted messages may not have all been processed.\n> + * See \\ref thread-stop for additional information.\n> + *\n> * If the \\a receiver is not bound to this thread the behaviour is undefined.\n> *\n> * \\sa exec()\n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 725C1C3200\n\tfor ;\n\tSun, 23 May 2021 06:47:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B24146891A;\n\tSun, 23 May 2021 08:47:13 +0200 (CEST)","from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com\n\t[IPv6:2a00:1450:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3A3E4602AE\n\tfor ;\n\tSun, 23 May 2021 08:47:12 +0200 (CEST)","by mail-lj1-x22f.google.com with SMTP id w7so15858195lji.6\n\tfor ;\n\tSat, 22 May 2021 23:47:12 -0700 (PDT)","from localhost (h-155-4-209-203.A463.priv.bahnhof.se.\n\t[155.4.209.203]) by smtp.gmail.com with ESMTPSA id\n\tu13sm1256407ljk.133.2021.05.22.23.47.11\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 22 May 2021 23:47:11 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"O23MY0hq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=ELpoBj48xikmfbgGBCmDuzW0iYYTTezDR0JAzaYqBhI=;\n\tb=O23MY0hqhxalqlv5P+OCz2n0aDJrAHu7mioGy8lqtAB0JrUxqUwiWsxXUnOp2JrDpR\n\tD9VYBHYR6oF/mLwc+keXH/z9DNF4wKGJz6OaGVbp41a6mBUHU0m9WLGEWdSIMWt9IzGv\n\tAuai6+DtLOonNCfHYNrzVCnHnoQdpn3u/K9vke/15VhSprMY+aNLYgg4I8iqyBwCd/YU\n\t4qPfW9jOijZTdE3mR43jpP6pt54bsFRZKJgDjdTlh/NsuMMp3UJUkR3gzbwZD/sF+IBt\n\tImU8HUFQMxTD9oX6KB7OoZciqmV+0dV5VnbdPeREVVL4qzu3e0mNTm+O4cra9BMK5yWD\n\tsCJw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=ELpoBj48xikmfbgGBCmDuzW0iYYTTezDR0JAzaYqBhI=;\n\tb=Gt+L/Qw5Q1nHxEZ/BrvAyPtWTuptH2cpmh200BiQlbFT7d15EPc/YJdXnqwLk3Zqcp\n\tGVANnTLitYldYYtwbpkm2TYOZpa1B/HH9azoIQGS1m8eDXq9maSOGtM0AJboeIovFlXO\n\tnU+snmyp/koDXrVH7qJli9KvEurO1cePtUeRK1IOJ8ixN1O2Eu0bAW/8zzWLZIG1tzHu\n\tSdhjpGwKKJY41kfng7M7cGgoMZBFKen/iafsI9Y5bnwQmVGYGAtkqOGEcAvwmIThgCso\n\taW9okdISZ1esP6snV/iemPHsr26e+OPH9BpoFrBttRH+QqU/5GLxv5O0Okc0KUeT8Bwp\n\t4eMg==","X-Gm-Message-State":"AOAM5309xaNDxikVaV++ah0ANXLEH/fdofZYpP+cC4Esl1qCOvmNKULF\n\taGAIQLpdaXvzWYtyseXuPYj+kSbFwxIYetTa","X-Google-Smtp-Source":"ABdhPJyLWlecJ2PIN84L79KoIwvkN4SdSg6AKp/jhDvXdv528HTAuLglROwQErgYcJ3B22MNDWsvtA==","X-Received":"by 2002:a05:651c:210f:: with SMTP id\n\ta15mr11980457ljq.160.1621752431625; \n\tSat, 22 May 2021 23:47:11 -0700 (PDT)","Date":"Sun, 23 May 2021 08:47:10 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"","References":"<20210523023155.20268-1-laurent.pinchart@ideasonboard.com>\n\t<20210523023155.20268-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210523023155.20268-5-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: thread: Document race\n\tcondition at stop time","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17160,"web_url":"https://patchwork.libcamera.org/comment/17160/","msgid":"<9c760ca1-f553-665d-2e63-be785035d9e0@ideasonboard.com>","date":"2021-05-24T03:17:00","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: thread: Document race\n\tcondition at stop time","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThanks for the work.\n\nGreat to read the contexts for message deliveries you have mentioned. \nHowever, how does one decide whether they need to ensure message \ndelivery before exit() ? What would be the applicable use-cases to that \ncontext? (Just curious)\n\nOn 5/23/21 8:01 AM, Laurent Pinchart wrote:\n> When a thread stops, messages may be left in its message queue. Document\n> this in details, with a way to force processing of pending messages when\n> the thread is stopped.\n>\n> Signed-off-by: Laurent Pinchart \nReviewed-by: Umang Jain \n> ---\n> src/libcamera/object.cpp | 8 ++++++++\n> src/libcamera/thread.cpp | 44 ++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 52 insertions(+)\n>\n> diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp\n> index cd83c684b989..5e6b73f9af84 100644\n> --- a/src/libcamera/object.cpp\n> +++ b/src/libcamera/object.cpp\n> @@ -155,6 +155,10 @@ void Object::deleteLater()\n> * running its event loop the message will not be delivered until the event\n> * loop gets started.\n> *\n> + * Due to their asynchronous nature, threads do not provide any guarantee that\n> + * all posted messages are delivered before the thread is stopped. See\n> + * \\ref thread-stop for additional information.\n> + *\n> * \\context This function is \\threadsafe.\n> */\n> void Object::postMessage(std::unique_ptr msg)\n> @@ -212,6 +216,10 @@ void Object::message(Message *msg)\n> * are passed untouched. The caller shall ensure that any pointer argument\n> * remains valid until the method is invoked.\n> *\n> + * Due to the asynchronous nature of threads, functions invoked asynchronously\n> + * with the ConnectionTypeQueued type are not guaranteed to be called before\n> + * the thread is stopped. See \\ref thread-stop for additional information.\n> + *\n> * \\context This function is \\threadsafe.\n> *\n> * \\return For connection types ConnectionTypeDirect and\n> diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp\n> index d59e43966d26..91e4737ad032 100644\n> --- a/src/libcamera/thread.cpp\n> +++ b/src/libcamera/thread.cpp\n> @@ -221,6 +221,47 @@ ThreadData *ThreadData::current()\n> * called. The event loop dispatches events (messages, notifiers and timers)\n> * sent to the objects living in the thread. This behaviour can be modified by\n> * overriding the run() function.\n> + *\n> + * \\section thread-stop Stopping Threads\n> + *\n> + * Threads can't be forcibly stopped. Instead, a thread user first requests the\n> + * thread to exit and then waits for the thread's main function to react to the\n> + * request and return, at which points the thread will stop.\n> + *\n> + * For threads running exec(), the exit() function is used to request the thread\n> + * to exit. For threads subclassing the Thread class and implementing a custom\n> + * run() function, a subclass-specific mechanism shall be provided. In either\n> + * case, the wait() function shall be called to wait for the thread to stop.\n> + *\n> + * Due to their asynchronous nature, threads are subject to race conditions when\n> + * they stop. This is of particular importance for messages posted to the thread\n> + * with postMessage() (and the other mechanisms that rely on it, such as\n> + * Object::invokeMethod() or asynchronous signal delivery). To understand the\n> + * issues, three contexts need to be considered:\n> + *\n> + * - The worker is the Thread performing work and being instructed to stop.\n> + * - The controller is the context which instructs the worker thread to stop.\n> + * - The other contexts are any threads other than the worker and controller\n> + * that interact with the worker thread.\n> + *\n> + * Messages posted to the worker thread from the controller context before\n> + * calling exit() are queued to the thread's message queue, and the Thread class\n> + * offers no guarantee that those messages will be processed before the thread\n> + * stops. This allows threads to stop fast.\n> + *\n> + * A thread that requires delivery of messages posted from the controller\n> + * context before exit() should reimplement the run() function and call\n> + * dispatchMessages() after exec().\n> + *\n> + * Messages posted to the worker thread from the other contexts are asynchronous\n> + * with respect to the exit() call from the controller context. There is no\n> + * guarantee as to whether those messages will be processed or not before the\n> + * thread stops.\n> + *\n> + * Messages that are not processed will stay in the queue, in the exact same way\n> + * as messages posted after the thread has stopped. They will be processed when\n> + * the thread is restarted. If the thread is never restarted, they will be\n> + * deleted without being processed when the Thread instance is destroyed.\n> */\n> \n> /**\n> @@ -480,6 +521,9 @@ EventDispatcher *Thread::eventDispatcher()\n> * running its event loop the message will not be delivered until the event\n> * loop gets started.\n> *\n> + * When the thread is stopped, posted messages may not have all been processed.\n> + * See \\ref thread-stop for additional information.\n> + *\n> * If the \\a receiver is not bound to this thread the behaviour is undefined.\n> *\n> * \\sa exec()","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 537A5C3200\n\tfor ;\n\tMon, 24 May 2021 03:17:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5592A6891B;\n\tMon, 24 May 2021 05:17:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0EBF5602AD\n\tfor ;\n\tMon, 24 May 2021 05:17:06 +0200 (CEST)","from localhost.localdomain (unknown [103.251.226.203])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EF367476;\n\tMon, 24 May 2021 05:17:04 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"wa5Ee2rf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621826225;\n\tbh=E+baH3Aemejv+n6Fj0rjGgbEBUZ3BjwABWqCFTPKD38=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=wa5Ee2rfdqa2H08KNapW+EtKwuKl5vk1AEglG0xwiKOYOz9dsMZkLTzeNMNMPNpd3\n\taX4i4QTyN4+3U5hI2RlvByIN7iD5d6C4SjJAwMXa2kIPYLCTaZEzonau79AdX86ywL\n\t0CmYHK3iEo4AbyaZ6DFomYHjNPVa7KQor6uhfU0g=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210523023155.20268-1-laurent.pinchart@ideasonboard.com>\n\t<20210523023155.20268-5-laurent.pinchart@ideasonboard.com>","From":"Umang Jain ","Message-ID":"<9c760ca1-f553-665d-2e63-be785035d9e0@ideasonboard.com>","Date":"Mon, 24 May 2021 08:47:00 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.11.0","MIME-Version":"1.0","In-Reply-To":"<20210523023155.20268-5-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: thread: Document race\n\tcondition at stop time","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17161,"web_url":"https://patchwork.libcamera.org/comment/17161/","msgid":"","date":"2021-05-24T03:44:18","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: thread: Document race\n\tcondition at stop time","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Mon, May 24, 2021 at 08:47:00AM +0530, Umang Jain wrote:\n> Hi Laurent,\n> \n> Thanks for the work.\n> \n> Great to read the contexts for message deliveries you have mentioned. \n> However, how does one decide whether they need to ensure message \n> delivery before exit() ? What would be the applicable use-cases to that \n> context? (Just curious)\n\nIt depends if the user of the thread needs a guarantee that all messages\nwill be processed. That doesn't really answer your question...\n\nLet's take a practical example, posting post-processing work to a thread\n(which we should do quite soon, JPEG compression in the request\ncompletion handler isn't a good idea). When stopping the camera (HAL\n.close() or .flush()), if the HAL required for proper operation that the\nthread reports completion of all post-processing requests, then we'd\nneed to ensure message delivery. On the other hand, the HAL could also\nstop the thread, and then look into its own queue of requests that are\nwaiting for post-processing. Any request in that queue would then be\nconsidered as cancelled are reported as such to the camera service. In\nthat case, we wouldn't require the thread to process all posted\nmessages.\n\n> On 5/23/21 8:01 AM, Laurent Pinchart wrote:\n> > When a thread stops, messages may be left in its message queue. Document\n> > this in details, with a way to force processing of pending messages when\n> > the thread is stopped.\n> >\n> > Signed-off-by: Laurent Pinchart \n>\n> Reviewed-by: Umang Jain \n>\n> > ---\n> > src/libcamera/object.cpp | 8 ++++++++\n> > src/libcamera/thread.cpp | 44 ++++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 52 insertions(+)\n> >\n> > diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp\n> > index cd83c684b989..5e6b73f9af84 100644\n> > --- a/src/libcamera/object.cpp\n> > +++ b/src/libcamera/object.cpp\n> > @@ -155,6 +155,10 @@ void Object::deleteLater()\n> > * running its event loop the message will not be delivered until the event\n> > * loop gets started.\n> > *\n> > + * Due to their asynchronous nature, threads do not provide any guarantee that\n> > + * all posted messages are delivered before the thread is stopped. See\n> > + * \\ref thread-stop for additional information.\n> > + *\n> > * \\context This function is \\threadsafe.\n> > */\n> > void Object::postMessage(std::unique_ptr msg)\n> > @@ -212,6 +216,10 @@ void Object::message(Message *msg)\n> > * are passed untouched. The caller shall ensure that any pointer argument\n> > * remains valid until the method is invoked.\n> > *\n> > + * Due to the asynchronous nature of threads, functions invoked asynchronously\n> > + * with the ConnectionTypeQueued type are not guaranteed to be called before\n> > + * the thread is stopped. See \\ref thread-stop for additional information.\n> > + *\n> > * \\context This function is \\threadsafe.\n> > *\n> > * \\return For connection types ConnectionTypeDirect and\n> > diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp\n> > index d59e43966d26..91e4737ad032 100644\n> > --- a/src/libcamera/thread.cpp\n> > +++ b/src/libcamera/thread.cpp\n> > @@ -221,6 +221,47 @@ ThreadData *ThreadData::current()\n> > * called. The event loop dispatches events (messages, notifiers and timers)\n> > * sent to the objects living in the thread. This behaviour can be modified by\n> > * overriding the run() function.\n> > + *\n> > + * \\section thread-stop Stopping Threads\n> > + *\n> > + * Threads can't be forcibly stopped. Instead, a thread user first requests the\n> > + * thread to exit and then waits for the thread's main function to react to the\n> > + * request and return, at which points the thread will stop.\n> > + *\n> > + * For threads running exec(), the exit() function is used to request the thread\n> > + * to exit. For threads subclassing the Thread class and implementing a custom\n> > + * run() function, a subclass-specific mechanism shall be provided. In either\n> > + * case, the wait() function shall be called to wait for the thread to stop.\n> > + *\n> > + * Due to their asynchronous nature, threads are subject to race conditions when\n> > + * they stop. This is of particular importance for messages posted to the thread\n> > + * with postMessage() (and the other mechanisms that rely on it, such as\n> > + * Object::invokeMethod() or asynchronous signal delivery). To understand the\n> > + * issues, three contexts need to be considered:\n> > + *\n> > + * - The worker is the Thread performing work and being instructed to stop.\n> > + * - The controller is the context which instructs the worker thread to stop.\n> > + * - The other contexts are any threads other than the worker and controller\n> > + * that interact with the worker thread.\n> > + *\n> > + * Messages posted to the worker thread from the controller context before\n> > + * calling exit() are queued to the thread's message queue, and the Thread class\n> > + * offers no guarantee that those messages will be processed before the thread\n> > + * stops. This allows threads to stop fast.\n> > + *\n> > + * A thread that requires delivery of messages posted from the controller\n> > + * context before exit() should reimplement the run() function and call\n> > + * dispatchMessages() after exec().\n> > + *\n> > + * Messages posted to the worker thread from the other contexts are asynchronous\n> > + * with respect to the exit() call from the controller context. There is no\n> > + * guarantee as to whether those messages will be processed or not before the\n> > + * thread stops.\n> > + *\n> > + * Messages that are not processed will stay in the queue, in the exact same way\n> > + * as messages posted after the thread has stopped. They will be processed when\n> > + * the thread is restarted. If the thread is never restarted, they will be\n> > + * deleted without being processed when the Thread instance is destroyed.\n> > */\n> > \n> > /**\n> > @@ -480,6 +521,9 @@ EventDispatcher *Thread::eventDispatcher()\n> > * running its event loop the message will not be delivered until the event\n> > * loop gets started.\n> > *\n> > + * When the thread is stopped, posted messages may not have all been processed.\n> > + * See \\ref thread-stop for additional information.\n> > + *\n> > * If the \\a receiver is not bound to this thread the behaviour is undefined.\n> > *\n> > * \\sa exec()","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CBE78C3201\n\tfor ;\n\tMon, 24 May 2021 03:44:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 21D9C6891B;\n\tMon, 24 May 2021 05:44:23 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE012602AD\n\tfor ;\n\tMon, 24 May 2021 05:44:21 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 21615476;\n\tMon, 24 May 2021 05:44:21 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kfvxogkA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621827861;\n\tbh=nf0GYB6Rp3f/izrIl+RXT5FLL4b/OgMfqKUXZ5sV0IM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kfvxogkACBr13ifKW0Uk725yyHLI8AwuC7X2GczMdeud8k7sSXTbYLB9adodhE+j0\n\tRAsg8iKrG1GqnUpgosJWqIUnB+ei5v/jHy9TUCbSL8UGWWO/XCruXWNP6lHc5kJBjo\n\tsHNZ3X8lO3qI71W6dWulJxnjIgNZXc7juzGDpPiI=","Date":"Mon, 24 May 2021 06:44:18 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210523023155.20268-1-laurent.pinchart@ideasonboard.com>\n\t<20210523023155.20268-5-laurent.pinchart@ideasonboard.com>\n\t<9c760ca1-f553-665d-2e63-be785035d9e0@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<9c760ca1-f553-665d-2e63-be785035d9e0@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: thread: Document race\n\tcondition at stop time","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17173,"web_url":"https://patchwork.libcamera.org/comment/17173/","msgid":"","date":"2021-05-24T06:19:24","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: thread: Document race\n\tcondition at stop time","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for the patch.\n\nOn Mon, May 24, 2021 at 12:44 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Umang,\n>\n> On Mon, May 24, 2021 at 08:47:00AM +0530, Umang Jain wrote:\n> > Hi Laurent,\n> >\n> > Thanks for the work.\n> >\n> > Great to read the contexts for message deliveries you have mentioned.\n> > However, how does one decide whether they need to ensure message\n> > delivery before exit() ? What would be the applicable use-cases to that\n> > context? (Just curious)\n>\n> It depends if the user of the thread needs a guarantee that all messages\n> will be processed. That doesn't really answer your question...\n>\n> Let's take a practical example, posting post-processing work to a thread\n> (which we should do quite soon, JPEG compression in the request\n> completion handler isn't a good idea). When stopping the camera (HAL\n> .close() or .flush()), if the HAL required for proper operation that the\n> thread reports completion of all post-processing requests, then we'd\n> need to ensure message delivery. On the other hand, the HAL could also\n> stop the thread, and then look into its own queue of requests that are\n> waiting for post-processing. Any request in that queue would then be\n> considered as cancelled are reported as such to the camera service. In\n> that case, we wouldn't require the thread to process all posted\n> messages.\n>\n> > On 5/23/21 8:01 AM, Laurent Pinchart wrote:\n> > > When a thread stops, messages may be left in its message queue.\n> Document\n> > > this in details, with a way to force processing of pending messages\n> when\n> > > the thread is stopped.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> >\n> > Reviewed-by: Umang Jain \n> >\n>\n\nReviewed-by: Hirokazu Honda \n\n\n> > > ---\n> > > src/libcamera/object.cpp | 8 ++++++++\n> > > src/libcamera/thread.cpp | 44\n> ++++++++++++++++++++++++++++++++++++++++\n> > > 2 files changed, 52 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp\n> > > index cd83c684b989..5e6b73f9af84 100644\n> > > --- a/src/libcamera/object.cpp\n> > > +++ b/src/libcamera/object.cpp\n> > > @@ -155,6 +155,10 @@ void Object::deleteLater()\n> > > * running its event loop the message will not be delivered until\n> the event\n> > > * loop gets started.\n> > > *\n> > > + * Due to their asynchronous nature, threads do not provide any\n> guarantee that\n> > > + * all posted messages are delivered before the thread is stopped. See\n> > > + * \\ref thread-stop for additional information.\n> > > + *\n> > > * \\context This function is \\threadsafe.\n> > > */\n> > > void Object::postMessage(std::unique_ptr msg)\n> > > @@ -212,6 +216,10 @@ void Object::message(Message *msg)\n> > > * are passed untouched. The caller shall ensure that any pointer\n> argument\n> > > * remains valid until the method is invoked.\n> > > *\n> > > + * Due to the asynchronous nature of threads, functions invoked\n> asynchronously\n> > > + * with the ConnectionTypeQueued type are not guaranteed to be called\n> before\n> > > + * the thread is stopped. See \\ref thread-stop for additional\n> information.\n> > > + *\n> > > * \\context This function is \\threadsafe.\n> > > *\n> > > * \\return For connection types ConnectionTypeDirect and\n> > > diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp\n> > > index d59e43966d26..91e4737ad032 100644\n> > > --- a/src/libcamera/thread.cpp\n> > > +++ b/src/libcamera/thread.cpp\n> > > @@ -221,6 +221,47 @@ ThreadData *ThreadData::current()\n> > > * called. The event loop dispatches events (messages, notifiers and\n> timers)\n> > > * sent to the objects living in the thread. This behaviour can be\n> modified by\n> > > * overriding the run() function.\n> > > + *\n> > > + * \\section thread-stop Stopping Threads\n> > > + *\n> > > + * Threads can't be forcibly stopped. Instead, a thread user first\n> requests the\n> > > + * thread to exit and then waits for the thread's main function to\n> react to the\n> > > + * request and return, at which points the thread will stop.\n> > > + *\n> > > + * For threads running exec(), the exit() function is used to request\n> the thread\n> > > + * to exit. For threads subclassing the Thread class and implementing\n> a custom\n> > > + * run() function, a subclass-specific mechanism shall be provided.\n> In either\n> > > + * case, the wait() function shall be called to wait for the thread\n> to stop.\n> > > + *\n> > > + * Due to their asynchronous nature, threads are subject to race\n> conditions when\n> > > + * they stop. This is of particular importance for messages posted to\n> the thread\n> > > + * with postMessage() (and the other mechanisms that rely on it, such\n> as\n> > > + * Object::invokeMethod() or asynchronous signal delivery). To\n> understand the\n> > > + * issues, three contexts need to be considered:\n> > > + *\n> > > + * - The worker is the Thread performing work and being instructed to\n> stop.\n> > > + * - The controller is the context which instructs the worker thread\n> to stop.\n> > > + * - The other contexts are any threads other than the worker and\n> controller\n> > > + * that interact with the worker thread.\n> > > + *\n> > > + * Messages posted to the worker thread from the controller context\n> before\n> > > + * calling exit() are queued to the thread's message queue, and the\n> Thread class\n> > > + * offers no guarantee that those messages will be processed before\n> the thread\n> > > + * stops. This allows threads to stop fast.\n> > > + *\n> > > + * A thread that requires delivery of messages posted from the\n> controller\n> > > + * context before exit() should reimplement the run() function and\n> call\n> > > + * dispatchMessages() after exec().\n> > > + *\n> > > + * Messages posted to the worker thread from the other contexts are\n> asynchronous\n> > > + * with respect to the exit() call from the controller context. There\n> is no\n> > > + * guarantee as to whether those messages will be processed or not\n> before the\n> > > + * thread stops.\n> > > + *\n> > > + * Messages that are not processed will stay in the queue, in the\n> exact same way\n> > > + * as messages posted after the thread has stopped. They will be\n> processed when\n> > > + * the thread is restarted. If the thread is never restarted, they\n> will be\n> > > + * deleted without being processed when the Thread instance is\n> destroyed.\n> > > */\n> > >\n> > > /**\n> > > @@ -480,6 +521,9 @@ EventDispatcher *Thread::eventDispatcher()\n> > > * running its event loop the message will not be delivered until\n> the event\n> > > * loop gets started.\n> > > *\n> > > + * When the thread is stopped, posted messages may not have all been\n> processed.\n> > > + * See \\ref thread-stop for additional information.\n> > > + *\n> > > * If the \\a receiver is not bound to this thread the behaviour is\n> undefined.\n> > > *\n> > > * \\sa exec()\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5FA5CC3200\n\tfor ;\n\tMon, 24 May 2021 06:19:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8E12D6891E;\n\tMon, 24 May 2021 08:19:37 +0200 (CEST)","from mail-ej1-x633.google.com (mail-ej1-x633.google.com\n\t[IPv6:2a00:1450:4864:20::633])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 908E2602AD\n\tfor ;\n\tMon, 24 May 2021 08:19:35 +0200 (CEST)","by mail-ej1-x633.google.com with SMTP id p24so38713966ejb.1\n\tfor ;\n\tSun, 23 May 2021 23:19:35 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"I314Maze\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=yHjo/JbnZhYypv6IENRWHNwhxC3kdcOvRb7jJWKboD8=;\n\tb=I314Maze/iIOsL7ybTxOKaRixqXsVqIqXJ5HR+f9WJeJtx26n2OAk4g8rHqVRr+gv2\n\tYuYT4HSByZndgFj6ffzgRloG++A2vCIeuK/xedHxkqHmpHkjAKISZBeZofpWs2oyzfBz\n\tGXyGPITooTYHqGrbS0Hds0F92S4A2bgdF/QhY=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=yHjo/JbnZhYypv6IENRWHNwhxC3kdcOvRb7jJWKboD8=;\n\tb=bXbwyTBcONcIxRM6s7Qs2tcANwDgeJN2qarv9X4D/up3qOMimrr89hAb8wlveWLlGS\n\te3xQJz4HpzIOy+BJ/3KOFF31fM7HXaG4S3zedTcCTTedblSXL9olErF4Hlieq6JexDhz\n\t9ILxid7zAVPzKlbj6Uhr7ko/vM9lH7eQYEe1BUAHGTDDm/sWNCmrztAsuE/dEPdFcuBl\n\tDGIftlnlyXzTF1fF936qR3LOIJPVOCjVUjbvrRQsONYQ2FQ6oMRMPawqlUOxvEQeDC0N\n\t+XA2dIDYcP76reuQ2y+/zzrqG0UVoBx98VsMnbPUt6TN3tsZ/MZpT121zq4h4z8O3vZT\n\tb7cQ==","X-Gm-Message-State":"AOAM533qDXNaQCX0ClKRF0yNqlQAFKnHWR2//5v7CKRDvqBvmoyWSyL4\n\tBZ7CGM5NS3Wu9XqfS+aY0phWQey3rJrgpyPYN9XrlQ==","X-Google-Smtp-Source":"ABdhPJxeRkk97HcmenRPmgJtIzQ2fgsgDkwrajDewEGKYapJAHT4lPg6UH5AXcaHtzgrUuLI4vby29OEO8l72RCzJDA=","X-Received":"by 2002:a17:906:3a04:: with SMTP id\n\tz4mr21582656eje.221.1621837175129; \n\tSun, 23 May 2021 23:19:35 -0700 (PDT)","MIME-Version":"1.0","References":"<20210523023155.20268-1-laurent.pinchart@ideasonboard.com>\n\t<20210523023155.20268-5-laurent.pinchart@ideasonboard.com>\n\t<9c760ca1-f553-665d-2e63-be785035d9e0@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 24 May 2021 15:19:24 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000b51d5705c30d6439\"","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: thread: Document race\n\tcondition at stop time","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17179,"web_url":"https://patchwork.libcamera.org/comment/17179/","msgid":"<20210524090621.7lv4usdcjlss7iqm@uno.localdomain>","date":"2021-05-24T09:06:21","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: thread: Document race\n\tcondition at stop time","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, May 23, 2021 at 05:31:55AM +0300, Laurent Pinchart wrote:\n> When a thread stops, messages may be left in its message queue. Document\n> this in details, with a way to force processing of pending messages when\n> the thread is stopped.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/object.cpp | 8 ++++++++\n> src/libcamera/thread.cpp | 44 ++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 52 insertions(+)\n>\n> diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp\n> index cd83c684b989..5e6b73f9af84 100644\n> --- a/src/libcamera/object.cpp\n> +++ b/src/libcamera/object.cpp\n> @@ -155,6 +155,10 @@ void Object::deleteLater()\n> * running its event loop the message will not be delivered until the event\n> * loop gets started.\n> *\n> + * Due to their asynchronous nature, threads do not provide any guarantee that\n> + * all posted messages are delivered before the thread is stopped. See\n> + * \\ref thread-stop for additional information.\n> + *\n> * \\context This function is \\threadsafe.\n> */\n> void Object::postMessage(std::unique_ptr msg)\n> @@ -212,6 +216,10 @@ void Object::message(Message *msg)\n> * are passed untouched. The caller shall ensure that any pointer argument\n> * remains valid until the method is invoked.\n> *\n> + * Due to the asynchronous nature of threads, functions invoked asynchronously\n> + * with the ConnectionTypeQueued type are not guaranteed to be called before\n> + * the thread is stopped. See \\ref thread-stop for additional information.\n> + *\n> * \\context This function is \\threadsafe.\n> *\n> * \\return For connection types ConnectionTypeDirect and\n> diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp\n> index d59e43966d26..91e4737ad032 100644\n> --- a/src/libcamera/thread.cpp\n> +++ b/src/libcamera/thread.cpp\n> @@ -221,6 +221,47 @@ ThreadData *ThreadData::current()\n> * called. The event loop dispatches events (messages, notifiers and timers)\n> * sent to the objects living in the thread. This behaviour can be modified by\n> * overriding the run() function.\n> + *\n> + * \\section thread-stop Stopping Threads\n> + *\n> + * Threads can't be forcibly stopped. Instead, a thread user first requests the\n> + * thread to exit and then waits for the thread's main function to react to the\n\nisn't it better to say 'the thread's event loop to stop' in place of\nthe 'main function to react' ?\n\n> + * request and return, at which points the thread will stop.\n> + *\n> + * For threads running exec(), the exit() function is used to request the thread\n\nThreads do not exactly run exec() directly, I guess the real\ndifference is made by the fact the Thread (sub-class) overrides run()\nor not, don't it ?\n\nWhat about something similar to:\n\n For Thread derived classes that do not overrider the run()\n function, the exit() function is used to interrupt the thread's\n even loop and exit. Derived classes that overrides the run()\n function implementation might provide a custom mechanism to\n handle the event loop exit sequence. In either cases, the wait()\n function shall be called to wait for the underlying thread to\n stop().\n\n> + * to exit. For threads subclassing the Thread class and implementing a custom\n> + * run() function, a subclass-specific mechanism shall be provided. In either\n> + * case, the wait() function shall be called to wait for the thread to stop.\n> + *\n> + * Due to their asynchronous nature, threads are subject to race conditions when\n> + * they stop. This is of particular importance for messages posted to the thread\n> + * with postMessage() (and the other mechanisms that rely on it, such as\n> + * Object::invokeMethod() or asynchronous signal delivery). To understand the\n> + * issues, three contexts need to be considered:\n\nYou know, I would move this paragraph (probelem statement) before the\nprevious one.\n\nThen explain threads sub-class might or might not override run()\n\nThe provide the example here below which is very clear.\n\n> + *\n> + * - The worker is the Thread performing work and being instructed to stop.\n> + * - The controller is the context which instructs the worker thread to stop.\n> + * - The other contexts are any threads other than the worker and controller\n> + * that interact with the worker thread.\n> + *\n> + * Messages posted to the worker thread from the controller context before\n> + * calling exit() are queued to the thread's message queue, and the Thread class\n> + * offers no guarantee that those messages will be processed before the thread\n> + * stops. This allows threads to stop fast.\n> + *\n> + * A thread that requires delivery of messages posted from the controller\n> + * context before exit() should reimplement the run() function and call\n> + * dispatchMessages() after exec().\n> + *\n> + * Messages posted to the worker thread from the other contexts are asynchronous\n> + * with respect to the exit() call from the controller context. There is no\n> + * guarantee as to whether those messages will be processed or not before the\n> + * thread stops.\n> + *\n> + * Messages that are not processed will stay in the queue, in the exact same way\n> + * as messages posted after the thread has stopped. They will be processed when\n> + * the thread is restarted. If the thread is never restarted, they will be\n> + * deleted without being processed when the Thread instance is destroyed.\n> */\n>\n> /**\n> @@ -480,6 +521,9 @@ EventDispatcher *Thread::eventDispatcher()\n> * running its event loop the message will not be delivered until the event\n> * loop gets started.\n> *\n> + * When the thread is stopped, posted messages may not have all been processed.\n> + * See \\ref thread-stop for additional information.\n> + *\n> * If the \\a receiver is not bound to this thread the behaviour is undefined.\n> *\n> * \\sa exec()\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DCB21C3201\n\tfor ;\n\tMon, 24 May 2021 09:05:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 43D9C68919;\n\tMon, 24 May 2021 11:05:37 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 11DEE602B1\n\tfor ;\n\tMon, 24 May 2021 11:05:36 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 77478240007;\n\tMon, 24 May 2021 09:05:35 +0000 (UTC)"],"Date":"Mon, 24 May 2021 11:06:21 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210524090621.7lv4usdcjlss7iqm@uno.localdomain>","References":"<20210523023155.20268-1-laurent.pinchart@ideasonboard.com>\n\t<20210523023155.20268-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210523023155.20268-5-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: thread: Document race\n\tcondition at stop time","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17182,"web_url":"https://patchwork.libcamera.org/comment/17182/","msgid":"","date":"2021-05-24T09:34:38","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: thread: Document race\n\tcondition at stop time","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, May 24, 2021 at 11:06:21AM +0200, Jacopo Mondi wrote:\n> On Sun, May 23, 2021 at 05:31:55AM +0300, Laurent Pinchart wrote:\n> > When a thread stops, messages may be left in its message queue. Document\n> > this in details, with a way to force processing of pending messages when\n> > the thread is stopped.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/object.cpp | 8 ++++++++\n> > src/libcamera/thread.cpp | 44 ++++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 52 insertions(+)\n> >\n> > diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp\n> > index cd83c684b989..5e6b73f9af84 100644\n> > --- a/src/libcamera/object.cpp\n> > +++ b/src/libcamera/object.cpp\n> > @@ -155,6 +155,10 @@ void Object::deleteLater()\n> > * running its event loop the message will not be delivered until the event\n> > * loop gets started.\n> > *\n> > + * Due to their asynchronous nature, threads do not provide any guarantee that\n> > + * all posted messages are delivered before the thread is stopped. See\n> > + * \\ref thread-stop for additional information.\n> > + *\n> > * \\context This function is \\threadsafe.\n> > */\n> > void Object::postMessage(std::unique_ptr msg)\n> > @@ -212,6 +216,10 @@ void Object::message(Message *msg)\n> > * are passed untouched. The caller shall ensure that any pointer argument\n> > * remains valid until the method is invoked.\n> > *\n> > + * Due to the asynchronous nature of threads, functions invoked asynchronously\n> > + * with the ConnectionTypeQueued type are not guaranteed to be called before\n> > + * the thread is stopped. See \\ref thread-stop for additional information.\n> > + *\n> > * \\context This function is \\threadsafe.\n> > *\n> > * \\return For connection types ConnectionTypeDirect and\n> > diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp\n> > index d59e43966d26..91e4737ad032 100644\n> > --- a/src/libcamera/thread.cpp\n> > +++ b/src/libcamera/thread.cpp\n> > @@ -221,6 +221,47 @@ ThreadData *ThreadData::current()\n> > * called. The event loop dispatches events (messages, notifiers and timers)\n> > * sent to the objects living in the thread. This behaviour can be modified by\n> > * overriding the run() function.\n> > + *\n> > + * \\section thread-stop Stopping Threads\n> > + *\n> > + * Threads can't be forcibly stopped. Instead, a thread user first requests the\n> > + * thread to exit and then waits for the thread's main function to react to the\n> \n> isn't it better to say 'the thread's event loop to stop' in place of\n> the 'main function to react' ?\n\nA thread doesn't necessarily have an event loop, but it always has a\nmain function.\n\n> > + * request and return, at which points the thread will stop.\n> > + *\n> > + * For threads running exec(), the exit() function is used to request the thread\n> \n> Threads do not exactly run exec() directly, I guess the real\n> difference is made by the fact the Thread (sub-class) overrides run()\n> or not, don't it ?\n>\n> What about something similar to:\n> \n> For Thread derived classes that do not overrider the run()\n> function, the exit() function is used to interrupt the thread's\n\nMore precisely, it's about whether the run() function will run exec(),\neither using the default implementation, or in an overridden run()\nfunction that calls exec() manually. It's thus not only about threads\nthat don't override run().\n\n> even loop and exit. Derived classes that overrides the run()\n> function implementation might provide a custom mechanism to\n> handle the event loop exit sequence. In either cases, the wait()\n> function shall be called to wait for the underlying thread to\n> stop().\n> \n> > + * to exit. For threads subclassing the Thread class and implementing a custom\n> > + * run() function, a subclass-specific mechanism shall be provided. In either\n> > + * case, the wait() function shall be called to wait for the thread to stop.\n> > + *\n> > + * Due to their asynchronous nature, threads are subject to race conditions when\n> > + * they stop. This is of particular importance for messages posted to the thread\n> > + * with postMessage() (and the other mechanisms that rely on it, such as\n> > + * Object::invokeMethod() or asynchronous signal delivery). To understand the\n> > + * issues, three contexts need to be considered:\n> \n> You know, I would move this paragraph (probelem statement) before the\n> previous one.\n\nI had that before, but I wasn't entirely happy with it. I decided to\nfirst explain how to stop a thread, and then focus on one specific\naspect of stop issues, which is message handling. Talking about the\nmessages issue first, and then explaining how to stop threads, makes it\nsound like the stop procedure is specific to message handling.\n\n> Then explain threads sub-class might or might not override run()\n> \n> The provide the example here below which is very clear.\n> \n> > + *\n> > + * - The worker is the Thread performing work and being instructed to stop.\n> > + * - The controller is the context which instructs the worker thread to stop.\n> > + * - The other contexts are any threads other than the worker and controller\n> > + * that interact with the worker thread.\n> > + *\n> > + * Messages posted to the worker thread from the controller context before\n> > + * calling exit() are queued to the thread's message queue, and the Thread class\n> > + * offers no guarantee that those messages will be processed before the thread\n> > + * stops. This allows threads to stop fast.\n> > + *\n> > + * A thread that requires delivery of messages posted from the controller\n> > + * context before exit() should reimplement the run() function and call\n> > + * dispatchMessages() after exec().\n> > + *\n> > + * Messages posted to the worker thread from the other contexts are asynchronous\n> > + * with respect to the exit() call from the controller context. There is no\n> > + * guarantee as to whether those messages will be processed or not before the\n> > + * thread stops.\n> > + *\n> > + * Messages that are not processed will stay in the queue, in the exact same way\n> > + * as messages posted after the thread has stopped. They will be processed when\n> > + * the thread is restarted. If the thread is never restarted, they will be\n> > + * deleted without being processed when the Thread instance is destroyed.\n> > */\n> >\n> > /**\n> > @@ -480,6 +521,9 @@ EventDispatcher *Thread::eventDispatcher()\n> > * running its event loop the message will not be delivered until the event\n> > * loop gets started.\n> > *\n> > + * When the thread is stopped, posted messages may not have all been processed.\n> > + * See \\ref thread-stop for additional information.\n> > + *\n> > * If the \\a receiver is not bound to this thread the behaviour is undefined.\n> > *\n> > * \\sa exec()","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 59C4CC3201\n\tfor ;\n\tMon, 24 May 2021 09:34:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B71066891E;\n\tMon, 24 May 2021 11:34:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C5396602B1\n\tfor ;\n\tMon, 24 May 2021 11:34:42 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3EE621315;\n\tMon, 24 May 2021 11:34:42 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"e2xtbZSp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621848882;\n\tbh=u2AHIVCdziXeoER2kdnsPP875HHZZm98m/otYsO/1Uo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=e2xtbZSpvVdvf8guOLOiNJHWwUBciolz3te/9o5fkMntuJWdlLM25mh3c8BKKPVH7\n\tOk/HGvb4i7EApuY7GKCCJ7CuP0vkeVG3efUmEFb9RcIn1JnJVKPnjGRsm2oBS/4UUx\n\toeqo3VxiJZ7htGZIcXntNcsBEK5p8WfeQFP+lSs8=","Date":"Mon, 24 May 2021 12:34:38 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210523023155.20268-1-laurent.pinchart@ideasonboard.com>\n\t<20210523023155.20268-5-laurent.pinchart@ideasonboard.com>\n\t<20210524090621.7lv4usdcjlss7iqm@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210524090621.7lv4usdcjlss7iqm@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: thread: Document race\n\tcondition at stop time","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17186,"web_url":"https://patchwork.libcamera.org/comment/17186/","msgid":"<20210524095429.ih2qxgisifuetuec@uno.localdomain>","date":"2021-05-24T09:54:29","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: thread: Document race\n\tcondition at stop time","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi LAurent,\n\nOn Mon, May 24, 2021 at 12:34:38PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, May 24, 2021 at 11:06:21AM +0200, Jacopo Mondi wrote:\n> > On Sun, May 23, 2021 at 05:31:55AM +0300, Laurent Pinchart wrote:\n> > > When a thread stops, messages may be left in its message queue. Document\n> > > this in details, with a way to force processing of pending messages when\n> > > the thread is stopped.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/libcamera/object.cpp | 8 ++++++++\n> > > src/libcamera/thread.cpp | 44 ++++++++++++++++++++++++++++++++++++++++\n> > > 2 files changed, 52 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp\n> > > index cd83c684b989..5e6b73f9af84 100644\n> > > --- a/src/libcamera/object.cpp\n> > > +++ b/src/libcamera/object.cpp\n> > > @@ -155,6 +155,10 @@ void Object::deleteLater()\n> > > * running its event loop the message will not be delivered until the event\n> > > * loop gets started.\n> > > *\n> > > + * Due to their asynchronous nature, threads do not provide any guarantee that\n> > > + * all posted messages are delivered before the thread is stopped. See\n> > > + * \\ref thread-stop for additional information.\n> > > + *\n> > > * \\context This function is \\threadsafe.\n> > > */\n> > > void Object::postMessage(std::unique_ptr msg)\n> > > @@ -212,6 +216,10 @@ void Object::message(Message *msg)\n> > > * are passed untouched. The caller shall ensure that any pointer argument\n> > > * remains valid until the method is invoked.\n> > > *\n> > > + * Due to the asynchronous nature of threads, functions invoked asynchronously\n> > > + * with the ConnectionTypeQueued type are not guaranteed to be called before\n> > > + * the thread is stopped. See \\ref thread-stop for additional information.\n> > > + *\n> > > * \\context This function is \\threadsafe.\n> > > *\n> > > * \\return For connection types ConnectionTypeDirect and\n> > > diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp\n> > > index d59e43966d26..91e4737ad032 100644\n> > > --- a/src/libcamera/thread.cpp\n> > > +++ b/src/libcamera/thread.cpp\n> > > @@ -221,6 +221,47 @@ ThreadData *ThreadData::current()\n> > > * called. The event loop dispatches events (messages, notifiers and timers)\n> > > * sent to the objects living in the thread. This behaviour can be modified by\n> > > * overriding the run() function.\n> > > + *\n> > > + * \\section thread-stop Stopping Threads\n> > > + *\n> > > + * Threads can't be forcibly stopped. Instead, a thread user first requests the\n> > > + * thread to exit and then waits for the thread's main function to react to the\n> >\n> > isn't it better to say 'the thread's event loop to stop' in place of\n> > the 'main function to react' ?\n>\n> A thread doesn't necessarily have an event loop, but it always has a\n> main function.\n>\n> > > + * request and return, at which points the thread will stop.\n> > > + *\n> > > + * For threads running exec(), the exit() function is used to request the thread\n> >\n> > Threads do not exactly run exec() directly, I guess the real\n> > difference is made by the fact the Thread (sub-class) overrides run()\n> > or not, don't it ?\n> >\n> > What about something similar to:\n> >\n> > For Thread derived classes that do not overrider the run()\n> > function, the exit() function is used to interrupt the thread's\n>\n> More precisely, it's about whether the run() function will run exec(),\n> either using the default implementation, or in an overridden run()\n> function that calls exec() manually. It's thus not only about threads\n> that don't override run().\n>\n\nThreads that do not override run() will use the default base\nimplementation that just run exec(). That's what I meant.\n\nHowever, details\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> > even loop and exit. Derived classes that overrides the run()\n> > function implementation might provide a custom mechanism to\n> > handle the event loop exit sequence. In either cases, the wait()\n> > function shall be called to wait for the underlying thread to\n> > stop().\n> >\n> > > + * to exit. For threads subclassing the Thread class and implementing a custom\n> > > + * run() function, a subclass-specific mechanism shall be provided. In either\n> > > + * case, the wait() function shall be called to wait for the thread to stop.\n> > > + *\n> > > + * Due to their asynchronous nature, threads are subject to race conditions when\n> > > + * they stop. This is of particular importance for messages posted to the thread\n> > > + * with postMessage() (and the other mechanisms that rely on it, such as\n> > > + * Object::invokeMethod() or asynchronous signal delivery). To understand the\n> > > + * issues, three contexts need to be considered:\n> >\n> > You know, I would move this paragraph (probelem statement) before the\n> > previous one.\n>\n> I had that before, but I wasn't entirely happy with it. I decided to\n> first explain how to stop a thread, and then focus on one specific\n> aspect of stop issues, which is message handling. Talking about the\n> messages issue first, and then explaining how to stop threads, makes it\n> sound like the stop procedure is specific to message handling.\n>\n> > Then explain threads sub-class might or might not override run()\n> >\n> > The provide the example here below which is very clear.\n> >\n> > > + *\n> > > + * - The worker is the Thread performing work and being instructed to stop.\n> > > + * - The controller is the context which instructs the worker thread to stop.\n> > > + * - The other contexts are any threads other than the worker and controller\n> > > + * that interact with the worker thread.\n> > > + *\n> > > + * Messages posted to the worker thread from the controller context before\n> > > + * calling exit() are queued to the thread's message queue, and the Thread class\n> > > + * offers no guarantee that those messages will be processed before the thread\n> > > + * stops. This allows threads to stop fast.\n> > > + *\n> > > + * A thread that requires delivery of messages posted from the controller\n> > > + * context before exit() should reimplement the run() function and call\n> > > + * dispatchMessages() after exec().\n> > > + *\n> > > + * Messages posted to the worker thread from the other contexts are asynchronous\n> > > + * with respect to the exit() call from the controller context. There is no\n> > > + * guarantee as to whether those messages will be processed or not before the\n> > > + * thread stops.\n> > > + *\n> > > + * Messages that are not processed will stay in the queue, in the exact same way\n> > > + * as messages posted after the thread has stopped. They will be processed when\n> > > + * the thread is restarted. If the thread is never restarted, they will be\n> > > + * deleted without being processed when the Thread instance is destroyed.\n> > > */\n> > >\n> > > /**\n> > > @@ -480,6 +521,9 @@ EventDispatcher *Thread::eventDispatcher()\n> > > * running its event loop the message will not be delivered until the event\n> > > * loop gets started.\n> > > *\n> > > + * When the thread is stopped, posted messages may not have all been processed.\n> > > + * See \\ref thread-stop for additional information.\n> > > + *\n> > > * If the \\a receiver is not bound to this thread the behaviour is undefined.\n> > > *\n> > > * \\sa exec()\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2CB92C3201\n\tfor ;\n\tMon, 24 May 2021 09:53:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8E0FF68919;\n\tMon, 24 May 2021 11:53:45 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 19AC1602B1\n\tfor ;\n\tMon, 24 May 2021 11:53:44 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 7C0FAFF808;\n\tMon, 24 May 2021 09:53:43 +0000 (UTC)"],"Date":"Mon, 24 May 2021 11:54:29 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210524095429.ih2qxgisifuetuec@uno.localdomain>","References":"<20210523023155.20268-1-laurent.pinchart@ideasonboard.com>\n\t<20210523023155.20268-5-laurent.pinchart@ideasonboard.com>\n\t<20210524090621.7lv4usdcjlss7iqm@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: thread: Document race\n\tcondition at stop time","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]