Skip to content

Commit d1bf02a

Browse files
committed
[Python] Don't rely on possible memory leaks in T(Seq)Collection tests
Some unit tests rely on the behavior that when adding object to `TLists`, Python ownership is always dropped, resulting in memory leaks if the ownership is not correctly handled on the C++ side. To make these unit tests transparently ready for the moment where the ownership is handled correctly, this commit suggests to use some global lists that keep around Python references until the end of the test. So the effect on the lifetime is the same as having the "controlled" memory leaks.
1 parent 914ff7a commit d1bf02a

3 files changed

Lines changed: 30 additions & 7 deletions

File tree

bindings/pyroot/pythonizations/test/tcollection_listmethods.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,16 @@ class TCollectionListMethods(unittest.TestCase):
1111

1212
num_elems = 3
1313

14+
_global_objects = []
15+
1416
# Helpers
1517
def create_tcollection(self):
1618
c = ROOT.TList()
1719
for _ in range(self.num_elems):
1820
o = ROOT.TObject()
19-
# Prevent immediate deletion of C++ TObjects
20-
ROOT.SetOwnership(o, False)
2121
c.Add(o)
22+
# To prevent deletion of the objects (TList is by default non-owning)
23+
self._global_objects.append(o)
2224

2325
return c
2426

@@ -44,6 +46,10 @@ def test_append(self):
4446
# Check that `o` is indeed the last element
4547
self.assertEqual(o, itc.Next())
4648

49+
# Clear before the added element might be garbage collected,
50+
# to avoid dangling pointer access.
51+
c.Clear()
52+
4753
def test_remove(self):
4854
c = ROOT.TList()
4955

@@ -69,6 +75,8 @@ def test_remove(self):
6975
with self.assertRaises(ValueError):
7076
c.remove(o1)
7177

78+
c.Clear()
79+
7280
def test_extend(self):
7381
c1 = self.create_tcollection()
7482
c2 = self.create_tcollection()
@@ -105,6 +113,8 @@ def test_count(self):
105113
self.assertEqual(c.count(o1), 2)
106114
self.assertEqual(c.count(o2), 1)
107115

116+
c.Clear()
117+
108118

109119
if __name__ == '__main__':
110120
unittest.main()

bindings/pyroot/pythonizations/test/tseqcollection_itemaccess.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,16 @@ class TSeqCollectionItemAccess(unittest.TestCase):
1212

1313
num_elems = 3
1414

15+
_global_objects = []
16+
1517
# Helpers
1618
def create_tseqcollection(self):
1719
sc = ROOT.TList()
1820
for _ in range(self.num_elems):
1921
o = ROOT.TObject()
20-
# Prevent immediate deletion of C++ TObjects
21-
ROOT.SetOwnership(o, False)
2222
sc.Add(o)
23+
# To prevent deletion of the objects (TList is by default non-owning)
24+
self._global_objects.append(o)
2325

2426
return sc
2527

bindings/pyroot/pythonizations/test/tseqcollection_listmethods.py

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,16 @@ class TSeqCollectionListMethods(unittest.TestCase):
1111

1212
num_elems = 3
1313

14+
_global_objects = []
15+
1416
# Helpers
1517
def create_tseqcollection(self):
1618
sc = ROOT.TList()
1719
for i in reversed(range(self.num_elems)):
18-
sc.Add(ROOT.TObjString(str(i)))
20+
o = ROOT.TObjString(str(i))
21+
sc.Add(o)
22+
# To prevent deletion of the objects (TList is by default non-owning)
23+
self._global_objects.append(o)
1924

2025
return sc
2126

@@ -49,6 +54,10 @@ def test_insert(self):
4954
self.assertEqual(sc.GetEntries(), self.num_elems + 4)
5055
self.assertEqual(sc.At(self.num_elems + 3), o4)
5156

57+
# Clear before the added element might be garbage collected,
58+
# to avoid dangling pointer access.
59+
sc.Clear()
60+
5261
def test_pop(self):
5362
sc = self.create_tseqcollection()
5463
l1 = [elem for elem in sc]
@@ -82,8 +91,10 @@ def test_pop(self):
8291
with self.assertRaises(TypeError):
8392
sc2.pop(1.0)
8493

85-
# Pop a repeated element
86-
sc2.append(ROOT.TObjString("2"))
94+
# Pop a repeated element.
95+
# Keep Python reference so the added element lives beyond the sc2.pop() call:
96+
new_elem = ROOT.TObjString("2")
97+
sc2.append(new_elem)
8798
elem = sc2.pop()
8899
self.assertEqual(sc2.At(0), elem)
89100

0 commit comments

Comments
 (0)