Patch Detail
Show a patch.
GET /api/1.1/patches/2696/?format=api
{ "id": 2696, "url": "https://patchwork.libcamera.org/api/1.1/patches/2696/?format=api", "web_url": "https://patchwork.libcamera.org/patch/2696/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "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=api", "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=api", "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" ] }