{"id":2696,"url":"https://patchwork.libcamera.org/api/1.1/patches/2696/?format=json","web_url":"https://patchwork.libcamera.org/patch/2696/","project":{"id":1,"url":"https://patchwork.libcamera.org/api/1.1/projects/1/?format=json","name":"libcamera","link_name":"libcamera","list_id":"libcamera_core","list_email":"libcamera-devel@lists.libcamera.org","web_url":"","scm_url":"","webscm_url":""},"msgid":"<20200120002437.6633-10-laurent.pinchart@ideasonboard.com>","date":"2020-01-20T00:24:27","name":"[libcamera-devel,09/19] libcamera: signal: Make slots list private","commit_ref":null,"pull_url":null,"state":"superseded","archived":false,"hash":"6f46ecc652dc2ccfbc8ae2f3c40b477a02fc65df","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/1.1/people/2/?format=json","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"delegate":null,"mbox":"https://patchwork.libcamera.org/patch/2696/mbox/","series":[{"id":641,"url":"https://patchwork.libcamera.org/api/1.1/series/641/?format=json","web_url":"https://patchwork.libcamera.org/project/libcamera/list/?series=641","date":"2020-01-20T00:24:19","name":"Initial libcamera threading model","version":1,"mbox":"https://patchwork.libcamera.org/series/641/mbox/"}],"comments":"https://patchwork.libcamera.org/api/patches/2696/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/2696/checks/","tags":{},"headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 073AE607DD\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 20 Jan 2020 01:24:46 +0100 (CET)","from pendragon.bb.dnainternet.fi (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 931C6B57\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 20 Jan 2020 01:24:45 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579479885;\n\tbh=5/f77IEbrZNeopV8uxm1wG8GJJMXo/vUPIp2sDTZG6c=;\n\th=From:To:Subject:Date:In-Reply-To:References:From;\n\tb=wW0kFYMyDSt5tYKQDyd6FYE5WrwO55o6l9y/J0Y5JebkUgxB13gstDPa3x3xhVOIA\n\tQ4HLELsWEXr+dOqMiIfe66QHhE1hOnBHVh15ZTTYuh5WC17Z6hS6yhOp/oUz1uRiYN\n\tb/1zpyP2BngSq04xgRzfm13zYLgu/Rpw61J+2wOg=","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"libcamera-devel@lists.libcamera.org","Date":"Mon, 20 Jan 2020 02:24:27 +0200","Message-Id":"<20200120002437.6633-10-laurent.pinchart@ideasonboard.com>","X-Mailer":"git-send-email 2.24.1","In-Reply-To":"<20200120002437.6633-1-laurent.pinchart@ideasonboard.com>","References":"<20200120002437.6633-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"[libcamera-devel] [PATCH 09/19] libcamera: signal: Make slots list\n\tprivate","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 20 Jan 2020 00:24:47 -0000"},"content":"The slots list is touched from most of the Signal template functions. In\norder to prepare for thread-safety, move handling of the lists list to a\nsmall number of non-template functions in the SignalBase class.\n\nThis incidently fixes a bug in signal disconnection handling where the\nsignal wasn't removed from the object's signals list, as pointed out by\nthe signals unit test.\n\nSigned-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n---\n include/libcamera/object.h |  4 +-\n include/libcamera/signal.h | 88 ++++++++++++++++----------------------\n src/libcamera/object.cpp   |  7 ++-\n src/libcamera/signal.cpp   | 36 ++++++++++++++++\n 4 files changed, 81 insertions(+), 54 deletions(-)","diff":"diff --git a/include/libcamera/object.h b/include/libcamera/object.h\nindex 9344af30bc79..4d16f3f2b610 100644\n--- a/include/libcamera/object.h\n+++ b/include/libcamera/object.h\n@@ -48,9 +48,7 @@ protected:\n \tvirtual void message(Message *msg);\n \n private:\n-\ttemplate<typename... Args>\n-\tfriend class Signal;\n-\tfriend class BoundMethodBase;\n+\tfriend class SignalBase;\n \tfriend class Thread;\n \n \tvoid notifyThreadMove();\ndiff --git a/include/libcamera/signal.h b/include/libcamera/signal.h\nindex 432d95d0ed1c..c13bb30f0636 100644\n--- a/include/libcamera/signal.h\n+++ b/include/libcamera/signal.h\n@@ -7,6 +7,7 @@\n #ifndef __LIBCAMERA_SIGNAL_H__\n #define __LIBCAMERA_SIGNAL_H__\n \n+#include <functional>\n #include <list>\n #include <type_traits>\n #include <vector>\n@@ -19,23 +20,18 @@ namespace libcamera {\n class SignalBase\n {\n public:\n-\ttemplate<typename T>\n-\tvoid disconnect(T *obj)\n-\t{\n-\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n-\t\t\tBoundMethodBase *slot = *iter;\n-\t\t\tif (slot->match(obj)) {\n-\t\t\t\titer = slots_.erase(iter);\n-\t\t\t\tdelete slot;\n-\t\t\t} else {\n-\t\t\t\t++iter;\n-\t\t\t}\n-\t\t}\n-\t}\n+\tvoid disconnect(Object *object);\n \n protected:\n-\tfriend class Object;\n-\tstd::list<BoundMethodBase *> slots_;\n+\tusing SlotList = std::list<BoundMethodBase *>;\n+\n+\tvoid connect(BoundMethodBase *slot);\n+\tvoid disconnect(std::function<bool(SlotList::iterator &)> match);\n+\n+\tSlotList slots();\n+\n+private:\n+\tSlotList slots_;\n };\n \n template<typename... Args>\n@@ -45,12 +41,7 @@ public:\n \tSignal() {}\n \t~Signal()\n \t{\n-\t\tfor (BoundMethodBase *slot : slots_) {\n-\t\t\tObject *object = slot->object();\n-\t\t\tif (object)\n-\t\t\t\tobject->disconnect(this);\n-\t\t\tdelete slot;\n-\t\t}\n+\t\tdisconnect();\n \t}\n \n #ifndef __DOXYGEN__\n@@ -59,8 +50,7 @@ public:\n \t\t     ConnectionType type = ConnectionTypeAuto)\n \t{\n \t\tObject *object = static_cast<Object *>(obj);\n-\t\tobject->connect(this);\n-\t\tslots_.push_back(new BoundMethodMember<T, void, Args...>(obj, object, func, type));\n+\t\tSignalBase::connect(new BoundMethodMember<T, void, Args...>(obj, object, func, type));\n \t}\n \n \ttemplate<typename T, typename R, typename std::enable_if<!std::is_base_of<Object, T>::value>::type * = nullptr>\n@@ -69,63 +59,62 @@ public:\n #endif\n \tvoid connect(T *obj, R (T::*func)(Args...))\n \t{\n-\t\tslots_.push_back(new BoundMethodMember<T, R, Args...>(obj, nullptr, func));\n+\t\tSignalBase::connect(new BoundMethodMember<T, R, Args...>(obj, nullptr, func));\n \t}\n \n \ttemplate<typename R>\n \tvoid connect(R (*func)(Args...))\n \t{\n-\t\tslots_.push_back(new BoundMethodStatic<R, Args...>(func));\n+\t\tSignalBase::connect(new BoundMethodStatic<R, Args...>(func));\n \t}\n \n \tvoid disconnect()\n \t{\n-\t\tfor (BoundMethodBase *slot : slots_)\n-\t\t\tdelete slot;\n-\t\tslots_.clear();\n+\t\tSignalBase::disconnect([](SlotList::iterator &iter) {\n+\t\t\treturn true;\n+\t\t});\n \t}\n \n \ttemplate<typename T>\n \tvoid disconnect(T *obj)\n \t{\n-\t\tSignalBase::disconnect(obj);\n+\t\tSignalBase::disconnect([obj](SlotList::iterator &iter) {\n+\t\t\treturn (*iter)->match(obj);\n+\t\t});\n \t}\n \n \ttemplate<typename T, typename R>\n \tvoid disconnect(T *obj, R (T::*func)(Args...))\n \t{\n-\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n+\t\tSignalBase::disconnect([obj, func](SlotList::iterator &iter) {\n \t\t\tBoundMethodArgs<R, Args...> *slot =\n \t\t\t\tstatic_cast<BoundMethodArgs<R, Args...> *>(*iter);\n+\n+\t\t\tif (!slot->match(obj))\n+\t\t\t\treturn false;\n+\n \t\t\t/*\n \t\t\t * If the object matches the slot, the slot is\n \t\t\t * guaranteed to be a member slot, so we can safely\n \t\t\t * cast it to BoundMethodMember<T, Args...> to match\n \t\t\t * func.\n \t\t\t */\n-\t\t\tif (slot->match(obj) &&\n-\t\t\t    static_cast<BoundMethodMember<T, R, Args...> *>(slot)->match(func)) {\n-\t\t\t\titer = slots_.erase(iter);\n-\t\t\t\tdelete slot;\n-\t\t\t} else {\n-\t\t\t\t++iter;\n-\t\t\t}\n-\t\t}\n+\t\t\treturn static_cast<BoundMethodMember<T, R, Args...> *>(slot)->match(func);\n+\t\t});\n \t}\n \n \ttemplate<typename R>\n \tvoid disconnect(R (*func)(Args...))\n \t{\n-\t\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n-\t\t\tBoundMethodArgs<R, Args...> *slot = *iter;\n-\t\t\tif (slot->match(nullptr) &&\n-\t\t\t    static_cast<BoundMethodStatic<R, Args...> *>(slot)->match(func)) {\n-\t\t\t\titer = slots_.erase(iter);\n-\t\t\t\tdelete slot;\n-\t\t\t} else {\n-\t\t\t\t++iter;\n-\t\t\t}\n-\t\t}\n+\t\tSignalBase::disconnect([func](SlotList::iterator &iter) {\n+\t\t\tBoundMethodArgs<R, Args...> *slot =\n+\t\t\t\tstatic_cast<BoundMethodArgs<R, Args...> *>(*iter);\n+\n+\t\t\tif (!slot->match(nullptr))\n+\t\t\t\treturn false;\n+\n+\t\t\treturn static_cast<BoundMethodStatic<R, Args...> *>(slot)->match(func);\n+\t\t});\n \t}\n \n \tvoid emit(Args... args)\n@@ -134,8 +123,7 @@ public:\n \t\t * Make a copy of the slots list as the slot could call the\n \t\t * disconnect operation, invalidating the iterator.\n \t\t */\n-\t\tstd::vector<BoundMethodBase *> slots{ slots_.begin(), slots_.end() };\n-\t\tfor (BoundMethodBase *slot : slots)\n+\t\tfor (BoundMethodBase *slot : slots())\n \t\t\tstatic_cast<BoundMethodArgs<void, Args...> *>(slot)->activate(args...);\n \t}\n };\ndiff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp\nindex 21aad5652b38..f2a8be172547 100644\n--- a/src/libcamera/object.cpp\n+++ b/src/libcamera/object.cpp\n@@ -76,7 +76,12 @@ Object::Object(Object *parent)\n  */\n Object::~Object()\n {\n-\tfor (SignalBase *signal : signals_)\n+\t/*\n+\t * Move signals to a private list to avoid concurrent iteration and\n+\t * deletion of items from Signal::disconnect().\n+\t */\n+\tstd::list<SignalBase *> signals(std::move(signals_));\n+\tfor (SignalBase *signal : signals)\n \t\tsignal->disconnect(this);\n \n \tif (pendingMessages_)\ndiff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp\nindex 190033317c72..9bb7eca8ed6e 100644\n--- a/src/libcamera/signal.cpp\n+++ b/src/libcamera/signal.cpp\n@@ -14,6 +14,42 @@\n \n namespace libcamera {\n \n+void SignalBase::connect(BoundMethodBase *slot)\n+{\n+\tObject *object = slot->object();\n+\tif (object)\n+\t\tobject->connect(this);\n+\tslots_.push_back(slot);\n+}\n+\n+void SignalBase::disconnect(Object *object)\n+{\n+\tdisconnect([object](SlotList::iterator &iter) {\n+\t\treturn (*iter)->match(object);\n+\t});\n+}\n+\n+void SignalBase::disconnect(std::function<bool(SlotList::iterator &)> match)\n+{\n+\tfor (auto iter = slots_.begin(); iter != slots_.end(); ) {\n+\t\tif (match(iter)) {\n+\t\t\tObject *object = (*iter)->object();\n+\t\t\tif (object)\n+\t\t\t\tobject->disconnect(this);\n+\n+\t\t\tdelete *iter;\n+\t\t\titer = slots_.erase(iter);\n+\t\t} else {\n+\t\t\t++iter;\n+\t\t}\n+\t}\n+}\n+\n+SignalBase::SlotList SignalBase::slots()\n+{\n+\treturn slots_;\n+}\n+\n /**\n  * \\class Signal\n  * \\brief Generic signal and slot communication mechanism\n","prefixes":["libcamera-devel","09/19"]}