Python bindings leak the QofSession/QofBook (and Query)

[array_hourly_0u at icloud.com]

Hi all,

Following the qof_session_end() lock work, I found a related but
separate issue in the Python bindings and I'd like to check the
intended direction before attempting a PR.  I have a known workaround
and I'm not sure I have the SWIG skills to get this right.  I filed a
bug and said there I'd send a follow-up email -- finally getting to
it.

Bug 799788 tracks the leak: https://bugs.gnucash.org/show_bug.cgi?id=799788

The leak: a Python Session leaks its underlying C QofSession -- and
the entire QofBook loaded into it -- unless it is explicitly
destroyed, either by calling .destroy() or by using the `with` context
manager (which since commit bdbc08e0ab calls destroy() on exit). There
is no __del__ on the Python Session wrapper (nor on the base wrapper
class), and qof_session_new is not %newobject, so a Session that is
simply ended and dropped -- or just garbage-collected -- never calls
qof_session_destroy. The same applies to Query (qof_query_create /
qof_query_destroy).


Because GnuCash loads the whole book into memory, the Session leak
scales with book size. I measured an open+load+end loop (gc each
iteration, no destroy): it leaks roughly a full in-memory copy of the
book per session -- about 140 KB for an empty book, about 3.6 MB for a
2000-account book (~1.7 KB/account), and a 5000-account loop
OOM-killed the process. Query leaks a smaller fixed amount per query
(~0.4 KB) but accumulates in loops (reports/searches). For a
long-running or looping consumer this adds up quickly.

To be clear on prior art:
https://github.com/Gnucash/gnucash/commit/bdbc08e0abaf1cbd88a34145f933ed266c8051db
fixed the *lock* symptom for the `with` path, but the memory is still
leaked for plain `s = Session(...); ...; s.end()` without a destroy(),
and for bare garbage collection.


I can sort of see two ways to fix it, but not totally clear on either:

1. A guarded __del__ on the Python Session/Query wrappers: make
destroy() idempotent and add a __del__ that calls it -- but only when
the wrapper created the object, so a wrapper around a borrowed
instance (e.g. Session(instance=gnc_get_current_session())) never
destroys something it doesn't own. To keep a borrowed wrapper safe
when the owner is another Python wrapper, the borrower would also hold
a keepalive reference to the owner.

2. SWIG ownership: %newobject qof_session_new plus %delobject
qof_session_destroy (and an %extend destructor so the proxy frees via
qof_session_destroy rather than free()), and likewise for Query. This
is the native thisown model and removes the hand-rolled
ownership/idempotency bookkeeping, but the Python .i currently uses no
ownership idioms at all (everything is opaque, thisown=0, via
GNC_ACCEPT_WRAPPER), so this would introduce a new pattern for just
these two types.


Both approaches share one inherent caveat: GC-freeing a singly-owned C
object means that if an owner is collected while a separate borrowed
wrapper (holding a raw instance pointer) is still alive, the borrower
is left dangling. That edge is presumably why the current fix used
explicit __exit__ rather than a finalizer.


Is a finalizer wanted here at all, or is explicit lifetime management
(with / destroy) the intended contract for the Python bindings? And if
a finalizer is acceptable, do you prefer the Python-wrapper approach
or the SWIG thisown approach?


Thanks for all your work on GnuCash.


Best,

Noah