From 175c3b32ea3d9d9bf2ea6965d3e94eb93792d0a8 Mon Sep 17 00:00:00 2001
From: thorstenhater <24411438+thorstenhater@users.noreply.github.com>
Date: Mon, 14 Dec 2020 09:32:14 +0100
Subject: [PATCH] Fix GIL related error in pybind11 interface. (#1272)
Fix a latent bug in the Python wrapper that was triggered in pybind11 v2.6.1
These changes ensure that the GIL is acquired before calling functions in C++ that
may consume a Python object with reference count 1 as an argument, in which case
the object's destructor is called at the end of the function, after any GIL acquired inside
the function would have been released.
Fixes #1271.
---
python/cells.cpp | 30 ------------------------
python/cells.hpp | 1 -
python/recipe.cpp | 59 +++++++++++++++++++++++++++++++++++++----------
3 files changed, 47 insertions(+), 43 deletions(-)
diff --git a/python/cells.cpp b/python/cells.cpp
index 62ac4011..7953593e 100644
--- a/python/cells.cpp
+++ b/python/cells.cpp
@@ -30,36 +30,6 @@
#include "strprintf.hpp"
namespace pyarb {
-
-// Convert a cell description inside a Python object to a cell
-// description in a unique_any, as required by the recipe interface.
-//
-// Warning: requires that the GIL has been acquired before calling,
-// if there is a segmentation fault in the cast or isinstance calls,
-// check that the caller has the GIL.
-arb::util::unique_any convert_cell(pybind11::object o) {
- using pybind11::isinstance;
- using pybind11::cast;
-
- pybind11::gil_scoped_acquire guard;
- if (isinstance<arb::spike_source_cell>(o)) {
- return arb::util::unique_any(cast<arb::spike_source_cell>(o));
- }
- if (isinstance<arb::benchmark_cell>(o)) {
- return arb::util::unique_any(cast<arb::benchmark_cell>(o));
- }
- if (isinstance<arb::lif_cell>(o)) {
- return arb::util::unique_any(cast<arb::lif_cell>(o));
- }
- if (isinstance<arb::cable_cell>(o)) {
- return arb::util::unique_any(cast<arb::cable_cell>(o));
- }
-
- throw pyarb_error("recipe.cell_description returned \""
- + std::string(pybind11::str(o))
- + "\" which does not describe a known Arbor cell type");
-}
-
//
// proxies
//
diff --git a/python/cells.hpp b/python/cells.hpp
index f4fd3175..8560f16a 100644
--- a/python/cells.hpp
+++ b/python/cells.hpp
@@ -7,7 +7,6 @@
#include <arbor/util/unique_any.hpp>
namespace pyarb {
-arb::util::unique_any convert_cell(pybind11::object o);
struct global_props_shim {
arb::mechanism_catalogue cat;
diff --git a/python/recipe.cpp b/python/recipe.cpp
index e1c67da5..d543fe6d 100644
--- a/python/recipe.cpp
+++ b/python/recipe.cpp
@@ -6,11 +6,13 @@
#include <pybind11/pytypes.h>
#include <pybind11/stl.h>
+#include <arbor/benchmark_cell.hpp>
#include <arbor/cable_cell.hpp>
+#include <arbor/lif_cell.hpp>
+#include <arbor/spike_source_cell.hpp>
#include <arbor/event_generator.hpp>
#include <arbor/morph/primitives.hpp>
#include <arbor/recipe.hpp>
-#include <arbor/spike_source_cell.hpp>
#include "cells.hpp"
#include "conversion.hpp"
@@ -21,22 +23,53 @@
namespace pyarb {
+// Convert a cell description inside a Python object to a cell description in a
+// unique_any, as required by the recipe interface.
+// This helper is only to be called while holding the GIL. We require this guard
+// across the lifetime of the description object `o`, since this function can be
+// called without holding the GIL, ie from `simulation::init`, and `o` is a
+// python object that can only be destroyed while holding the GIL. The fact that
+// `cell_description` has a scoped GIL does not help with destruction as it
+// happens outside that scope. `Description` needs to be extended in Python,
+// inheriting from the pybind11 class.
+static arb::util::unique_any convert_cell(pybind11::object o) {
+ using pybind11::isinstance;
+ using pybind11::cast;
+
+ if (isinstance<arb::spike_source_cell>(o)) {
+ return arb::util::unique_any(cast<arb::spike_source_cell>(o));
+ }
+ if (isinstance<arb::benchmark_cell>(o)) {
+ return arb::util::unique_any(cast<arb::benchmark_cell>(o));
+ }
+ if (isinstance<arb::lif_cell>(o)) {
+ return arb::util::unique_any(cast<arb::lif_cell>(o));
+ }
+ if (isinstance<arb::cable_cell>(o)) {
+ return arb::util::unique_any(cast<arb::cable_cell>(o));
+ }
+
+ throw pyarb_error("recipe.cell_description returned \""
+ + std::string(pybind11::str(o))
+ + "\" which does not describe a known Arbor cell type");
+}
+
// The py::recipe::cell_decription returns a pybind11::object, that is
// unwrapped and copied into a arb::util::unique_any.
-arb::util::unique_any py_recipe_shim::get_cell_description(arb::cell_gid_type gid) const {
- return try_catch_pyexception(
- [&](){ return convert_cell(impl_->cell_description(gid)); },
- "Python error already thrown");
+ arb::util::unique_any py_recipe_shim::get_cell_description(arb::cell_gid_type gid) const {
+ return try_catch_pyexception([&](){
+ pybind11::gil_scoped_acquire guard;
+ return convert_cell(impl_->cell_description(gid));
+ },
+ "Python error already thrown");
}
-std::vector<arb::event_generator> convert_gen(std::vector<pybind11::object> pygens, arb::cell_gid_type gid) {
+// This helper is only to called while holding the GIL, see above.
+static std::vector<arb::event_generator> convert_gen(std::vector<pybind11::object> pygens, arb::cell_gid_type gid) {
using namespace std::string_literals;
using pybind11::isinstance;
using pybind11::cast;
- // Aquire the GIL because it must be held when calling isinstance and cast.
- pybind11::gil_scoped_acquire guard;
-
std::vector<arb::event_generator> gens;
gens.reserve(pygens.size());
@@ -58,9 +91,11 @@ std::vector<arb::event_generator> convert_gen(std::vector<pybind11::object> pyge
}
std::vector<arb::event_generator> py_recipe_shim::event_generators(arb::cell_gid_type gid) const {
- return try_catch_pyexception(
- [&](){ return convert_gen(impl_->event_generators(gid), gid); },
- "Python error already thrown");
+ return try_catch_pyexception([&](){
+ pybind11::gil_scoped_acquire guard;
+ return convert_gen(impl_->event_generators(gid), gid);
+ },
+ "Python error already thrown");
}
std::string con_to_string(const arb::cell_connection& c) {
--
GitLab