Python garbage collection

Hi,

There are still some minor issues with python garbage collection. I
briefly talked about this with Marcus on IRC but I thought it would be
a good idea to write this down and get some feedback.

  1. python scripts return objects which may be destroyed before we use them

    a) PythonExtension::performAction

Should return a pointer to QUndoCommand. Creating a derived
QUndoCommand derived class in python and returning this works fine as
long as the command isn’t destroyed by python garbage collection. But
even if the script holds a reference, reloading plugins will destroy
the command! However, we still want the command to work when we undo
the last action.

QUndoCommand is a simple class, it doesn’t inherit QObject which means
we can’t connect to a destroyed() signal. Since it is such a simple
class I think the best way to handle this would be to create a
Avogadro.UndoCommand class in the python bindings. This class would
inherit QUndoCommand but boost python allows us to hold the object
using a std::auto_ptr. Next we can release (take ownership) of this
auto_ptr in the PythonExtension::performAction C++ function.

b) PythonTool::settingsWidget & PythonEngine::settingsWidget

This has been solved I think, both PythonTool and PythonEngine wrap
the QWidget extracted (using boost::python::extract) in a C++ created
QWidget. So even when the python script returns a QWidget without
holding a reference to it, the wrapper QWidget is still valid and we
don’t segfault… (note: a Tool or Engine’s settings QWidget never has
to live longer than the plugin itself)

  1. Who owns or should own Molecule(s)?

While we could add a MoleculeStack or something similar to libavogadro
to own Molecules (currently MainWindow owns molecules), I would just
remove python functions like GLWidget::setMolecule and add a copy
function to the python bindings.

This would look like:

self.molCopy = Avogadro.Molecule(glwidget.molecule) # OK! this
molecule will never be used outside script

glwidget.molecule.copy(self.molCopy) # We don’t actually change the molecule…

Currently we allow:

glwidget.molecule = Avogadro.Molecule(self.molCopy) # this calls
GLWidget::setMolecule, the new Molecule is destroyed right away…

We could make this work using an std::auto_ptr which we release in a
function overloading [1] GLWidget::setMolecule in the python bindings,
but even then it doesn’t make much sense. For example, we have no way
to tell MainWindow the molecule has changed.

[1] In case anyone wondered, we can overload any function in the
python bindings like this:

// the first parameter is the object (like self in python)
void GLWidget_setMolecule(GLWidget &glwidget, std::auto_ptr mol)
{
glwidget.setMolecule(mol.get());
mol.release();
}

class_<Avogadro::GLWidget, boost::noncopyable>(“GLWidget”)
.add_property(“molecule”, /* get method left out */, &GLWidget_setMolecule)

Suggestions?

Thanks,
Tim

Hi,

You can ignore point 1. We can just transfer ownership from python to
C++ using the sip api. We can do this for all PyQt classes:

QAction(s) returned by actions()
QUndoCommand returned by performAction()
QWidget returned by settingsWidget()

The python script no longer has to hold a reference:

class ClearCommand(QUndoCommand):
def init(self, glwidget):

class Extension(QObject):

def performAction(self, action, glwidget):
return ClearCommand(glwidget) # works fine now!

I’m having doubts about point 2 though. What happens if you want to
run a standalone script? At some point you would need to create a
molecule and call GLWidget::setMolecule. If this happens in a function
like readMolecule (below), we need to hold a reference to the
molecule.

class MyAppWindow(QMainWindow):
def init(self):
self.glwidget = Avogadro.GLWidget()

def readMolecule(self, filename)
mol = Avogadro.Molecule()
self.glwidget.molecule = mol # crash after return from function

  self.mol =  Avogadro.Molecule()
  self.glwidget.molecule = self.mol  # works....

This still leaves MainWindow and ToolGroup in the dark about the change…

Sorry for the long vague mails but python support is getting closer
and closer to 1.0 :slight_smile:

Tim

On Tue, Feb 10, 2009 at 9:28 PM, Tim Vandermeersch
tim.vandermeersch@gmail.com wrote:

Hi,

There are still some minor issues with python garbage collection. I
briefly talked about this with Marcus on IRC but I thought it would be
a good idea to write this down and get some feedback.

  1. python scripts return objects which may be destroyed before we use them

    a) PythonExtension::performAction

Should return a pointer to QUndoCommand. Creating a derived
QUndoCommand derived class in python and returning this works fine as
long as the command isn’t destroyed by python garbage collection. But
even if the script holds a reference, reloading plugins will destroy
the command! However, we still want the command to work when we undo
the last action.

QUndoCommand is a simple class, it doesn’t inherit QObject which means
we can’t connect to a destroyed() signal. Since it is such a simple
class I think the best way to handle this would be to create a
Avogadro.UndoCommand class in the python bindings. This class would
inherit QUndoCommand but boost python allows us to hold the object
using a std::auto_ptr. Next we can release (take ownership) of this
auto_ptr in the PythonExtension::performAction C++ function.

b) PythonTool::settingsWidget & PythonEngine::settingsWidget

This has been solved I think, both PythonTool and PythonEngine wrap
the QWidget extracted (using boost::python::extract) in a C++ created
QWidget. So even when the python script returns a QWidget without
holding a reference to it, the wrapper QWidget is still valid and we
don’t segfault… (note: a Tool or Engine’s settings QWidget never has
to live longer than the plugin itself)

  1. Who owns or should own Molecule(s)?

While we could add a MoleculeStack or something similar to libavogadro
to own Molecules (currently MainWindow owns molecules), I would just
remove python functions like GLWidget::setMolecule and add a copy
function to the python bindings.

This would look like:

self.molCopy = Avogadro.Molecule(glwidget.molecule) # OK! this
molecule will never be used outside script

glwidget.molecule.copy(self.molCopy) # We don’t actually change the molecule…

Currently we allow:

glwidget.molecule = Avogadro.Molecule(self.molCopy) # this calls
GLWidget::setMolecule, the new Molecule is destroyed right away…

We could make this work using an std::auto_ptr which we release in a
function overloading [1] GLWidget::setMolecule in the python bindings,
but even then it doesn’t make much sense. For example, we have no way
to tell MainWindow the molecule has changed.

[1] In case anyone wondered, we can overload any function in the
python bindings like this:

// the first parameter is the object (like self in python)
void GLWidget_setMolecule(GLWidget &glwidget, std::auto_ptr mol)
{
glwidget.setMolecule(mol.get());
mol.release();
}

class_<Avogadro::GLWidget, boost::noncopyable>(“GLWidget”)
.add_property(“molecule”, /* get method left out */, &GLWidget_setMolecule)

Suggestions?

Thanks,
Tim