[PyROOT] Set memory policy to "strict"

PyROOT has many memory leaks, which is a major pain point for people
using it for long-running scripts in batch jobs.

One source of memory leaks was indentified to be the "heuristic memory
policy" of PyROOT. This means that PyROOT assumes that every non-const
pointer member function argument was interpreted as the object taking
ownership if the argument.

For examle, take the non-owning RooLinkedList container. It has a
`RooLinkedList::Add(RooAbsArg *arg)` method. PyROOT wrongly assumes that
this means the RooLinkedList takes ownership of arg, and it drops the
PyROOT overship. Nobody feels responsible for deleting the object
anymore, and there is a memory leak or `arg`.

That particular leak was reported in this forum post:
https://root-forum.cern.ch/t/memory-leak-in-fits/56249

Function parameters of type `T *` are very common in ROOT, and only
rarely do they imply ownership transfer. So changing the memory policy
to "strict" would surely fix also many other memory leaks that are not
reported so far. In fact, upstream cppyy doesn't even have this
heuristic memory policy anymore! So moving PyROOT also to the strict
memory policy closes the gap between PyROOT and cppyy.

The potential drawback of this change are crashes in usercode if memory
is not properly managed. But these problems should either be fixed by:

  * the user

  * dedicated pythonizations for these methods to manage shared
    ownership via Python reference counters (i.e., setting the parameter
    as an attribute of the object that the member function was called
    on)

This follows up on PR #4294, in particular it reverts 3a12063.

Author: guitargeek

README/ReleaseNotes/v632/index.md

