On Thu, Jul 24, 2025 at 11:41:20AM +0100, Joe Orton wrote:
> On Wed, Jul 23, 2025 at 09:08:48PM +0200, Daniel Sahlberg wrote:
> > Hi,
> >
> > The patch below has been floating in dev@ for about a month. It looks looks
> > like an improvement to me, but it is way out of my comfort zone.
> >
> > @Joe Orton <[email protected]> are you able to test this?
>
> Sure thing, I've fired off a test build against Fedora Rawhide with
> Yasuhito's patch applied, will let you know how it goes.
It failed as below. I'm testing 1.14.5 with the attached patch, which
brings repository.py to match trunk + Yasuhito's patch.
======================================================================
FAIL: test_replay_batons_refcounts
(repository.SubversionRepositoryTestCase.test_replay_batons_refcounts)
Issue SVN-4917: check ref-count of batons created and used in callbacks
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/builddir/build/BUILD/subversion-1.14.5-build/subversion-1.14.5/subversion/bindings/swig/python/tests/repository.py",
line 447, in test_replay_batons_refcounts
self.assertEqual(sys.getrefcount(baton_tuple[2]), baton_tuple[3],
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"leak on baton %s after replay without errors"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
% repr(baton_tuple))
^^^^^^^^^^^^^^^^^^^^
AssertionError: 2 != 0 : leak on baton (b'dir baton', b'', (b'', b'', 11), 0)
after replay without errors
----------------------------------------------------------------------
--- subversion-1.14.5/subversion/bindings/swig/python/tests/mergeinfo.py.10
+++ subversion-1.14.5/subversion/bindings/swig/python/tests/mergeinfo.py
@@ -138,8 +138,8 @@
# ....and now 3 (incref during iteration of each range object)
refcount = sys.getrefcount(r)
- # ....and finally, 4 (getrefcount() also increfs)
- expected = 4
+ # ....and finally, 4 (getrefcount() also increfs, but not as of 3.14)
+ expected = 3 if sys.version_info >= (3, 14) else 4
# Note: if path and index are not '/trunk' and 0 respectively, then
# only some of the range objects are leaking, which is, as far as
--- subversion-1.14.5/subversion/bindings/swig/python/tests/repository.py.10
+++ subversion-1.14.5/subversion/bindings/swig/python/tests/repository.py
@@ -95,7 +95,9 @@
def open_root(self, base_revision, dir_pool=None):
bt = repos.ChangeCollector.open_root(self, base_revision, dir_pool)
- self.batons.append((b'dir baton', b'', bt, sys.getrefcount(bt)))
+ # refcount for the object refered by bt is counted down after
+ # the variable bt is deleted (i.e. after leaving this function)
+ self.batons.append((b'dir baton', b'', bt, sys.getrefcount(bt) - 1))
return bt
def add_directory(self, path, parent_baton,
@@ -104,14 +106,18 @@
copyfrom_path,
copyfrom_revision,
dir_pool)
- self.batons.append((b'dir baton', path, bt, sys.getrefcount(bt)))
+ # refcount for the object refered by bt is counted down after
+ # the variable bt is deleted (i.e. after leaving this function)
+ self.batons.append((b'dir baton', path, bt, sys.getrefcount(bt) - 1))
return bt
def open_directory(self, path, parent_baton, base_revision,
dir_pool=None):
bt = repos.ChangeCollector.open_directory(self, path, parent_baton,
base_revision, dir_pool)
- self.batons.append((b'dir baton', path, bt, sys.getrefcount(bt)))
+ # refcount for the object refered by bt is counted down after
+ # the variable bt is deleted (i.e. after leaving this function)
+ self.batons.append((b'dir baton', path, bt, sys.getrefcount(bt) - 1))
return bt
def add_file(self, path, parent_baton,
@@ -119,13 +125,17 @@
bt = repos.ChangeCollector.add_file(self, path, parent_baton,
copyfrom_path, copyfrom_revision,
file_pool)
- self.batons.append((b'file baton', path, bt, sys.getrefcount(bt)))
+ # refcount for the object refered by bt is counted down after
+ # the variable bt is deleted (i.e. after leaving this function)
+ self.batons.append((b'file baton', path, bt, sys.getrefcount(bt) - 1))
return bt
def open_file(self, path, parent_baton, base_revision, file_pool=None):
bt = repos.ChangeCollector.open_file(self, path, parent_baton,
base_revision, file_pool)
- self.batons.append((b'file baton', path, bt, sys.getrefcount(bt)))
+ # refcount for the object refered by bt is counted down after
+ # the variable bt is deleted (i.e. after leaving this function)
+ self.batons.append((b'file baton', path, bt, sys.getrefcount(bt) - 1))
return bt
def close_edit(self, pool=None):
@@ -138,7 +148,7 @@
class BatonCollectorErrorOnClose(BatonCollector):
"""Same as BatonCollector, but raises an Exception when close the
- file/dir specfied by error_path"""
+ file/dir specified by error_path"""
def __init__(self, fs_ptr, root, pool=None, notify_cb=None, error_path=b''):
BatonCollector.__init__(self, fs_ptr, root, pool, notify_cb)
self.error_path = error_path
@@ -429,29 +439,35 @@
root = fs.revision_root(self.fs, self.rev)
editor = BatonCollector(self.fs, root)
e_ptr, e_baton = delta.make_editor(editor)
+ refcount_at_first = sys.getrefcount(e_ptr)
repos.replay(root, e_ptr, e_baton)
- for baton in editor.batons:
- self.assertEqual(sys.getrefcount(baton[2]), 2,
+ for baton_tuple in editor.batons:
+ # baton_tuple: 4-tuple(baton_type: bytes, node: bytes, bt: baton,
+ # expected_refcount_of_bt: int)
+ self.assertEqual(sys.getrefcount(baton_tuple[2]), baton_tuple[3],
"leak on baton %s after replay without errors"
- % repr(baton))
+ % repr(baton_tuple))
del e_baton
- self.assertEqual(sys.getrefcount(e_ptr), 2,
+ self.assertEqual(sys.getrefcount(e_ptr), refcount_at_first,
"leak on editor baton after replay without errors")
editor = BatonCollectorErrorOnClose(self.fs, root,
error_path=b'branches/v1x')
e_ptr, e_baton = delta.make_editor(editor)
+ refcount_at_first = sys.getrefcount(e_ptr)
self.assertRaises(SubversionException, repos.replay, root, e_ptr, e_baton)
batons = editor.batons
# As svn_repos_replay calls neither close_edit callback nor abort_edit
- # if an error has occured during processing, references of Python objects
- # in decendant batons may live until e_baton is deleted.
+ # if an error has occurred during processing, references of Python objects
+ # in descendant batons may live until e_baton is deleted.
del e_baton
- for baton in batons:
- self.assertEqual(sys.getrefcount(baton[2]), 2,
+ for baton_tuple in batons:
+ # baton_tuple: 4-tuple(baton_type: bytes, node: bytes, bt: baton,
+ # expected_refcount_of_bt: int)
+ self.assertEqual(sys.getrefcount(baton_tuple[2]), baton_tuple[3],
"leak on baton %s after replay with an error"
- % repr(baton))
- self.assertEqual(sys.getrefcount(e_ptr), 2,
+ % repr(baton_tuple))
+ self.assertEqual(sys.getrefcount(e_ptr), refcount_at_first,
"leak on editor baton after replay with an error")
def test_delta_editor_apply_textdelta_handler_refcount(self):
