Commit 1b41edc3 authored by T.J. Corona's avatar T.J. Corona

Properly handle memory between C++ and Python for operations

To allow smtk to manipulate Python operations as C++ objects, we
originally held a reference to the Python operation within the C++
instance to prevent Python from garbage collecting the object. This
resulted in a circular dependency where the Python object would never
be deleted, causing memory overlap issues when multiple Python
operations were used. This MR moves the logic for keeping the Python
representation of an operation into a specialized typecaster for
operations, breaking the cyclic dependency and allowing C++ and Python
to safely free operations when they go out of scope.
parent 22b4aef2
......@@ -132,8 +132,9 @@ bool ImportPythonOperation::importOperation(
smtk::operation::Operation::Index index = std::hash<std::string>{}(typeName);
auto create = std::bind(smtk::operation::PyOperation::create, moduleName, opName, index);
return manager.registerOperation(
Metadata(typeName, index, create()->createSpecification(), create));
auto specification = create()->createSpecification();
return manager.registerOperation(Metadata(typeName, index, specification, create));
}
ImportPythonOperation::Result ImportPythonOperation::operateInternal()
......
......@@ -25,6 +25,62 @@ SMTK_THIRDPARTY_POST_INCLUDE
#include "smtk/common/PythonInterpreter.h"
namespace pybind11
{
namespace detail
{
template <>
struct type_caster<std::shared_ptr<smtk::operation::Operation> >
{
PYBIND11_TYPE_CASTER(std::shared_ptr<smtk::operation::Operation>, _("Operation"));
using OperationCaster = copyable_holder_caster<smtk::operation::Operation,
std::shared_ptr<smtk::operation::Operation> >;
bool load(handle src, bool b)
{
OperationCaster oc;
bool success = oc.load(src, b);
if (!success)
{
return false;
}
auto py_obj = reinterpret_borrow<object> (src);
auto base_ptr = static_cast<std::shared_ptr<smtk::operation::Operation> > (oc);
// Construct a shared_ptr to the object
auto py_obj_ptr = std::shared_ptr<object>{
new object{py_obj},
[](object* py_object_ptr)
{
// It's possible that when the shared_ptr dies we won't have the
// gil (if the last holder is in a non-Python thread), so we
// make sure to acquire it in the deleter.
gil_scoped_acquire gil;
delete py_object_ptr;
}
};
value = std::shared_ptr<smtk::operation::Operation> (py_obj_ptr, base_ptr.get());
return true;
}
static handle cast (std::shared_ptr<smtk::operation::Operation> base,
return_value_policy rvp,
handle h)
{
return OperationCaster::cast (base, rvp, h);
}
};
template <>
struct is_holder_type<smtk::operation::Operation,
std::shared_ptr<smtk::operation::Operation> > : std::true_type {};
}
}
namespace smtk
{
namespace operation
......@@ -35,7 +91,9 @@ public:
PyOperation() : Operation() {}
virtual ~PyOperation() {}
static std::shared_ptr<smtk::operation::Operation> create(std::string modulename, std::string className, smtk::operation::Operation::Index index)
static std::shared_ptr<smtk::operation::Operation> create(std::string modulename,
std::string className,
smtk::operation::Operation::Index index)
{
// Import the module containing our operation
pybind11::module module = pybind11::module::import(modulename.c_str());
......@@ -43,53 +101,18 @@ public:
// Create an instance of our operation
pybind11::object obj = module.attr(className.c_str())();
if (smtk::common::PythonInterpreter::instance().isEmbedded())
{
// If we are running in embedded mode, we allow the C++ side to perform
// memory management. We do this by internally incrementing the
// python-side object's internel reference count to ensure that the
// python environment does not delete our operation out from under us.
obj.inc_ref();
std::shared_ptr<smtk::operation::PyOperation> op(obj.cast<smtk::operation::PyOperation*>());
// Have the instance hold onto its python object, so when the instance
// is removed so is the tether to the python object.
op->setObject(obj);
// For C++ operations, index() is a compile-time intrinsic of the
// operation class. Python operations only come into existence at runtime,
// though, so we need to manually set a python operation's index.
op->setIndex(index);
// The precedent for python operation names is estabilished in
// ImportPythonOperation to be the modulename.className
// We follow that convention here.
op->setTypeName(modulename + "." + className);
return std::static_pointer_cast<smtk::operation::Operation>(op);
}
else
{
// Have the instance hold onto its python object, so when the instance
// is removed so is the tether to the python object.
obj.cast<std::shared_ptr<smtk::operation::PyOperation> >()->setObject(obj);
// For C++ operations, index() is a compile-time intrinsic of the
// operation class. Python operations only come into existence at runtime,
// though, so we need to manually set a python operation's index.
obj.cast<std::shared_ptr<smtk::operation::PyOperation> >()->setIndex(index);
// The precedent for python operation names is estabilished in
// ImportPythonOperation to be the modulename.className
// We follow that convention here.
obj.cast<std::shared_ptr<smtk::operation::PyOperation> >()->setTypeName(modulename + "." + className);
// If we are running in a native python instance (i.e. not our embedded
// instance), then memory management is handled by python. We need only
// to cast our python object into a shared_ptr so the SMTK operation
// handling system can use it.
return obj.cast<std::shared_ptr<smtk::operation::Operation> >();
}
// For C++ operations, index() is a compile-time intrinsic of the
// operation class. Python operations only come into existence at runtime,
// though, so we need to manually set a python operation's index.
obj.cast<std::shared_ptr<smtk::operation::PyOperation> >()->setIndex(index);
// The precedent for python operation names is estabilished in
// ImportPythonOperation to be the modulename.className
// We follow that convention here.
obj.cast<std::shared_ptr<smtk::operation::PyOperation> >()->setTypeName(
modulename + "." + className);
return obj.cast<std::shared_ptr<smtk::operation::Operation> >();
}
Index index() const override { return m_index; }
......@@ -147,13 +170,11 @@ private:
Specification createSpecification() override
{ PYBIND11_OVERLOAD_PURE(Specification, Operation, createSpecification); }
void setObject(pybind11::object obj) { m_object = obj; }
void setIndex(Index index) { m_index = index; }
void setTypeName(const std::string& typeName) { m_typeName = typeName; }
Result operateInternalPy() { PYBIND11_OVERLOAD_PURE(Result, Operation, operateInternal, ); }
pybind11::object m_object;
Index m_index;
std::string m_typeName;
};
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment