From f30c420496268ff7b0a533b5c8cfc0087660aeb4 Mon Sep 17 00:00:00 2001
From: akuesters <42005107+akuesters@users.noreply.github.com>
Date: Wed, 2 Oct 2019 15:09:35 +0200
Subject: [PATCH] Python wrapper: thread safe recipe (#882)
Ensure that errors in Python callbacks that are called from multithreaded C++ code propogate the correct Python error back to the parent Python callback site, and that no callbacks are called from other threads if an error has already ocurred.
- protects each recipe callback with a mutex, stores python exception, catches and throws python exception if occured
- methods calling recipe (in simulation and partition_load_balance) are protected as well by try catch, resets and rethrows python exception (if occured) or else throws C++ exception
fixes #792
---
python/CMakeLists.txt | 1 +
python/domain_decomposition.cpp | 9 ++++++++-
python/error.cpp | 18 ++++++++++++++++++
python/error.hpp | 26 ++++++++++++++++++++++++++
python/recipe.cpp | 16 +++++++++-------
python/recipe.hpp | 22 ++++++++++++----------
python/simulation.cpp | 8 +++++++-
7 files changed, 81 insertions(+), 19 deletions(-)
create mode 100644 python/error.cpp
diff --git a/python/CMakeLists.txt b/python/CMakeLists.txt
index ed87571f..ad34fce7 100644
--- a/python/CMakeLists.txt
+++ b/python/CMakeLists.txt
@@ -20,6 +20,7 @@ add_library(pyarb MODULE
config.cpp
context.cpp
domain_decomposition.cpp
+ error.cpp
event_generator.cpp
identifiers.cpp
morphology.cpp
diff --git a/python/domain_decomposition.cpp b/python/domain_decomposition.cpp
index 3b43d215..a8771013 100644
--- a/python/domain_decomposition.cpp
+++ b/python/domain_decomposition.cpp
@@ -10,6 +10,7 @@
#include <arbor/load_balance.hpp>
#include "context.hpp"
+#include "error.hpp"
#include "recipe.hpp"
#include "strprintf.hpp"
@@ -104,7 +105,13 @@ void register_domain_decomposition(pybind11::module& m) {
// The Python recipe has to be shimmed for passing to the function that takes a C++ recipe.
m.def("partition_load_balance",
[](std::shared_ptr<py_recipe>& recipe, const context_shim& ctx, arb::partition_hint_map hint_map) {
- return arb::partition_load_balance(py_recipe_shim(recipe), ctx.context, std::move(hint_map));
+ try {
+ return arb::partition_load_balance(py_recipe_shim(recipe), ctx.context, std::move(hint_map));
+ }
+ catch (...) {
+ py_reset_and_throw();
+ throw;
+ }
},
"Construct a domain_decomposition that distributes the cells in the model described by recipe\n"
"over the distributed and local hardware resources described by context.\n"
diff --git a/python/error.cpp b/python/error.cpp
new file mode 100644
index 00000000..a4d8c48a
--- /dev/null
+++ b/python/error.cpp
@@ -0,0 +1,18 @@
+#include <pybind11/pybind11.h>
+
+#include "error.hpp"
+
+namespace pyarb {
+
+std::exception_ptr py_exception;
+std::mutex py_callback_mutex;
+
+void py_reset_and_throw() {
+ if (py_exception) {
+ std::exception_ptr copy = py_exception;
+ py_exception = nullptr;
+ std::rethrow_exception(copy);
+ }
+}
+
+} // namespace pyarb
diff --git a/python/error.hpp b/python/error.hpp
index f3b0cb6e..d475f2fa 100644
--- a/python/error.hpp
+++ b/python/error.hpp
@@ -1,10 +1,14 @@
#pragma once
+#include <mutex>
#include <stdexcept>
#include <string>
namespace pyarb {
+extern std::exception_ptr py_exception;
+extern std::mutex py_callback_mutex;
+
// Python wrapper errors
struct pyarb_error: std::runtime_error {
@@ -19,4 +23,26 @@ void assert_throw(bool pred, const char* msg) {
if (!pred) throw pyarb_error(msg);
}
+// This function resets a python exception to nullptr
+// and rethrows a copy of the set python exception.
+// It should be used in serial code
+// just before handing control back to Python.
+void py_reset_and_throw();
+
+template <typename L>
+auto try_catch_pyexception(L func, const char* msg){
+ std::lock_guard<std::mutex> g(py_callback_mutex);
+ try {
+ if(!py_exception) {
+ return func();
+ }
+ else {
+ throw pyarb_error(msg);
+ }
+ }
+ catch (pybind11::error_already_set& e) {
+ py_exception = std::current_exception();
+ throw;
+ }
+}
} // namespace pyarb
diff --git a/python/recipe.cpp b/python/recipe.cpp
index 0f503269..35283549 100644
--- a/python/recipe.cpp
+++ b/python/recipe.cpp
@@ -24,8 +24,9 @@ namespace pyarb {
// 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 {
- pybind11::gil_scoped_acquire guard;
- return convert_cell(impl_->cell_description(gid));
+ return try_catch_pyexception(
+ [&](){ return convert_cell(impl_->cell_description(gid)); },
+ "Python error already thrown");
}
arb::probe_info cable_probe(std::string kind, arb::cell_member_type id, arb::mlocation loc) {
@@ -44,7 +45,7 @@ arb::probe_info cable_probe(std::string kind, arb::cell_member_type id, arb::mlo
return arb::probe_info{id, pkind, probe};
};
-std::vector<arb::event_generator> py_recipe_shim::event_generators(arb::cell_gid_type gid) const {
+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;
@@ -52,9 +53,6 @@ std::vector<arb::event_generator> py_recipe_shim::event_generators(arb::cell_gid
// Aquire the GIL because it must be held when calling isinstance and cast.
pybind11::gil_scoped_acquire guard;
- // Get the python list of pyarb::event_generator_shim from the python front end.
- auto pygens = impl_->event_generators(gid);
-
std::vector<arb::event_generator> gens;
gens.reserve(pygens.size());
@@ -75,7 +73,11 @@ std::vector<arb::event_generator> py_recipe_shim::event_generators(arb::cell_gid
return gens;
}
-// TODO: implement py_recipe_shim::probe_info
+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");
+}
std::string con_to_string(const arb::cell_connection& c) {
return util::pprintf("<arbor.connection: source ({},{}), destination ({},{}), delay {}, weight {}>",
diff --git a/python/recipe.hpp b/python/recipe.hpp
index 3e349242..ce1ef326 100644
--- a/python/recipe.hpp
+++ b/python/recipe.hpp
@@ -122,8 +122,10 @@ public:
py_recipe_shim(std::shared_ptr<py_recipe> r): impl_(std::move(r)) {}
+ const char* msg = "Python error already thrown";
+
arb::cell_size_type num_cells() const override {
- return impl_->num_cells();
+ return try_catch_pyexception([&](){ return impl_->num_cells(); }, msg);
}
// The pyarb::recipe::cell_decription returns a pybind11::object, that is
@@ -131,40 +133,40 @@ public:
arb::util::unique_any get_cell_description(arb::cell_gid_type gid) const override;
arb::cell_kind get_cell_kind(arb::cell_gid_type gid) const override {
- return impl_->cell_kind(gid);
+ return try_catch_pyexception([&](){ return impl_->cell_kind(gid); }, msg);
}
arb::cell_size_type num_sources(arb::cell_gid_type gid) const override {
- return impl_->num_sources(gid);
+ return try_catch_pyexception([&](){ return impl_->num_sources(gid); }, msg);
}
arb::cell_size_type num_targets(arb::cell_gid_type gid) const override {
- return impl_->num_targets(gid);
+ return try_catch_pyexception([&](){ return impl_->num_targets(gid); }, msg);
}
arb::cell_size_type num_gap_junction_sites(arb::cell_gid_type gid) const override {
- return impl_->num_gap_junction_sites(gid);
+ return try_catch_pyexception([&](){ return impl_->num_gap_junction_sites(gid); }, msg);
}
std::vector<arb::event_generator> event_generators(arb::cell_gid_type gid) const override;
std::vector<arb::cell_connection> connections_on(arb::cell_gid_type gid) const override {
- return impl_->connections_on(gid);
+ return try_catch_pyexception([&](){ return impl_->connections_on(gid); }, msg);
}
std::vector<arb::gap_junction_connection> gap_junctions_on(arb::cell_gid_type gid) const override {
- return impl_->gap_junctions_on(gid);
+ return try_catch_pyexception([&](){ return impl_->gap_junctions_on(gid); }, msg);
}
arb::cell_size_type num_probes(arb::cell_gid_type gid) const override {
- return impl_->num_probes(gid);
+ return try_catch_pyexception([&](){ return impl_->num_probes(gid); }, msg);
}
arb::probe_info get_probe(arb::cell_member_type id) const override {
- return impl_->get_probe(id);
+ return try_catch_pyexception([&](){ return impl_->get_probe(id); }, msg);
}
- // TODO: wrap
+ // TODO: wrap and make thread safe
arb::util::any get_global_properties(arb::cell_kind kind) const override {
if (kind==arb::cell_kind::cable) {
arb::cable_cell_global_properties gprop;
diff --git a/python/simulation.cpp b/python/simulation.cpp
index 60137614..03b01906 100644
--- a/python/simulation.cpp
+++ b/python/simulation.cpp
@@ -22,7 +22,13 @@ void register_simulation(pybind11::module& m) {
// before forwarding it to the arb::recipe constructor.
.def(pybind11::init(
[](std::shared_ptr<py_recipe>& rec, const arb::domain_decomposition& decomp, const context_shim& ctx) {
- return new arb::simulation(py_recipe_shim(rec), decomp, ctx.context);
+ try {
+ return new arb::simulation(py_recipe_shim(rec), decomp, ctx.context);
+ }
+ catch (...) {
+ py_reset_and_throw();
+ throw;
+ }
}),
// Release the python gil, so that callbacks into the python recipe don't deadlock.
pybind11::call_guard<pybind11::gil_scoped_release>(),
--
GitLab