@@ -208,6 +208,63 @@ Please use the higher-level functions `RooAbsPdf::createNLL()` and `RooAbsPdf::c
 ## PROOF Libraries
 
 
+## PyROOT
+
+### Changed ownership policy for non-`const` pointer member function parameters
+
+If you have a member function taking a raw pointer, like `MyClass::foo(T *obj)`,
+PyROOT was so far assuming that calling this method on `my_instance`
+transfers the ownership of `obj` to `my_instance`.
+
+However, this resulted in many memory leaks, since many functions with such a
+signature actually don't take ownership of the object.
+
+To avoid such memory leaks, PyROOT now doesn't make this guess anymore as of
+ROOT 6.32. Because of this change, some double-deletes or dangling references
+might creep up in your scripts. These need to be fixedby properly managing
+object lifetime with Python references.
+
+You can fix the dangling references problem for example via:
+
+  1. Assigning the object to a python variable
+  2. Creating an owning collection that keeps the objects alive
+  3. Writing a pythonization for the member function that does the ownership
+     transfer if needed
+  
+The double-delete problems can be fixed via:
+
+  1. Drop the ownership on the Python side with `ROOT.SetOwnership(obj, False)`
+  3. Writing a pythonization for the member function that drops the ownership on the Python side as above
+
+This affects for example the `TList::Add(TObject *obj)` member function, which
+will not transfer ownership from PyROOT to the TList anymore. The new policy
+fixes a memory leak, but at the same time it is not possible anymore to create
+the contained elements in place:
+
+```python
+# A TList is by default a non-owning container
+my_list = ROOT.TList()
+
+
+# This is working, but resulted in memory leak prior to ROOT 6.32:
+obj_1 = ROOT.TObjString("obj_1")
+my_list.Add(obj_1)
+
+
+# This is not allowed anymore, as the temporary would be
+# deleted immediately leaving a dangling pointer:
+my_list.Add(ROOT.TObjString("obj_2"))
+
+# Python reference count to contained object is now zero,
+# TList contains dangling pointer!
+```
+
+**Note:** You can change back to the old policy by calling
+`ROOT.SetMemoryPolicy(ROOT.kMemoryHeuristics)` after importing ROOT, but this
+should be only used for debugging purposes and this function might be removed
+in the future!
+
+
 ## Language Bindings
 
 

bindings/pyroot/pythonizations/CMakeLists.txt

@@ -43,6 +43,7 @@ if(roofit)
         ROOT/_pythonization/_roofit/_roomcstudy.py
         ROOT/_pythonization/_roofit/_roomsgservice.py
         ROOT/_pythonization/_roofit/_roonllvar.py
+        ROOT/_pythonization/_roofit/_rooplot.py
         ROOT/_pythonization/_roofit/_rooprodpdf.py
         ROOT/_pythonization/_roofit/_roorealvar.py
         ROOT/_pythonization/_roofit/_roosimultaneous.py

bindings/pyroot/pythonizations/python/ROOT/_facade.py

@@ -173,11 +173,6 @@ def _finalSetup(self):
         if not self.gROOT.IsBatch() and self.PyConfig.StartGUIThread:
             self.app.init_graphics()
 
-        # Set memory policy to kUseHeuristics.
-        # This restores the default in PyROOT which was changed
-        # by new Cppyy
-        self.SetMemoryPolicy(self.kMemoryHeuristics)
-
         # Redirect lookups to cppyy's global namespace
         self.__class__.__getattr__ = self._fallback_getattr
         self.__class__.__setattr__ = lambda self, name, val: setattr(gbl_namespace, name, val)

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/__init__.py

@@ -51,6 +51,7 @@
 from ._roomcstudy import RooMCStudy
 from ._roomsgservice import RooMsgService
 from ._roonllvar import RooNLLVar
+from ._rooplot import RooPlot
 from ._rooprodpdf import RooProdPdf
 from ._roorealvar import RooRealVar
 from ._roosimultaneous import RooSimultaneous
@@ -82,6 +83,7 @@
     RooMCStudy,
     RooMsgService,
     RooNLLVar,
+    RooPlot,
     RooProdPdf,
     RooRealVar,
     RooSimultaneous,

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooabspdf.py

@@ -31,13 +31,19 @@ def _pack_cmd_args(*args, **kwargs):
     assert len(kwargs) == 0
 
     # Put RooCmdArgs in a RooLinkedList
-    cmdList = ROOT.RooLinkedList()
+    cmd_list = ROOT.RooLinkedList()
     for cmd in args:
         if not isinstance(cmd, ROOT.RooCmdArg):
             raise TypeError("This function only takes RooFit command arguments.")
-        cmdList.Add(cmd)
+        cmd_list.Add(cmd)
 
-    return cmdList
+    # The RooLinkedList passed to functions like fitTo() is expected to be
+    # non-owning. To make sure that the RooCmdArgs live long enough, we attach
+    # then as an attribute of the output list, such that the Python reference
+    # counter doesn't hit zero.
+    cmd_list.owning_pylist = args
+
+    return cmd_list
 
 
 class RooAbsPdf(RooAbsReal):

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooplot.py

@@ -0,0 +1,19 @@
+# Authors:
+# * Jonas Rembser 09/2023
+
+################################################################################
+# Copyright (C) 1995-2020, Rene Brun and Fons Rademakers.                      #
+# All rights reserved.                                                         #
+#                                                                              #
+# For the licensing terms see $ROOTSYS/LICENSE.                                #
+# For the list of contributors see $ROOTSYS/README/CREDITS.                    #
+################################################################################
+
+
+class RooPlot(object):
+    def addObject(self, obj, *args, **kwargs):
+        import ROOT
+
+        # PyROOT transfers the ownership to the RooPlot.
+        ROOT.SetOwnership(obj, False)
+        return self._addObject(obj, *args, **kwargs)

bindings/pyroot/pythonizations/test/tcollection_listmethods.py

@@ -1,7 +1,6 @@
 import unittest
 
 import ROOT
-from libcppyy import SetOwnership
 
 
 class TCollectionListMethods(unittest.TestCase):
@@ -15,12 +14,16 @@ class TCollectionListMethods(unittest.TestCase):
     # Helpers
     def create_tcollection(self):
         c = ROOT.TList()
+        pylist = []
         for _ in range(self.num_elems):
             o = ROOT.TObject()
             # Prevent immediate deletion of C++ TObjects
-            SetOwnership(o, False)
+            ROOT.SetOwnership(o, False)
             c.Add(o)
+            pylist.append(o)
 
+        # To prevent memory leaks
+        c._owned_objects = pylist
         return c
 
     # Tests
@@ -70,6 +73,14 @@ def test_remove(self):
         with self.assertRaises(ValueError):
             c.remove(o1)
 
+        # The TList destructor will call TList::Clear(), which triggers a
+        # ROOT-internal garbage collection routine implemented in
+        # TCollection::GarbageCollect(). This can segfault if Python already
+        # garbage collected the objects in the TList itself, because then the
+        # TList has dangling pointers. To avoid this, we call Clear() now,
+        # before the reference count of the objects in the list goes to zero.
+        c.Clear()
+
     def test_extend(self):
         c1 = self.create_tcollection()
         c2 = self.create_tcollection()
@@ -106,6 +117,14 @@ def test_count(self):
         self.assertEqual(c.count(o1), 2)
         self.assertEqual(c.count(o2), 1)
 
+        # The TList destructor will call TList::Clear(), which triggers a
+        # ROOT-internal garbage collection routine implemented in
+        # TCollection::GarbageCollect(). This can segfault if Python already
+        # garbage collected the objects in the TList itself, because then the
+        # TList has dangling pointers. To avoid this, we call Clear() now,
+        # before the reference count of the objects in the list goes to zero.
+        c.Clear()
+
 
 if __name__ == '__main__':
     unittest.main()

bindings/pyroot/pythonizations/test/tseqcollection_listmethods.py

@@ -15,20 +15,26 @@ class TSeqCollectionListMethods(unittest.TestCase):
     # Helpers
     def create_tseqcollection(self):
         sc = ROOT.TList()
+        sc_pylist = []
         for i in reversed(range(self.num_elems)):
-            sc.Add(ROOT.TObjString(str(i)))
+            sc_pylist.append(ROOT.TObjString(str(i)))
+            sc.Add(sc_pylist[-1])
 
+        # A TList is by default non-owning. To make sure that the objects live
+        # long enough, we attach then as an attribute of the output list, such
+        # that the Python reference counter doesn't hit zero.
+        sc.owning_pylist = sc_pylist
         return sc
 
     # Tests
     def test_insert(self):
         sc = self.create_tseqcollection()
 
         # Insert with positive index
-        o = ROOT.TObject()
-        sc.insert(1, o)
+        o1 = ROOT.TObject()
+        sc.insert(1, o1)
         self.assertEqual(sc.GetEntries(), self.num_elems + 1)
-        self.assertEqual(sc.At(1), o)
+        self.assertEqual(sc.At(1), o1)
 
         # Insert with negative index (starts from end)
         o2 = ROOT.TObject()
@@ -84,7 +90,8 @@ def test_pop(self):
             sc2.pop(1.0)
 
         # Pop a repeated element
-        sc2.append(ROOT.TObjString('2'))
+        o1 = ROOT.TObjString('2')
+        sc2.append(o1)
         elem = sc2.pop()
         self.assertEqual(sc2.At(0), elem)
 
@@ -145,9 +152,9 @@ def test_index(self):
             self.assertEqual(sc.index(elem), i)
 
         # Check element not in collection
-        o = ROOT.TObjString(str(self.num_elems))
+        o1 = ROOT.TObjString(str(self.num_elems))
         with self.assertRaises(ValueError):
-            sc.index(o)
+            sc.index(o1)
 
 
 if __name__ == '__main__':

tutorials/fit/combinedFit.py

@@ -122,14 +122,16 @@ def __call__(self, par):
 fB.SetFitResult(result, iparB)
 fB.SetRange(rangeB().first, rangeB().second)
 fB.SetLineColor(ROOT.kBlue)
-hB.GetListOfFunctions().Add(fB)
+# The function list takes ownership of the added elements, so we are cloning
+# the function to avoid a double delete.
+hB.GetListOfFunctions().Add(fB.Clone())
 hB.Draw()
 
 c1.cd(2)
 fSB.SetFitResult(result, iparSB)
 fSB.SetRange(rangeSB().first, rangeSB().second)
 fSB.SetLineColor(ROOT.kRed)
-hSB.GetListOfFunctions().Add(fSB)
+hSB.GetListOfFunctions().Add(fSB.Clone())
 hSB.Draw()
 
 c1.SaveAs("combinedFit.png")