PowderPattern is never freed once a diffraction component is added

[clemisch]

Hi, I think there is a reference cycle in the PowderPattern bindings that keeps patterns alive forever.

Adding a diffraction component makes the pattern and the component hold references to each other. boost.python objects are invisible to Python's cycle collector, so nothing can ever break the loop and neither object is freed. The crystal is held too.

This is annoying for large batched refinements. I was able to circumvent it somehow by manually managing a persistent pool of PowderPatterns, but a root fix would be a lot cleaner.

Repro:

import gc, weakref
import numpy as np
from pyobjcryst.crystal import Crystal
from pyobjcryst.powderpattern import PowderPattern

def make(attach):
    pp = PowderPattern()
    pp.SetWavelength(1.0)
    pp.SetPowderPatternX(np.radians(np.linspace(5., 60., 4000)))
    if attach:
        pp.AddPowderPatternDiffraction(Crystal(4., 4., 4., "P1"))
    return pp

for attach in (False, True):
    pp = make(attach); ref = weakref.ref(pp)
    del pp; gc.collect()   # try to garbage collect the PowderPattern
    print(f"attached: {attach} -> collected: {ref() is None}")

gives

attached: False -> collected: True  # garbage collected if no pdiff attached
attached: True -> collected: False  # not garbage collected if pdiff attached

A plain PowderPattern is collected fine, so it looks specific to AddPowderPatternDiffraction.

The cause seems to be the call policy at src/extensions/powderpattern_ext.cpp:217:

with_custodian_and_ward_postcall<1,0,
  with_custodian_and_ward_postcall<0,2,return_internal_reference<> > > ()

With 0 as the return value, 1 as self and 2 as the crystal argument: the pattern keeps the returned component alive, and return_internal_reference makes the component keep the pattern alive. That is the loop. The middle policy (component keeps the crystal alive) looks necessary, since the component stores a raw pointer to the crystal, and I assume the first one is there so that protection still holds when callers discard the return value, which is common.

Why it matters in practice:

• Memory grows in anything long lived that builds patterns repeatedly, such as notebook kernels, GUIs, or batch jobs over many datasets. About 94 kB per pattern above, more for longer patterns.
• gCrystalRegistry never returns to zero. CreateCrystalFromCIF skips a data block when it has no atoms and the registry is non-empty, so importing a cell-only CIF can depend on how many patterns you happened to build earlier in the session.
• There is no workaround from Python. DeRegisterAll() resets the count but frees nothing, since DeRegister does not own the objects.

One idea for a fix: tie the crystal to the pattern directly instead of going through the return value, so the pattern no longer holds the component.

.def("AddPowderPatternDiffraction", &addppdiffraction,
        with_custodian_and_ward<1, 2, return_internal_reference<> >())

Both guarantees seem preserved (the crystal outlives the pattern that points at it, and a returned handle cannot outlive its owner) but the loop is gone. It is the style already used for AddScatterer in crystal_ext.cpp, without the reverse direction.

I have not tested that, so I am raising this as a question rather than a PR. Two things I am unsure about: with_custodian_and_ward runs before the call while the current policies run after, and the change would affect every caller, so anything relying on a dropped component staying alive would need checking. Getting that wrong would crash rather than raise.

Also, I don't know how that connects with the planned switch to pybind or nanobind.

[vincefn]

I tested the proposed fix, but without success - I'm afraid using:
with_custodian_and_ward_postcall<1,0, with_custodian_and_ward_postcall<0,1,...
creates a circular reference, so it won't work.

what is needed:

• the PowderPatternDiffraction object depends on the Crystal and the PowderPattern
• the PowderPattern also depends on the added PowderPatternDiffraction - unless it's removed as a component

The problem is, once a custodian/ward relationship has been made, there is no way to remove it.

What makes things rather messy is the 3 layers of dependency handling:

• the python refs
• boost's custodian/ward
• objcryst's (De)RegisterClient and {Add|Remove}SubRefObj (which in retrospect is an odd implementation)

I think the way to handle this will probably be to avoid the hardwired custodian/ward, only rely on custom python refs, and find a way to follow the Client/SubRefObj objcryst policies, but I'm not sure how to achieve this...

I tested a few ways a long time ago, and I think the unsatisfactory implementation is the reason for the lack of the RemovePowderPatternComponent - because it led to crashes.

For now when handling many powder patterns the only clean way is to reuse the PowderPattern object - assuming the size is the same, and the crystalline phases also are the same (which is more questionable), to limit memory usage.

[vincefn]

(hmm I think I got lost in. along series of test, so the above with_custodian_and_ward_postcall test is not what you proposed - but I don't think tying the crystal to the powderpattern is correct)

[vincefn]

And testing you solution - it does work in your case, where you are changing a lot of powder patterns. But imagine a different case, where you have a single powder pattern and you want to test many possible crystal structures (for QPA), then it would have the adverse effect and keep all the crystal objects in memory. Here again maybe using python refs instead may be better ?

[vincefn]

Lifetime management notes for issue #95 (Boost.Python vs ObjCryst client graph vs nanobind)

1) Can ObjCryst RegisterClient / DeRegisterClient automatically drive Python refcounts (Py_INCREF/Py_DECREF)?

Short answer: not automatically.

• RegisterClient/DeRegisterClient are C++-side dependency mechanisms in ObjCryst.
• Boost.Python with_custodian_and_ward is a Python-wrapper-level keepalive mechanism (it adds Python reference edges).
• There is no built-in bridge where ObjCryst client registration automatically increments/decrements Python wrapper refcounts.

In principle, one could add such a bridge in bindings (look up corresponding PyObject* for C++ objects and call Py_INCREF/Py_DECREF), but this is fragile because ObjCryst client registration is used internally for C++ behavior, not exclusively for Python lifetime semantics. Coupling those directly risks incorrect refcount changes from internal C++ calls.

2) Would switching to nanobind change this picture?

Short answer: yes, somewhat, but it does not remove the need for careful lifetime design.

• nanobind offers cleaner, more modern lifetime/ownership controls than Boost.Python (e.g., easier-to-reason-about keep_alive usage and holder strategies).
• However, keepalive relationships are still Python reference relationships; cycles and teardown complexity can still exist if modeled poorly.
• The key benefit would be implementation clarity and flexibility, especially for designing reversible/explicit lifetime bookkeeping instead of relying on Boost’s one-way declarative custodian/ward policies.

Practical takeaway for #95

• The current Boost.Python lifetime constraints are hard to “undo” once declared.
• Reusing ObjCryst RegisterClient directly as a refcount driver is possible but risky due to mixed responsibilities.
• A future nanobind migration could make robust lifetime modeling easier, but correctness will still depend on explicit graph design (parent/component/crystal/client relationships) rather than the binding library alone.