{"action":"created","issue":{"url":"https://api.github.com/repos/gift-surg/GIFT-Grab/issues/16","repository_url":"https://api.github.com/repos/gift-surg/GIFT-Grab","labels_url":"https://api.github.com/repos/gift-surg/GIFT-Grab/issues/16/labels{/name}","comments_url":"https://api.github.com/repos/gift-surg/GIFT-Grab/issues/16/comments","events_url":"https://api.github.com/repos/gift-surg/GIFT-Grab/issues/16/events","html_url":"https://github.com/gift-surg/GIFT-Grab/issues/16","id":368708193,"node_id":"MDU6SXNzdWUzNjg3MDgxOTM=","number":16,"title":"IObservableObserverWrapper unable to acquire GIL","user":{"login":"dzhoshkun","id":9197424,"node_id":"MDQ6VXNlcjkxOTc0MjQ=","avatar_url":"https://avatars2.githubusercontent.com/u/9197424?v=4","gravatar_id":"","url":"https://api.github.com/users/dzhoshkun","html_url":"https://github.com/dzhoshkun","followers_url":"https://api.github.com/users/dzhoshkun/followers","following_url":"https://api.github.com/users/dzhoshkun/following{/other_user}","gists_url":"https://api.github.com/users/dzhoshkun/gists{/gist_id}","starred_url":"https://api.github.com/users/dzhoshkun/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dzhoshkun/subscriptions","organizations_url":"https://api.github.com/users/dzhoshkun/orgs","repos_url":"https://api.github.com/users/dzhoshkun/repos","events_url":"https://api.github.com/users/dzhoshkun/events{/privacy}","received_events_url":"https://api.github.com/users/dzhoshkun/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"dzhoshkun","id":9197424,"node_id":"MDQ6VXNlcjkxOTc0MjQ=","avatar_url":"https://avatars2.githubusercontent.com/u/9197424?v=4","gravatar_id":"","url":"https://api.github.com/users/dzhoshkun","html_url":"https://github.com/dzhoshkun","followers_url":"https://api.github.com/users/dzhoshkun/followers","following_url":"https://api.github.com/users/dzhoshkun/following{/other_user}","gists_url":"https://api.github.com/users/dzhoshkun/gists{/gist_id}","starred_url":"https://api.github.com/users/dzhoshkun/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dzhoshkun/subscriptions","organizations_url":"https://api.github.com/users/dzhoshkun/orgs","repos_url":"https://api.github.com/users/dzhoshkun/repos","events_url":"https://api.github.com/users/dzhoshkun/events{/privacy}","received_events_url":"https://api.github.com/users/dzhoshkun/received_events","type":"User","site_admin":false},"assignees":[{"login":"dzhoshkun","id":9197424,"node_id":"MDQ6VXNlcjkxOTc0MjQ=","avatar_url":"https://avatars2.githubusercontent.com/u/9197424?v=4","gravatar_id":"","url":"https://api.github.com/users/dzhoshkun","html_url":"https://github.com/dzhoshkun","followers_url":"https://api.github.com/users/dzhoshkun/followers","following_url":"https://api.github.com/users/dzhoshkun/following{/other_user}","gists_url":"https://api.github.com/users/dzhoshkun/gists{/gist_id}","starred_url":"https://api.github.com/users/dzhoshkun/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dzhoshkun/subscriptions","organizations_url":"https://api.github.com/users/dzhoshkun/orgs","repos_url":"https://api.github.com/users/dzhoshkun/repos","events_url":"https://api.github.com/users/dzhoshkun/events{/privacy}","received_events_url":"https://api.github.com/users/dzhoshkun/received_events","type":"User","site_admin":false}],"milestone":null,"comments":14,"created_at":"2018-10-10T14:56:59Z","updated_at":"2018-10-23T19:16:08Z","closed_at":null,"author_association":"CONTRIBUTOR","body":"It looks like the [GIL acquisition mechanism](https://github.com/gift-surg/GIFT-Grab/blob/master/src/python/wrapper.cpp#L338) does not work when multiple Python intermediate processing nodes (`IObservableObserverWrapper` extenders [C++], `IObservableObserver` [Python]) are involved.\r\nAnd when this is the case, the application crashes (mostly with a segfault, but also sometimes with `boost::python::error_already_set`).\r\n\r\n* [x] confirm this\r\n\r\nThe [Python documentation](https://docs.python.org/3.7/c-api/init.html#c.PyGILState_Ensure) states:\r\n\r\n> When the function returns, the current thread will hold the GIL and be able to call arbitrary Python code. Failure is a fatal error.\r\n\r\nThe following patch seems to resolve this issue:\r\n\r\n```diff\r\ndiff --git a/src/python/wrapper.cpp b/src/python/wrapper.cpp\r\nindex ba34c64..a82018b 100644\r\n--- a/src/python/wrapper.cpp\r\n+++ b/src/python/wrapper.cpp\r\n@@ -332,11 +332,13 @@ public:\r\n \r\n     void update(gg::VideoFrame & frame)\r\n     {\r\n-        if (override f = this->get_override(\"update\"))\r\n         {\r\n-            VideoFrameNumPyWrapper wrapped_frame(&frame);\r\n             gg::ScopedPythonGILLock gil_lock;\r\n-            f(boost::ref(wrapped_frame));\r\n+            if (override f = this->get_override(\"update\"))\r\n+            {\r\n+                VideoFrameNumPyWrapper wrapped_frame(&frame);\r\n+                f(boost::ref(wrapped_frame));\r\n+            }\r\n         }\r\n         notify(frame);\r\n     }\r\n```\r\n\r\nThis seems to be the case when multiple intermediate processing nodes are involved. Which is probably why this issue has never been encountered before.\r\n\r\n* [x] confirm this with a simple example\r\n\r\nAlso, for the sake of completeness, it might make sense to use the following patch that introduces the same logic to both `update` implementation queries:\r\n\r\n```diff\r\ndiff --git a/src/python/wrapper.cpp b/src/python/wrapper.cpp\r\nindex ba34c64..83c4f11 100644\r\n--- a/src/python/wrapper.cpp\r\n+++ b/src/python/wrapper.cpp\r\n@@ -268,7 +268,8 @@ public:\r\n     {\r\n         gg::ScopedPythonGILLock gil_lock;\r\n         VideoFrameNumPyWrapper wrapped_frame(&frame);\r\n-        this->get_override(\"update\")(boost::ref(wrapped_frame));\r\n+        if (override f = this->get_override(\"update\"))\r\n+            f(boost::ref(wrapped_frame));\r\n     }\r\n };\r\n \r\n@@ -332,11 +333,13 @@ public:\r\n \r\n     void update(gg::VideoFrame & frame)\r\n     {\r\n-        if (override f = this->get_override(\"update\"))\r\n         {\r\n-            VideoFrameNumPyWrapper wrapped_frame(&frame);\r\n             gg::ScopedPythonGILLock gil_lock;\r\n-            f(boost::ref(wrapped_frame));\r\n+            if (override f = this->get_override(\"update\"))\r\n+            {\r\n+                VideoFrameNumPyWrapper wrapped_frame(&frame);\r\n+                f(boost::ref(wrapped_frame));\r\n+            }\r\n         }\r\n         notify(frame);\r\n     }\r\n```"},"comment":{"url":"https://api.github.com/repos/gift-surg/GIFT-Grab/issues/comments/432382305","html_url":"https://github.com/gift-surg/GIFT-Grab/issues/16#issuecomment-432382305","issue_url":"https://api.github.com/repos/gift-surg/GIFT-Grab/issues/16","id":432382305,"node_id":"MDEyOklzc3VlQ29tbWVudDQzMjM4MjMwNQ==","user":{"login":"dzhoshkun","id":9197424,"node_id":"MDQ6VXNlcjkxOTc0MjQ=","avatar_url":"https://avatars2.githubusercontent.com/u/9197424?v=4","gravatar_id":"","url":"https://api.github.com/users/dzhoshkun","html_url":"https://github.com/dzhoshkun","followers_url":"https://api.github.com/users/dzhoshkun/followers","following_url":"https://api.github.com/users/dzhoshkun/following{/other_user}","gists_url":"https://api.github.com/users/dzhoshkun/gists{/gist_id}","starred_url":"https://api.github.com/users/dzhoshkun/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dzhoshkun/subscriptions","organizations_url":"https://api.github.com/users/dzhoshkun/orgs","repos_url":"https://api.github.com/users/dzhoshkun/repos","events_url":"https://api.github.com/users/dzhoshkun/events{/privacy}","received_events_url":"https://api.github.com/users/dzhoshkun/received_events","type":"User","site_admin":false},"created_at":"2018-10-23T19:16:08Z","updated_at":"2018-10-23T19:16:08Z","author_association":"CONTRIBUTOR","body":"On Kyrie with Epiphan:\r\n\r\n```\r\ndzhoshkun@gift-kyrie ‹ 16-iobservableobserverwrapper-unable-to-acquire-gil › : ~/ws/GIFT-Grab/src/tests/pipeline\r\n[0] % cat /home/dzhoshkun/ws/GIFT-Grab/src/tests/pipeline/gg-iss-16/2018-10-23-19-25-55/exit-codes-100.csv\r\n exit-code frequency\r\n         0       100\r\n\r\n```"}}
{"actor":{"display_login":"dzhoshkun"}}