[prev in list] [next in list] [prev in thread] [next in thread] 

List:       subversion-users
Subject:    Re: Should 'svn help' with APR lifetime debug enabled run without issues?
From:       Jun Omae <jun66j5 () gmail ! com>
Date:       2021-05-03 9:06:13
Message-ID: CAEVLMagzrQa5jpYLUTVfGNQVopN5RpBnUHkatVY6+pLQ_sVAtA () mail ! gmail ! com
[Download RAW message or body]

Hi,

On Wed, Apr 28, 2021 at 5:39 PM Rick van der Zwet
<info@rickvanderzwet.nl> wrote:
> > Ack, but the backtrace does go through Subversion's swig-py bindings an=
d
> > libsvn_fs_fs, so we might be involved nevertheless.
>
> ...
>
> error which seems to be related to the Trac code, I filled it over
> there: https://trac.edgewall.org/ticket/13401

The root cause is that sub pool is doubly destroyed because weakref's
callback is
not invoked when the pools are finalized by cyclic garbage collector
in Python 3.

See also https://bugs.python.org/issue40312

Proposed patch is attached.

[[[
swig-py: Fix doubly destroying memory pool because weakref's callback is no=
t
invoked when it is finalized by cyclic garbage collector.

* subversion/bindings/swig/include/proxy_apr.swg
  (apr_pool_t.valid): Check whether parent pool is valid.

* subversion/bindings/swig/python/tests/pool.py
  (PoolTestCase): Add tests for pools referred from circular reference.
}}}

--
Jun Omae <jun66j5@gmail.com> (=E5=A4=A7=E5=89=8D =E6=BD=A4)

["pools-in-circular-ref.diff" (application/octet-stream)]

swig-py: Fix doubly destroying memory pool because weakref's callback is not
invoked when it is finalized by cyclic garbage collector.

* subversion/bindings/swig/include/proxy_apr.swg
  (apr_pool_t.valid): Check whether parent pool is valid.

* subversion/bindings/swig/python/tests/pool.py
  (PoolTestCase): Add tests for pools referred from circular reference.

Index: subversion/bindings/swig/include/proxy_apr.swg
===================================================================
--- subversion/bindings/swig/include/proxy_apr.swg	(revision 1889441)
+++ subversion/bindings/swig/include/proxy_apr.swg	(working copy)
@@ -142,9 +142,15 @@
         are still valid"""
         try:
           self._is_valid
-          return True
         except AttributeError:
           return False
+        # We must check whether the parent pool is valid even if
+        # the pool is valid because weakref's callback is not
+        # invoked when it is finalized by cyclic garbage collector
+        if self._parent_pool:
+          return self._parent_pool.valid()
+        else:
+          return True
 
       def assert_valid(self):
         """Assert that this memory_pool is still valid."""
Index: subversion/bindings/swig/python/tests/pool.py
===================================================================
--- subversion/bindings/swig/python/tests/pool.py	(revision 1889441)
+++ subversion/bindings/swig/python/tests/pool.py	(working copy)
@@ -19,10 +19,11 @@
 #
 #
 import unittest, weakref, setup_path
-import os, tempfile
+import os, tempfile, gc
 import svn.core, svn.client, libsvn.core
 from svn.core import *
 from libsvn.core import application_pool, GenericSWIGWrapper
+import utils
 
 # Test case for the new automatic pool management infrastructure
 
@@ -208,6 +209,51 @@
     # We can still destroy and create pools at will
     svn_pool_destroy(svn_pool_create())
 
+  def _test_pools_in_circular_reference(self, finalizer=False):
+
+    class Circular(object):
+
+      def __init__(self, pool):
+        self.pool = pool
+        self.loop = None
+
+      if finalizer:
+        def __del__(self):
+          self.pool = self.loop = None
+
+    def create_circularl():
+      pool = Pool(libsvn.core.application_pool)
+      subpool1 = Pool(pool)
+      subpool2 = Pool(pool)
+      circularly1 = Circular(pool)
+      circularly2 = Circular(subpool2)
+      circularly3 = Circular(subpool1)
+      circularly1.loop = circularly3
+      circularly2.loop = circularly1
+      circularly3.loop = circularly2
+      refs = weakref.WeakValueDictionary()
+      refs['pool'] = pool
+      refs['subpool1'] = subpool1
+      refs['subpool2'] = subpool2
+      return refs
+
+    refs = create_circularl()
+    self.assertEqual({'pool', 'subpool1', 'subpool2'},
+                     set(name for name, pool in refs.items()
+                              if pool is not None))
+    gc.collect()
+    self.assertEqual(set(), set(name for name, pool in refs.items()
+                                     if pool is not None))
+
+  def test_pools_in_circular_reference_without_finalizer(self):
+    self._test_pools_in_circular_reference(finalizer=False)
+
+  @unittest.skipIf(not utils.IS_PY3,
+                   "Python 2 cannot collect garbage which involves circular "
+                   "references with finalizer")
+  def test_pools_in_circular_reference_with_finalizer(self):
+    self._test_pools_in_circular_reference(finalizer=True)
+
 def suite():
   return unittest.defaultTestLoader.loadTestsFromTestCase(PoolTestCase)
 


[prev in list] [next in list] [prev in thread] [next in thread]