[pypy-commit] pypy default: Another attempt to make it so that weakrefs in RPython are cleared as

[arigo]

Author: Armin Rigo <arigo at tunes.org>
Branch: 
Changeset: r69125:dc505e09021c
Date: 2014-02-13 17:18 +0100
http://bitbucket.org/pypy/pypy/changeset/dc505e09021c/

Log:	Another attempt to make it so that weakrefs in RPython are cleared
	as soon as the finalizer is enqueued. So if the __del__ is called,
	we know that we really have no more strong references to the object
	anywhere at that point in time. Before, due to the incremental GC,
	it would be possible to fetch the content of the weakref at just the
	wrong point in time, and still have the finalizer called afterwards.
	This is explained in https://bugs.pypy.org/issue1687, where this
	causes surprizes.

diff --git a/rpython/memory/gc/incminimark.py b/rpython/memory/gc/incminimark.py
--- a/rpython/memory/gc/incminimark.py
+++ b/rpython/memory/gc/incminimark.py
@@ -1837,6 +1837,11 @@
                 #
                 if self.objects_with_finalizers.non_empty():
                     self.deal_with_objects_with_finalizers()
+                elif self.old_objects_with_weakrefs.non_empty():
+                    # Weakref support: clear the weak pointers to dying objects
+                    # (if we call deal_with_objects_with_finalizers(), it will
+                    # invoke invalidate_old_weakrefs() itself directly)
+                    self.invalidate_old_weakrefs()
 
                 ll_assert(not self.objects_to_trace.non_empty(),
                           "objects_to_trace should be empty")
@@ -1846,9 +1851,7 @@
                 self.more_objects_to_trace.delete()
 
                 #
-                # Weakref support: clear the weak pointers to dying objects
-                if self.old_objects_with_weakrefs.non_empty():
-                    self.invalidate_old_weakrefs()
+                # Light finalizers
                 if self.old_objects_with_light_finalizers.non_empty():
                     self.deal_with_old_objects_with_finalizers()
                 #objects_to_trace processed fully, can move on to sweeping
@@ -2206,6 +2209,12 @@
                     self._recursively_bump_finalization_state_from_2_to_3(y)
             self._recursively_bump_finalization_state_from_1_to_2(x)
 
+        # Clear the weak pointers to dying objects.  Also clears them if
+        # they point to objects which have the GCFLAG_FINALIZATION_ORDERING
+        # bit set here.  These are objects which will be added to
+        # run_finalizers().
+        self.invalidate_old_weakrefs()
+
         while marked.non_empty():
             x = marked.popleft()
             state = self._finalization_state(x)
@@ -2333,7 +2342,9 @@
             ll_assert((self.header(pointing_to).tid & GCFLAG_NO_HEAP_PTRS)
                       == 0, "registered old weakref should not "
                             "point to a NO_HEAP_PTRS obj")
-            if self.header(pointing_to).tid & GCFLAG_VISITED:
+            tid = self.header(pointing_to).tid
+            if ((tid & (GCFLAG_VISITED | GCFLAG_FINALIZATION_ORDERING)) ==
+                        GCFLAG_VISITED):
                 new_with_weakref.append(obj)
             else:
                 (obj + offset).address[0] = llmemory.NULL
diff --git a/rpython/memory/test/gc_test_base.py b/rpython/memory/test/gc_test_base.py
--- a/rpython/memory/test/gc_test_base.py
+++ b/rpython/memory/test/gc_test_base.py
@@ -29,6 +29,7 @@
     GC_CAN_SHRINK_ARRAY = False
     GC_CAN_SHRINK_BIG_ARRAY = False
     BUT_HOW_BIG_IS_A_BIG_STRING = 3*WORD
+    WREF_IS_INVALID_BEFORE_DEL_IS_CALLED = False
 
     def setup_class(cls):
         cls._saved_logstate = py.log._getstate()
@@ -370,15 +371,23 @@
         class A(object):
             count = 0
         a = A()
+        expected_invalid = self.WREF_IS_INVALID_BEFORE_DEL_IS_CALLED
         class B(object):
             def __del__(self):
                 # when __del__ is called, the weakref to myself is still valid
-                # in RPython (at least with most GCs; this test might be
-                # skipped for specific GCs)
-                if self.ref() is self:
-                    a.count += 10  # ok
+                # in RPython with most GCs.  However, this can lead to strange
+                # bugs with incminimark.  https://bugs.pypy.org/issue1687
+                # So with incminimark, we expect the opposite.
+                if expected_invalid:
+                    if self.ref() is None:
+                        a.count += 10  # ok
+                    else:
+                        a.count = 666  # not ok
                 else:
-                    a.count = 666  # not ok
+                    if self.ref() is self:
+                        a.count += 10  # ok
+                    else:
+                        a.count = 666  # not ok
         def g():
             b = B()
             ref = weakref.ref(b)
diff --git a/rpython/memory/test/test_incminimark_gc.py b/rpython/memory/test/test_incminimark_gc.py
--- a/rpython/memory/test/test_incminimark_gc.py
+++ b/rpython/memory/test/test_incminimark_gc.py
@@ -1,6 +1,38 @@
-from rpython.rlib.rarithmetic import LONG_BIT
+from rpython.rtyper.lltypesystem import lltype
+from rpython.rtyper.lltypesystem.lloperation import llop
 
 from rpython.memory.test import test_minimark_gc
 
 class TestIncrementalMiniMarkGC(test_minimark_gc.TestMiniMarkGC):
     from rpython.memory.gc.incminimark import IncrementalMiniMarkGC as GCClass
+    WREF_IS_INVALID_BEFORE_DEL_IS_CALLED = True
+
+    def test_weakref_not_in_stack(self):
+        import weakref
+        class A(object):
+            pass
+        class B(object):
+            def __init__(self, next):
+                self.next = next
+        def g():
+            a = A()
+            a.x = 5
+            wr = weakref.ref(a)
+            llop.gc__collect(lltype.Void)   # make everything old
+            assert wr() is not None
+            assert a.x == 5
+            return wr
+        def f():
+            ref = g()
+            llop.gc__collect(lltype.Void, 1)    # start a major cycle
+            # at this point the stack is scanned, and the weakref points
+            # to an object not found, but still reachable:
+            b = ref()
+            llop.debug_print(lltype.Void, b)
+            assert b is not None
+            llop.gc__collect(lltype.Void)   # finish the major cycle
+            # assert does not crash, because 'b' is still kept alive
+            b.x = 42
+            return ref() is b
+        res = self.interpret(f, [])
+        assert res == True